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(esm-shim): pattern matching #1560

Merged
merged 4 commits into from
Aug 26, 2023

Conversation

klevente
Copy link
Contributor

@klevente klevente commented Aug 16, 2023

Rollup Plugin Name: esm-shim

This PR contains:

  • bugfix
  • feature
  • refactor
  • documentation
  • other

Are tests included?

  • yes (bugfixes and features will not be merged without tests)
  • no

Breaking Changes?

  • yes (breaking changes will not be merged unless absolutely necessary)
  • no

If yes, then include "BREAKING CHANGES:" in the first commit message body, followed by a description of what is breaking.

List any relevant issue numbers:

Description

When using esm-shim to insert the required variables inside ES Modules, the regex matching logic was not working correctly, the matchAllPolyfill function did not return the index from where in the file the matches was found. This led to an incorrect calculation on where to put the shim in the bundled file, which could cause invalid JS to be generated.

For example take a look at this Blitz: https://stackblitz.com/edit/node-hz948e?file=index.js.

Since the String.matchAll function is not available in this project, as lib is set to es6 in tsconfig.json, I decided to modify the polyfill to correctly return the indices of where the matches start. I also refactored the code a bit so it does only what it needs to - find the last ESM import so it know to place the shim after it.

I added 2 test cases to test this feature: 1 with a single ESM import, and 1 with multiple imports.

I also tested a solution where I used String.matchAll, and that one worked out of the box - if whenever the lib option is updated to a newer standard, it'd be better to just use that instead.

@klevente klevente requested a review from tada5hi as a code owner August 16, 2023 07:01
@tada5hi tada5hi changed the title Fix esm-shim pattern matching fix: esm-shim pattern matching Aug 16, 2023
Copy link
Member

@tada5hi tada5hi left a comment

Choose a reason for hiding this comment

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

looks good to me 👍
Thank you!

@shellscape shellscape changed the title fix: esm-shim pattern matching fix(esm-shim): pattern matching Aug 17, 2023
@klevente
Copy link
Contributor Author

@shellscape could I ask for a review please? 🙏

@tada5hi tada5hi requested a review from a team August 23, 2023 19:41
@shellscape shellscape merged commit 0f73e2a into rollup:master Aug 26, 2023
14 checks passed
@shellscape
Copy link
Collaborator

thanks!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants