-
Notifications
You must be signed in to change notification settings - Fork 3.5k
Use new URL(..., import.meta.url) pattern in minimal runtime if EXPORT_ES6 is enabled
#26039
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
base: main
Are you sure you want to change the base?
Conversation
311043c to
af94025
Compare
|
Ignore Copilot; GitHub auto-enabled that setting for my account 🙃 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pull request overview
This PR adds support for bundler-friendly new URL(..., import.meta.url) pattern in minimal runtime when EXPORT_ES6 is enabled, specifically for the MINIMAL_RUNTIME_STREAMING_WASM_INSTANTIATION feature. This enables better integration with modern bundlers like Vite that recognize and transform this pattern.
- Introduces conditional logic to use
new URL()pattern when bothEXPORT_ES6andMINIMAL_RUNTIME_STREAMING_WASM_INSTANTIATIONare enabled - Adds test to verify the generated code contains the expected URL pattern
- Handles special case for Audio Worklet environments where
new URL()cannot be used
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 3 comments.
| File | Description |
|---|---|
| src/postamble_minimal.js | Adds preprocessor directives to conditionally generate new URL() pattern for wasm module loading based on ES6 and Audio Worklet settings |
| test/test_other.py | Adds test to verify that ES6 modules with streaming instantiation generate correct URL resolution using import.meta.url |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
af94025 to
13afe52
Compare
|
cc @RReverser |
RReverser
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We already do this for the non-minimal runtime, so makes sense to me.
Technically it does add a few extra bytes to the minimal runtime's output, but I suppose that's fine? @sbc100
|
|
||
| src = read_file('test.mjs') | ||
| # Verify that the generated code uses import.meta.url for the wasm URL | ||
| self.assertContained("new URL('test.wasm', import.meta.url)", src) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We have test_vite in test_browser.py what should be able to not only test that this string exists but that it works in the vite bundler.
IIUC we could add a variant of that test that uses -sMINIMAL_RUNTIME -sMINIMAL_RUNTIME_STREAMING_WASM_INSTANTIATION ?
| instantiatePromise = | ||
| #endif | ||
| WebAssembly.instantiateStreaming(fetch('{{{ TARGET_BASENAME }}}.wasm'), imports).then((output) => { | ||
| WebAssembly.instantiateStreaming(fetch(moduleUrl), imports).then((output) => { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You could use the same trick we use in src/lib/libpthread.js to avoid creating a new variable here maybe?
i.e. do {{{ moduleUrl }}} here and then define moduleUrl inside a {{{ }}} block above?
This should make it possible to use
MINIMAL_RUNTIME_STREAMING_WASM_INSTANTIATIONwith bundlers like Vite that recognize and transformnew URL('...', import.meta.url).