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

Use unescaped import specifier #1387

Conversation

edoardocavazza
Copy link
Contributor

Since version 0.4.0, es-module-lexer exposes a n property for each import specifier with its unescaped source. This is useful (and more accurate) for virtual modules like \0commonHelpers.

What I did

  1. try to use the n property of the specifier, fallback to the previous version if missing.

@changeset-bot
Copy link

changeset-bot bot commented Apr 8, 2021

⚠️ No Changeset found

Latest commit: a971dbe

Merging this PR will not cause a version bump for any packages. If these changes should not result in a new version, you're good to go. If these changes should result in a version bump, you need to add a changeset.

This PR includes no changesets

When changesets are added to this PR, you'll see the packages that this PR includes changesets for and the associated semver types

Click here to learn what changesets are, and how to add one.

Click here if you're a maintainer who wants to add a changeset to this PR

@edoardocavazza
Copy link
Contributor Author

Hello @LarsDenBakker, I am sorry for the mention :( Could you please review this PR? It fixes a bug with the import of the commonjs helper. Thank you!

@LarsDenBakker
Copy link
Member

This is interesting.. but in the @web/dev-server-rollup we already handle these null character. Could you elaborate which use case this fixes? Also can JS handle the unescaped import characters?

@edoardocavazza
Copy link
Contributor Author

The problem occurs when using both esbuild and commonjs plugins. I think that esbuild output "reverts" the null character transformation 🤔

@LarsDenBakker
Copy link
Member

I'm not sure that's the case, we transform the import to a whole different import. Do you see that this change fixes a specific problem? What I saw in the past was that the null character would be sent to the browser but not sent back in the request URL.. making it useless.

@edoardocavazza
Copy link
Contributor Author

I'll try to setup a unit test for reproduction.

@edoardocavazza
Copy link
Contributor Author

Hello @LarsDenBakker, I added an unit test with the use case. If you revert the proposed fix, you can see that null character is not handled after esbuild transformation.

@LarsDenBakker
Copy link
Member

Thanks, should be good to go then.

@LarsDenBakker LarsDenBakker merged commit b45f294 into modernweb-dev:master Apr 20, 2021
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.

None yet

2 participants