/cc @nodejs/modules

#37712 (comment)

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

@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.

@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,
};

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?

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

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.

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

@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.

@bmeck I've updated to cjs-module-lexer@1.2.1 here with support for unicode escapes in strings and surrogate validation as discussed.

CI: https://ci.nodejs.org/job/node-test-pull-request/37802/

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?

CI: https://ci.nodejs.org/job/node-test-pull-request/37811/

CI: https://ci.nodejs.org/job/node-test-pull-request/37814/

Landed in 50991df.