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

Update to [email protected] #38450

Closed
wants to merge 4 commits into from

Conversation

guybedford
Copy link
Contributor

@guybedford guybedford commented Apr 28, 2021

This version of [email protected] includes support for non-identifier exports thanks to @nicolo-ribaudo.

This will mean import { "?" as name } from 'cjs' for exports['?'] = 'export' can be supported.

In versions of Node.js without string import support, import * as m from 'cjs'; m['?'] can be used instead for these cases as well.

Test included to verify the behaviour.

@guybedford
Copy link
Contributor Author

/cc @nodejs/modules

@github-actions github-actions bot added the needs-ci PRs that need a full CI run. label Apr 28, 2021
@guybedford guybedford requested a review from bmeck April 28, 2021 11:19
@aduh95
Copy link
Contributor

aduh95 commented Apr 28, 2021

#37712 (comment)

[cjs-module-lexer]: https://github.com/guybedford/cjs-module-lexer/tree/1.1.1

@bmeck
Copy link
Member

bmeck commented Apr 28, 2021

reading v8/v8@81d168d , do we ensure we don't have unpaired surrogates in the lexer? Luckily even w/ the internal name stuff v8 does they didn't use the same space for the exports so it should be safe

@guybedford
Copy link
Contributor Author

@bmeck the module load would fail on unpaired surrogates due to the CJS loader failing execution as it wouldn't be valid JS source text. The lexer itself does not bail on unpaired surrogates though (it handles stepping through surrogates but not validating them for performance), so would still return the exports as part of its analysis but this would be unobservable to users.

@bmeck
Copy link
Member

bmeck commented Apr 28, 2021

@guybedford

