-
Notifications
You must be signed in to change notification settings - Fork 3.4k
Remove USE_ES6_IMPORT_META setting #23171
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
Conversation
@@ -44,7 +44,7 @@ export function processMacros(text, filename) { | |||
// Also handles #include x.js (similar to C #include <file>) | |||
export function preprocess(filename) { | |||
let text = read(filename); | |||
if (EXPORT_ES6 && USE_ES6_IMPORT_META) { | |||
if (EXPORT_ES6) { | |||
// `eval`, Terser and Closure don't support module syntax; to allow it, |
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.
Potential follow up: I'm pretty sure closure supports module syntax now. Not sure about terser.
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.
I looked into this and will try to come up with followup. For now is seems like the eval
phase is the one were we still need this.
2a546ac
to
79f3fef
Compare
FWIW, Cloudflare Workers doesn't support That said, I don't think that should block this PR, since users can simply omit |
Or presumably one could just use a bundler before deploying to cloudflare? Which seems like a likely/good choice anyway, right? |
79f3fef
to
4800608
Compare
This setting was added (IIUC) as a temporary workaround for the lack or support for `import.meta` in browser and tooling. However `import.meta` has been at stage 4 for almost 5 years now: tc39/proposal-import-meta#21 Webback (released in 2020) also has builtin support for `import.meta`, so I would hope the use case for disabling this setting no longer exists. See emscripten-core#9234 and emscripten-core#8729
4800608
to
8d46778
Compare
This fix breaks cross-origin workers via mainScriptUrlOrBlob. Since the worker script is now hard-coded as the URL, we don't have the ability to use an inline worker to importScripts or use a downloaded blob. |
I was using this option for a use case that was probably not the intention of this option. I used this option to avoid a bundler from embedding the wasm file. And without this option I now need to find a way in the tooling that uses the output to avoid inlining For those who are reading this in the future I use the following "hack" to work around this issue. Replace |
Interesting. It does sounds odd that this would be the option used to prevent the bundler from embedding the wasm. Can I ask what bundler you are using? Have you looked for a blunder setting that might avoid this? |
I am using vite 6 that is using rollup as it's bundler and I have tried using several of the suggested options for this but none of them seemed to work for my project. |
Is there an open bug again vite for that? It certainly seems like a bundler bug/feature, rather than something emscripten should be having to deal with. |
I have not found a bug about that so I will probably need to report one. But that will probably take me some time to create something that can be used to reproduce it. And the work around can be a possible "fix/hack" when someone else also has a problem with their bundler. |
Spun out from #23804 . This is a fix for #23769 which was introduced by #23171 , where the use of the bundler pattern hard-coding a script name was made to apply to all ES6 exports. `mainScriptUrlOrBlob` is necessary under es6 exports to support running a threaded emscripten module from another origin, e.g. to distribute an emscripten module on a CDN (the script will load for the main thread but won’t be allowed to create workers). Using blobs is an effective workaround.
I have exactly the same use case as what @dlemstra mentioned above for |
Removing USE_ES6_IMPORT_META will result in library users being unable to customize the loading location of .js when using multi-threaded .wasm. // emsdk 4.0.1
allocateUnusedWorker() {
var worker;
var workerOptions = { type: "module", name: "em-pthread" };
var pthreadMainJs = _scriptName;
if (Module["mainScriptUrlOrBlob"]) {
pthreadMainJs = Module["mainScriptUrlOrBlob"];
if (typeof pthreadMainJs != "string") {
pthreadMainJs = URL.createObjectURL(pthreadMainJs);
}
}
worker = new Worker(pthreadMainJs, workerOptions);
PThread.unusedWorkers.push(worker);
} Users can set the loading location through mainScriptUrlOrBlob const moduleConfig = {
locateFile: (file: string) => {
const path = `${demoDir}.wasm`;
return path;
},
mainScriptUrlOrBlob: `${demoDir}.js`
}; The multi-threaded .js code obtained when USE_ES6_IMPORT_META=1 is as follows: // emsdk 4.0.4
allocateUnusedWorker() {
var worker;
worker = new Worker(new URL("demo.js", import.meta.url), { type: "module", name: "em-pthread" });
PThread.unusedWorkers.push(worker);
} At this time, users can only load files from the following path `{import.meta.url}/demo.js` Cannot load files from the specified location unless manually modifying demo.js `{import.meta.url}/dir1/demo.js`
`{import.meta.url}/dir1/dir2/demo.js`
`{import.meta.url}/dir1/dir2/dir3/demo.js` How to customize the loading location after removing USE_ES6_IMPORT_META |
@jinwuwu001 Could you re-test with Emscripten 4.0.6 or later? I think your use-case was fixed via commit 9e21488. |
This setting was added (IIUC) as a temporary workaround for the lack or support for
import.meta
in browser and tooling.However
import.meta
has been at stage 4 for almost 5 years now: tc39/proposal-import-meta#21Webback (released in 2020) also has builtin support for
import.meta
, so I would hope the use case for disabling this setting no longer exists.See #9234 and #8729