Publish a dual CJS/ESM package with platform-specific loaders #167
Conversation
|
As discussed: unfortunately, this is going to break the Jest tests in matrix-js-sdk. We looked into trying to get Jest running on ES modules, but ultimately gave up. |
|
I switched gears a little bit, and instead opted for making the package have 4 entrypoints:
One annoying thing, is that the matrix-js-sdk expects the WASM to be already loaded and available, and doesn't call To achieve that, I used the |
…tween deploys (#2823) * Add explicit code split on matrix-sdk-crypto-wasm to allow caching between deploys * Comment on removing once matrix-org/matrix-rust-sdk-crypto-wasm#167 lands
index.js
Outdated
| export * from "./pkg/matrix_sdk_crypto_wasm_bg.js"; | ||
| import * as wasm from "./pkg/matrix_sdk_crypto_wasm_bg.js"; | ||
|
|
||
| const moduleUrl = new URL("./pkg/matrix_sdk_crypto_wasm_bg.wasm", import.meta.url); |
There was a problem hiding this comment.
for reasons I don't entirely understand, webpack is putting the wasm in the top-level build directory, rather than in a bundle directory. That's probably a problem in element-web's webpack config, but it would be good to fix before we ship this.
There was a problem hiding this comment.
I also noticed that, no idea why. Element Call (so, Vite) properly handled this as regular asset.
|
@richvdh I've reverted the move to vitest, now that it works correctly again in CJS environments 💃 For consumers, the bindings don't synchronously initialise themselves automatically anymore. If they need that behaviour, they need to import |
sandhose
changed the title
|
@richvdh I've added typechecking of JS files to avoid some of the dumb errors you've spotted previously |
There was a problem hiding this comment.
This looks like a sensible direction to me now. Some general points:
- I've pulled out the test file changes into a separate PR (Update tests to use the public entry point to the module #171). Could you rebase on top of that?
- There's a lot of duplicate code between the entry points. I'd like to see if we can factor a lot of that out to one or two shared modules (possibly: one module which is shared across all four entry points and one which is shared between the two
nodeentry points.) AIUI a commonjs module can be easilyimported into an ESM module? - I'd like to see comments on the various function definitions describing what they do, which global variables they touch, what they return, etc.
Co-authored-by: Richard van der Hoff <[email protected]>
sandhose
force-pushed
the
quenting/proper-async-loading
branch
from
e5c1094 to
accdcf7
Compare
yarn keeps adding this, as I'm using corepack locally…
| * @param {WebAssembly.Module} mod | ||
| * @returns {typeof import("./pkg/matrix_sdk_crypto_wasm_bg.wasm.d")} | ||
| */ | ||
| function initInstance(mod) { |
There was a problem hiding this comment.
| function initInstance(mod) { | |
| function initInstance(mod) { | |
| if (initialised) { | |
| // This should be unreachable | |
| throw new Error("initInstance called twice"); | |
| } | |
There was a problem hiding this comment.
This actually throws if you call initAsync, it loads in the background, and then init it synchronously before it finishes; but I think it is fine to throw because the consumer shouldn't do that
There was a problem hiding this comment.
This actually throws if you call initAsync, it loads in the background, and then init it synchronously before it finishes;
Will it? In that case, wouldn't modPromise be set, so that the synchronous init will fail early with an exception?
This fixes a problem (introduced in #167) on Chrome, which would complain about the module being too big. It also significantly simplifies the code on the browser entry points, because instantiateStreaming is widely supported.
As of matrix-org/matrix-rust-sdk-crypto-wasm#167, the wasm build of matrix-sdk-crypto is actually shipped as a `.wasm` file, rather than base64-ed into Javascript. Our current webpack config then dumps it into the top level of the build directory, which will be a problem on redeployment (specifically, if you try to fetch the wasm artifact for vN after vN+1 has been deployed, you'll get a 404 and sadness). So, instead we use Webpack's experimental support for WASM-as-ES-module, which makes Webpack put the wasm file in the bundle, along with everything else. Fixes: #28632
As of matrix-org/matrix-rust-sdk-crypto-wasm#167, the wasm build of matrix-sdk-crypto is actually shipped as a `.wasm` file, rather than base64-ed into Javascript. Our current webpack config then dumps it into the top level of the build directory, which will be a problem on redeployment (specifically, if you try to fetch the wasm artifact for vN after vN+1 has been deployed, you'll get a 404 and sadness). So, instead we use Webpack's experimental support for WASM-as-ES-module, which makes Webpack put the wasm file in the bundle, along with everything else. Fixes: #28632
This package currently ships the WASM module as a JS string, base64-encoded.
This is very inefficient for three reasons:
For it to be handled correctly by bundlers, it is better to provide an ESM-compatible package. Because some environments (like the JS SDK test suite) still run on CJS, I've made it a proper dual CJS/ESM package.
The loading logic is also platform-specific: on node-like environments, it needs to read the file from the disk, whereas on browser-like environments it needs to
fetchthe WASM file. This is why this now provides platform-specific entrypoints.To give an idea of what performance impact this change has, here is a before/after comparison on Element Call:
Notice how:
Same before/after on Element Web: