wasm-bindgen-rayon and new OMT plugin #1007
Conversation
lib/client-bundle-plugin.js
Outdated
| const inlineDefines = [ | ||
| ...allModules.map( | ||
| (item) => | ||
| `nextDefineUri=location.origin+${resolveFileUrl(item)};${ |
There was a problem hiding this comment.
@jakearchibald Quick question: here I'm just calling resolveFileUrl(item) since our resolveFileUrl only needs fileName property anyway, but I see that other calls in the same file use
return resolveFileUrl({
fileName: clientEntry.fileName,
moduleId,
format,
});or
return resolveFileUrl({
fileName: entry.fileName,
moduleId,
format: outputOptions.format,
});instead of just passing object directly.
Should I do the same?
There was a problem hiding this comment.
I think it's fine. If we change format there'll be a lot of refactoring anyway.
|
I’m not sure what broke, but staging version doesn’t work :D |
|
Hm, it worked locally, let me recheck. (If only we had tests... :P) |
|
Heh, it's something that I first assumed wouldn't work, but then tried with |
|
Ah no, that's some minifier issue where it doesn't see one of the variable usages. Sigh. |
|
"Fixed". |
RReverser
force-pushed
the
wasm-bindgen-rayon
branch
from
63b992c to
dd95ea8
Compare
RReverser
force-pushed
the
wasm-bindgen-rayon
branch
from
dd95ea8 to
57217c8
Compare
There was a problem hiding this comment.
There's something not quite right here. With the current version of squoosh.app we ensure that the HTML contains module definitions for everything needed for the first interaction, so we can get to first interaction without external dependencies.
However, this version introduces https://deploy-preview-1007--squoosh.netlify.app/c/logo-with-text.svg-8d769ff2.js as an external module needed for first-interaction.
The HTML contains the definition for this module, but it doesn't appear to be used.
| const inlineDefines = [ | ||
| ...allModules.map( | ||
| (item) => | ||
| `self.nextDefineUri=location.origin+${resolveFileUrl(item)};${ |
There was a problem hiding this comment.
I know we aren't governed by https://url.spec.whatwg.org/#goals, but maybe worth considering:
Standardize on the term URL. URI and IRI are just confusing. In practice a single algorithm is used for both so keeping them distinct is not helping anyone.
There was a problem hiding this comment.
I guess this makes sense; however the right place to change it would be upstream in https://github.com/surma/rollup-plugin-off-main-thread/blob/master/loader.ejs first, as this name comes from there (and I want to keep Squoosh loader not too different from upstream one to make future updates easier).
This comment has been minimized.
This comment has been minimized.
Hmm, good catch. Maybe it's not correctly defined then, let me recheck. |
This gives inline `define` calls a chance to define dependencies for earlier `define` calls on the same page.
Hah, right, it's actually something I correctly handled first, but then reverted because app seemed to work fine anyway. Fixed. |
|
I could also put the dependencies first in the HTML, but I'm a little worried that dependencies-of-dependencies might go wrong. |
In theory, Rollup should really take care of this - as in, one should be able to say "if module size is < given threshold, inline it like you would if it wasn't code-split". |
OH DO I HAVE SOMETHING FOR YOU |
WELL DO YOU?! |
|
Needed to finish the post :D |
Emscripten uses dynamic `import(fullUrlOfTheMainJS)` in generated Workers. It appears that upstream rollup-plugin-off-main-thread never handled this correctly, but we had a customization in Squoosh loader template that allowed those anyway, which I removed together with most other customizations in #1007, thus breaking Emscripten codecs. This adds a hotfix back, but we need to fix this properly upstream too (TBD separately).
Emscripten uses dynamic `import(fullUrlOfTheMainJS)` in generated Workers. It appears that upstream rollup-plugin-off-main-thread never handled this correctly, but we had a customization in Squoosh loader template that allowed those anyway, which I removed together with most other customizations in #1007, thus breaking Emscripten codecs. This adds a hotfix back, but we need to fix this properly upstream too (TBD separately).
Emscripten uses dynamic `import(fullUrlOfTheMainJS)` in generated Workers. It appears that upstream rollup-plugin-off-main-thread never handled this correctly, but we had a customization in Squoosh loader template that allowed those anyway, which I removed together with most other customizations in #1007, thus breaking Emscripten codecs. This adds a hotfix back, but we need to fix this properly upstream too (TBD separately).
Few months ago, I've extracted and published universal Rust + Rayon adapter as a separate library: https://github.com/GoogleChromeLabs/wasm-bindgen-rayon. It means we no longer need to support our own custom glue code in Squoosh, but can use it directly as a dependency, which is what this PR does.
However, in the process of working on wasm-bindgen-rayon I also made a series of changes to make it work with rollup-plugin-off-main-thread. Those changes made it closer to real AMD, as well as reduced size of the generated code, made it simpler etc., but, unfortunately, turned out to be incompatible with some of the tricks we use in Squoosh - in particular, defining multiple modules inline in a single HTML page.
Previously this worked because OMT was replacing Rollup-generated anonymous
define(calls with named entry chunks, but that caused other problems when using multiple AMD modules on the same page, so I've removed that on the OMT side and made all thedefines anonymous again. To preserve current behaviour on Squoosh side, I copied the updated loader code, and exposed an internalnextDefineUrias a global variable. This way, build system can emitnextDefineUri="..."before eachdefine(call, which, in turn, will allow multiple defines on the same page again. This is a hack, but it works and preserves existing behaviour without having to extract everything into separate scripts as Rollup normally expects.