-
Notifications
You must be signed in to change notification settings - Fork 346
WASM Improvements #5838
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
base: main
Are you sure you want to change the base?
WASM Improvements #5838
Conversation
efdfb97 to
6a1c74c
Compare
|
Hm not entirely sure why it fails on the feature flag for uniffi as the workspace Cargo.toml claims 0.30.0. I know it works with 0.29.4 upwards. I am guessing it may just need a new lockfile right? |
sqlite does not compile on wasm platforms. Therefor we probably shouldn't have it as the default since indexeddb support was added. Instead enabling it on demand together with the sqlite feature seems like a better solution Signed-off-by: MTRNord <[email protected]>
wasm methods use futures-executor which was missing from the dependencies so far. Additionally as wasm isn't Send we need to tell uniffi about that when compiling for wasm. Signed-off-by: MTRNord <[email protected]>
This adds stubs for wasm Uniffi does not support cfg attributes like previously used here. If _any_ platform exports it will generate the skeleton for _all_ platforms every time. Using uniffi::method attributes also will not fix this where we have different exports based on platform. Therefor stubs sadly are the only way to handle this as of now. Signed-off-by: MTRNord <[email protected]>
Signed-off-by: MTRNord <[email protected]>
6a1c74c to
fd93a0f
Compare
|
Also sorry for the force pushing. Realised the conventional commit policy too late. From now on, I will instead use fixup commits when needed. |
complement-crypto, which is used for one of the CI runs, downgrades to I think only enabling this feature under Wasm, which makes sense regardless of complement-crypto, would be one way to solve this problem. |
CodSpeed Performance ReportMerging #5838 will not alter performanceComparing Summary
|
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #5838 +/- ##
=======================================
Coverage 88.60% 88.60%
=======================================
Files 361 361
Lines 102150 102150
Branches 102150 102150
=======================================
+ Hits 90508 90509 +1
+ Misses 7425 7423 -2
- Partials 4217 4218 +1 ☔ View full report in Codecov by Sentry. |
Well ok, I updated complement-crypto here matrix-org/complement-crypto#215. |
This brings a few issues I noticed while trying to port https://gitlab.opencode.de/fitko/matrix-g2x/fit-neo-web/-/merge_requests/3 from the filament Wasm mega PR over to the main branch of the SDK. Without these (and downgrading uniffi 0.29.4 due to jhugman/uniffi-bindgen-react-native#316 ) the bindings cannot compile.
Note that also the inline stubs seem to be required as uniffi does checksums over the actual function signature including any attributes. This seems to happen before evaluation of them, however. Which caused checksum mismatches if I used nicer looking cfg attributes (splitting Wasm and non Wasm world into 2 functions instead of inline blocks). Ideally, one day uniffi gets smarter about that, but it doesn't seem to be a thing yet. https://mozilla.github.io/uniffi-rs/latest/proc_macro/index.html#conditional-compilation seems to also not help us here.
One more thing I noticed is that while the indexeddb support was added, the
bundled-sqlitefeature still stayed as a default feature. This causes issues on Wasm as SQLite can't be compiled on that platform using thesqlite-syspackage which rust-sdk utilizes. (It will complain about missing headers). Therefore, I think it's easiest to just remove it from the defaults and have it active conditionally by the consumer.Finally, WASM threading support currently lacks Send, so uniffi has to be told to be less strict about their requirements to support this. This might change in the future, however, if Wasm gets proper threading support in rust land. Additionally, there was some code using futures-executor, but the dependency was not added to the wasm platform.
(This PR is made with my Nordeck 🎩 for the FITKO Matrix-G2X project over at https://gitlab.opencode.de/fitko/matrix-g2x/gitlab-profile )