-
Notifications
You must be signed in to change notification settings - Fork 270
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
feat(crypto-js): Encode the WASM as base64 for portability #1167
Conversation
"pack": "wasm-pack pack", | ||
"prepublish": "npm run pack", | ||
"publish": "wasm-pack publish" |
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've removed these for now as I don't think they'll do the right thing, and they seemed to be a footgun.
Some nasty hackery to get around the nastiness of the JS ecosystem
5dfece8
to
0938566
Compare
hold off on review for a minute, have realised a thinko |
Use a lookup table instead of a function with if statements
Codecov ReportBase: 73.07% // Head: 73.62% // Increases project coverage by
Additional details and impacted files@@ Coverage Diff @@
## main #1167 +/- ##
==========================================
+ Coverage 73.07% 73.62% +0.55%
==========================================
Files 110 110
Lines 12304 12384 +80
==========================================
+ Hits 8991 9118 +127
+ Misses 3313 3266 -47
Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here. ☔ View full report at Codecov. |
@Hywan these tests pass locally, and I don't really understand what they are complaining about. Any ideas? |
This patch also improves the “phrasing”, simplifies the code a little bit etc. Small stuff.
RUSTFLAGS='-C opt-level=z' WASM_BINDGEN_WEAKREF=1 wasm-pack build --release --target nodejs --scope matrix-org --out-dir pkg | ||
|
||
# Convert the Wasm into a JS file that exports the base64'ed Wasm. | ||
echo "module.exports = \`$(base64 pkg/matrix_sdk_crypto_js_bg.wasm)\`;" > pkg/matrix_sdk_crypto_js_bg.wasm.js |
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.
@Hywan I don't love that this means that we'll slurp the whole 4.3MB of base64ed-WASM into bash and pass it as an argument to echo
. Did my previous solution of writing it straight to the output file not work for you?
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.
Oh sorry, I was just trying to debug something. Your previous solution was good too!
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.
LGTM, finally! :-)
# 2. Remove the imports of `TextDecoder` and `TextEncoder`. We rely on the global defaults. | ||
loadwasm='const bytes = require("./unbase64.js")(require("./matrix_sdk_crypto_js_bg.wasm.js"));' | ||
|
||
# sed on OSX uses different syntax for sed -i, so let's just avoid 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.
👍
Mostly because, as of #1167, we no longer have a `publish` script.
Now that we use the Rust crypto stack (matrix-org/matrix-react-sdk#12630), the legacy olm library is unneeded. Remove all the dedicated cruft for loading it. (All this crap is why matrix-sdk-crypto-wasm has to base64 its wasm artifact, cf matrix-org/matrix-rust-sdk#1167.)
I'm sorry about the filthiness of this hack.
It seems like the only way to get the WASM loaded correctly everywhere is to base64 it into a regular JS module. So, let's do that...
The
--target nodejs
seems to be closest to what we want; I also tried--target bundler
, but orchestrating the different modules was rather more complicated.