My concern is for valid CJS that exports unpaired surrogates like (this does not play with WASM's requirement of valid UTF8):

// '\u{D83C}\u{DF10}' is 🌐, 2 surrogates
module.exports = {
  '\u{D83C}': 123,
  '\uDF10': 456,
};

@guybedford
Copy link
Contributor Author

Ah thanks for clarifying. Yes we will parse and support unpaired surrogates just like normal JS since the lexer runs as UTF-16. Is that a problem?

@nicolo-ribaudo
Copy link
Contributor

I can open a PR to cjs-module-lexer to disallow them, but reading tc39/ecma262#2154 I think that the spec wouldn't mandate that imported non-ESM modules must only have valid unicode exports. As far as I understand, it only mandates that you cannot have an import/export with unparied surrogates in an ESM file.

i.e. if we merge this PR without disallowing unparied surrogates, I don't think this would violate the spec:

// dep.cjs
module.exports = {
  '\u{D83C}': 123,
  '\uDF10': 456,
};
import * as dep from "./dep.cjs";

dep["\uDF10"]; // 456, not undefined

@bmeck
Copy link
Member

bmeck commented Apr 28, 2021

The spec was written in a way to explicitly ban them and was generally thought that UTF8 compatibility was desired so WASM could properly integrate against any module JS deals with. If WASM directly imported that CJS module I'm unclear what would happen but it certainly wouldn't be able to use those exports. I'd prefer we disable them just to ensure compatibility unless there is reason to allow them.

@nicolo-ribaudo
Copy link
Contributor

nicolo-ribaudo commented Apr 28, 2021

I'll prepare a PR. "disallow" = "ignore it", right? (not throwing)

@bmeck
Copy link
Member

bmeck commented Apr 28, 2021

@nicolo-ribaudo yes, just drop them/ignore them. As long as they don't show up in the exported names we should be safe. No reason to error/throw.

@guybedford guybedford changed the title Update to [email protected] Update to [email protected] Apr 29, 2021
@guybedford
Copy link
Contributor Author

@bmeck I've updated to [email protected] here with support for unicode escapes in strings and surrogate validation as discussed.

Copy link
Member

@bmeck bmeck left a comment

Choose a reason for hiding this comment

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

let's do this~, LGTM

@nodejs-github-bot
Copy link
Collaborator

@bmeck
Copy link
Member

bmeck commented Apr 29, 2021

can we get a rocket emoji reaction for a fast track to this since it is trying to sync up w/ native ESM that is out in 16?

Copy link
Contributor

@aduh95 aduh95 left a comment

Choose a reason for hiding this comment

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

RSLGTM

@nodejs-github-bot
Copy link
Collaborator

Copy link
Member

@Trott Trott left a comment

Choose a reason for hiding this comment

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

Rubber-stamp LGTM

@Trott Trott added the fast-track PRs that do not need to wait for 48 hours to land. label Apr 29, 2021
@nodejs-github-bot
Copy link
Collaborator

guybedford added a commit that referenced this pull request Apr 30, 2021
PR-URL: #38450
Reviewed-By: Bradley Farias <[email protected]>
Reviewed-By: Antoine du Hamel <[email protected]>
Reviewed-By: Jan Krems <[email protected]>
Reviewed-By: Rich Trott <[email protected]>
@guybedford
Copy link
Contributor Author

guybedford commented Apr 30, 2021

Landed in 50991df.

@guybedford guybedford closed this Apr 30, 2021
guybedford added a commit that referenced this pull request Apr 30, 2021
PR-URL: #38450
Reviewed-By: Bradley Farias <[email protected]>
Reviewed-By: Antoine du Hamel <[email protected]>
Reviewed-By: Jan Krems <[email protected]>
Reviewed-By: Rich Trott <[email protected]>
@aduh95 aduh95 removed the fast-track PRs that do not need to wait for 48 hours to land. label Apr 30, 2021
targos pushed a commit that referenced this pull request May 3, 2021
PR-URL: #38450
Reviewed-By: Bradley Farias <[email protected]>
Reviewed-By: Antoine du Hamel <[email protected]>
Reviewed-By: Jan Krems <[email protected]>
Reviewed-By: Rich Trott <[email protected]>
@targos targos mentioned this pull request May 3, 2021
targos pushed a commit that referenced this pull request May 30, 2021
PR-URL: #38450
Reviewed-By: Bradley Farias <[email protected]>
Reviewed-By: Antoine du Hamel <[email protected]>
Reviewed-By: Jan Krems <[email protected]>
Reviewed-By: Rich Trott <[email protected]>
targos pushed a commit that referenced this pull request Jun 5, 2021
PR-URL: #38450
Reviewed-By: Bradley Farias <[email protected]>
Reviewed-By: Antoine du Hamel <[email protected]>
Reviewed-By: Jan Krems <[email protected]>
Reviewed-By: Rich Trott <[email protected]>
targos pushed a commit that referenced this pull request Jun 5, 2021
PR-URL: #38450
Reviewed-By: Bradley Farias <[email protected]>
Reviewed-By: Antoine du Hamel <[email protected]>
Reviewed-By: Jan Krems <[email protected]>
Reviewed-By: Rich Trott <[email protected]>
targos pushed a commit that referenced this pull request Jun 11, 2021
PR-URL: #38450
Reviewed-By: Bradley Farias <[email protected]>
Reviewed-By: Antoine du Hamel <[email protected]>
Reviewed-By: Jan Krems <[email protected]>
Reviewed-By: Rich Trott <[email protected]>
guybedford added a commit to guybedford/node that referenced this pull request Jul 17, 2021
PR-URL: nodejs#38450
Reviewed-By: Bradley Farias <[email protected]>
Reviewed-By: Antoine du Hamel <[email protected]>
Reviewed-By: Jan Krems <[email protected]>
Reviewed-By: Rich Trott <[email protected]>
richardlau pushed a commit that referenced this pull request Jul 23, 2021
PR-URL: #38450
Backport-PR-URL: #39422
Reviewed-By: Bradley Farias <[email protected]>
Reviewed-By: Antoine du Hamel <[email protected]>
Reviewed-By: Jan Krems <[email protected]>
Reviewed-By: Rich Trott <[email protected]>
@richardlau richardlau mentioned this pull request Jul 23, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
needs-ci PRs that need a full CI run.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants