Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

fix: support multiple imports of one module with multiple lines #30314

Merged
merged 2 commits into from
Nov 22, 2024

Conversation

wenfw
Copy link
Contributor

@wenfw wenfw commented Sep 29, 2024

The . in RegExp matches all characters except line terminators, so the following won't match:

import {
  a,
  b,
  c,
} from 'x';

Changing it to [^;'"] allows it to match all characters, including \n, except for ;'", which resolves the issue.

Using the current regular expression /(?<=^|\s)import (.+?) from ['"](.*?)['"]/g will cause the new test cases to fail.

Additional details

Steps to test

How has the user experience changed?

PR Tasks

@CLAassistant
Copy link

CLAassistant commented Sep 29, 2024

CLA assistant check
All committers have signed the CLA.

@cypress-app-bot
Copy link
Collaborator

@jennifer-shehane
Copy link
Member

@wenfw Could you write a test for this change?

@wenfw
Copy link
Contributor Author

wenfw commented Oct 10, 2024

@wenfw Could you write a test for this change?

@jennifer-shehane Test cases have been updated. Thanks.

Copy link
Contributor

@AtofStryker AtofStryker left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hi @wenfw. Thank you for the contribution! At first glance I am not sure exactly what this is solving. Can you add a more descriptive PR description to describe the current problem and how your solution addresses that problem?

@wenfw
Copy link
Contributor Author

wenfw commented Oct 12, 2024

@AtofStryker The . in RegExp matches all characters except line terminators, so the following won't match:

import {
  a,
  b,
  c,
} from 'x';

Changing it to [^;'"] allows it to match all characters, including \n, except for ;'", which resolves the issue.

Using the current regular expression /(?<=^|\s)import (.+?) from ['"](.*?)['"]/g will cause the new test cases to fail.

@AtofStryker AtofStryker self-requested a review October 15, 2024 15:11
Copy link

@allexiusw allexiusw left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@AtofStryker
Copy link
Contributor

@wenfw everything looks good. Are you able to add a changelog entry?

@wenfw wenfw force-pushed the patch-1 branch 2 times, most recently from 89e7724 to f61adeb Compare November 22, 2024 07:20
e.g.
```
import {
  a,
  b,
  c,
} from 'x';
```

chore: remove unnecessary deep equal

Co-authored-by: Bill Glesias <[email protected]>
@AtofStryker AtofStryker merged commit 12df40e into cypress-io:develop Nov 22, 2024
69 checks passed
@wenfw wenfw deleted the patch-1 branch November 25, 2024 01:42
@MikeMcC399
Copy link
Contributor

MikeMcC399 commented Nov 25, 2024

@AtofStryker

I'm not sure if this PR has gone through correctly regarding changelog and release.

  1. If the PR only affects npm/vite-plugin-cypress-esm then I believe that the npm/vite-plugin-cypress-esm/CHANGELOG.md entry is created automatically on a successful release, so there shouldn't be an entry for this PR in cli/CHANGELOG.md.
  2. Although the PR has merged into the develop branch, the merge has CircleCI failures, so no release was carried out. See https://app.circleci.com/pipelines/github/cypress-io/cypress/65765, meaning also that there is no npm/vite-plugin-cypress-esm/CHANGELOG.md created yet.

@jennifer-shehane
Copy link
Member

@MikeMcC399 Thanks for surfacing! We're looking into this.

@AtofStryker
Copy link
Contributor

AtofStryker commented Dec 4, 2024

Looks like this released yesterday to npm https://www.npmjs.com/package/@cypress/vite-plugin-cypress-esm. When the percy job fails it is fairly hard to rectify and usually means it will release on the next passing merge to develop, which happened to be 06aeb9d (which also removes the entry).

@cypress-bot
Copy link
Contributor

cypress-bot bot commented Dec 4, 2024

Released in 13.16.1.

This comment thread has been locked. If you are still experiencing this issue after upgrading to
Cypress v13.16.1, please open a new issue.

@cypress-bot cypress-bot bot locked as resolved and limited conversation to collaborators Dec 4, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants