Support blob URLs on ES6 with USE_ES6_IMPORT_META enabled #23804
Conversation
| if (Module['mainScriptUrlOrBlob']) { | ||
| var pthreadMainJs = Module['mainScriptUrlOrBlob']; | ||
| if (typeof pthreadMainJs != 'string') { | ||
| pthreadMainJs = URL.createObjectURL(pthreadMainJs); |
There was a problem hiding this comment.
It looks like pthreadMainJs it not being used?
There was a problem hiding this comment.
Oops, I had a typo on the next line. Should have read new Worker(pThreadMainJS). I’ll submit a fix soon.
There was a problem hiding this comment.
I’ve corrected it but I’m also ok to throw an error in case mainScriptUrlOrBlob is set under ES6_MODULES given -sASSERTIONS.
There was a problem hiding this comment.
Can we split this up and into a PR just for mainScriptUrlOrBlob support? And include a test for that?
I think the change to use new Worker(import.meta.url..) can / should be separate .. and might take a longer to land since we want to confirm that it works in all the different bundlers.
There was a problem hiding this comment.
Also, can you remove the USE_ES6_IMPORT_META from the title (that setting was removed).
There was a problem hiding this comment.
Yes, I can do this.
src/lib/libpthread.js
Outdated
| // instead of just using new URL(import.meta.url) because bundler's only recognize | ||
| // the first case in their bundling step. The latter ends up producing an invalid | ||
| // URL to import from the server (e.g., for webpack the file:// path). | ||
| worker = new Worker(new URL('{{{ TARGET_JS_NAME }}}', import.meta.url), {{{ pthreadWorkerOptions }}}); |
There was a problem hiding this comment.
Man.. this limitation of the bundlers really makes our generated code nasty. I wonder if the bundlers can be fixed... its really a shame to have all this extra stuff just to make them happy.
There was a problem hiding this comment.
I wonder if we could just fix webpack to support new Worker(import.meta.url) which is what we actually want to write here
There was a problem hiding this comment.
I found a webpack bug from way back about this: webpack/webpack#12638
There was a problem hiding this comment.
My understanding from webpack/webpack#12638 (comment) is that this is fixed now, so what’s the move? To keep the mainScriptUrlOrBlob path and default to new Worker(import.meta.url, options), eliminating the new URL calls/paths?
There was a problem hiding this comment.
Fantastic. I think we should probably check with all the different bundlers, but assuming we can go with new Worker(import.meta.ur) that sounds like the best solution.
Re-adding support for mainScriptUrlOrBlob sounds good too.
src/lib/libpthread.js
Outdated
| { | ||
| if (import.meta.url.startsWith('blob:')) { | ||
| // If our import.meta.url is a blob, we should just use it. | ||
| worker = new Worker(import.meta.url, {{{ pthreadWorkerOptions }}}); |
There was a problem hiding this comment.
I think this syntax is not currently supported by webpack :( Although it does look like it now is support by vite, which is good!
There was a problem hiding this comment.
shouldn't there be some mechanism to make emscripten use worker = new Worker(import.meta.url, ...)?
i was excited to hear about USE_ES6_IMPORT_META but it seems that's the bundler hack with import.meta.url as the base.
|
While you're tinkering there, why isn't |
JoeOsborn
force-pushed
the
support-bloburls-importmeta
branch
from
d31507d to
1e1251a
Compare
Per the discussion in emscripten-core#23769, implement this policy: - If mainScriptUrlOrBlob is set, create the worker as in the old non-use-import-meta case - Otherwise, use import.meta.url
JoeOsborn
force-pushed
the
support-bloburls-importmeta
branch
from
1e1251a to
467de0c
Compare
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.
Per the discussion in #23769, implement this policy:
I know I didn't add any automated tests; I'd appreciate pointers on how to implement tests for this.