[js-api] Skip string constant imports in ESM integration - #124
guybedford wants to merge 2 commits into
Conversation
The ESM integration sets the imported string module to "wasm:js/string-constants" but never checked it when computing requested modules or linking imports, so a string constant import would have been resolved as a module specifier. Add an [[ImportedStringModule]] check alongside the existing builtin check in parse and ExecuteModule. Adds ESM and source phase tests for string constants imported from wasm:js/string-constants.
|
@chicoxyzzy while you have the context, I'd value your sign-off on this correctness fix if you have a moment. |
chicoxyzzy
left a comment
chicoxyzzy
left a comment
There was a problem hiding this comment.
The parse-time requestedModules skip looks right. String constants stay out of the specifier set the same way builtins already do.
HostGetModuleSourceModuleRecord still puts wasm:js/string-constants and wasm:js-string into requestedModules. I am not sure whether that path needs the same skip.
|
@chicoxyzzy good catch thanks, yes that should be included as well. I've abstracted a shared helper for this now to ensure all codepaths stay together. Will land this fix further tomorrow, unless there's further feedback. |
|
Reserved Firefox and WebKit still check every import field first. V8 only sees |
|
Yes, intended, the prefix reservation is only for public imports. User string consts starting with |
This branch has not been deployed
2 participants
#115 landed support for importing a given builtin name for a string constant, but the name was not remove from the computed set of imports like other builtins.
This fixes that sso that
[[ImportedStringModule]]is excluded alongside the builtins in the same way so it is not treated as a normal import.Tests are added to verify the behavior.