build: Add js Cargo feature flag for WASM builds#118
build: Add js Cargo feature flag for WASM builds#118lpahlavi wants to merge 10 commits intoanza-xyz:masterfrom
js Cargo feature flag for WASM builds#118Conversation
Previously, the wasm32 target implicitly assumed a browser environment, which caused issues when building for non-browser WASM environments due to the unconditional inclusion of `wasm-bindgen`. This commit introduces an explicit 'js' feature flag, making `wasm-bindgen` and `js-sys` conditional dependencies. This allows greater flexibility for different WASM execution environments. Related to anza-xyz#117
|
@joncinque requesting your review on this since you just did the feature powerset change. I don't have a strong opinion, if you're okay w/ it I will sign-off on it for the |
joncinque
left a comment
There was a problem hiding this comment.
Thanks for your contribution! I'm in agreement for this change, makes total sense to me.
The main issue: I believe this is a breaking change for people currently using the packages in wasm builds, since they're expecting all of the JS stuff to come with it. That's unfortunately going to complicate / slow down the release process a bit.
Also, in all the Cargo.tomls, we should keep gating the js-sys and wasm-bindgen crates on:
[target.'cfg(target_family = "wasm")'.dependencies]
As another note, I looked around at different feature names used for enabling JS for wasm builds, and here are some popular crates:
getrandomuseswasm_jstimeuseswasm-bindgenas the feature namechronouseswasmbinduuidandjiffusejs
So your choice of js is good, but I could also be convinced to do something like wasm-js if you liked that more.
| wasm-bindgen = { workspace = true, optional = true } | ||
|
|
||
| [dev-dependencies] | ||
| solana-hash = { path = ".", features = ["dev-context-only-utils"] } |
There was a problem hiding this comment.
I can't comment lower down, but can you remove js-sys and wasm-bindgen from the wasm32 dependencies on lines 38-40?
There was a problem hiding this comment.
As per your suggestion, I've moved all the dependencies for the new js feature back to a section gated by the target_arch = "wasm32".
| @@ -19,7 +19,7 @@ use { | |||
| }, | |||
There was a problem hiding this comment.
I can't comment further up on line 6, but does a non-JS wasm build always need std? We might be able to change the std usage to:
#[cfg(feature = "std")]
Maybe more specifically -- does a wasm build like yours need std by default? And if we simplify that feature, then we shoudl also remove the target_arch bit on line 12.
There was a problem hiding this comment.
Good question! In our particular case, we actually do always use std for non-JS builds, but I think what makes most sense here is probably to change most usages of target_arch = "wasm32" to feature = "js". I tried to have a look in each affected file where the std dependencies where needed and I tried to make it so that when the std imports were only needed in code gated by the js feature, I changed target_arch = "wasm32" to feature = "js", and otherwise I removed the gate on the target architecture. WDYT?
| @@ -31,7 +31,7 @@ use { | |||
| num_traits::{FromPrimitive, ToPrimitive}, | |||
There was a problem hiding this comment.
Same bit at the top, lines 7 and 15, changing to
#[cfg(feature = "std")]
| #[cfg_attr(feature = "js", wasm_bindgen)] | ||
| #[repr(transparent)] | ||
| #[cfg_attr(feature = "frozen-abi", derive(solana_frozen_abi_macro::AbiExample))] | ||
| #[cfg_attr( |
There was a problem hiding this comment.
I can't comment there, but there's a TryFrom<Vec<u8>> for Pubkey impl around line 429 and some gating around new_unique() (lines 483), and we should change the gating to just:
#[cfg(feature = "std")]
| std::string::ToString::to_string(&display).into() | ||
| } | ||
|
|
||
| #[allow(non_snake_case)] |
There was a problem hiding this comment.
We'll still need the allow(non_snake_case) directive
There was a problem hiding this comment.
Ah I mistakenly seem to have deleted #[allow(non_snake_case)] instead of #[cfg(target_arch = "wasm32")]. Thanks for noticing this! It is now fixed.
|
|
||
| #[allow(non_snake_case)] | ||
| #[cfg(feature = "js")] | ||
| #[cfg(target_arch = "wasm32")] |
There was a problem hiding this comment.
We can probably remove this since the js feature gates it
| solana_sanitize::Sanitize, | ||
| }; | ||
| #[cfg(target_arch = "wasm32")] | ||
| #[cfg(feature = "js")] |
There was a problem hiding this comment.
Assuming that we gate std just on the std feature, let's gate this properly with:
#[cfg(all(feature = "std", feature = "js"))]
For reference, I'm looking at how uuid declares something similar here: https://github.com/uuid-rs/uuid/blob/cb19a46cf17cd9840205e0d67a32ea8a18e58374/src/timestamp.rs#L308
I don't think we need the full target_arch part too, but the std + js feature gate seems to make sense
| impl_borsh_serialize!(borsh0_10); | ||
|
|
||
| #[cfg(all(target_arch = "wasm32", feature = "curve25519"))] | ||
| #[cfg(all(feature = "js", feature = "curve25519"))] |
There was a problem hiding this comment.
This one will also need the std feature
| } | ||
|
|
||
| #[allow(non_snake_case)] | ||
| #[cfg(feature = "js")] |
There was a problem hiding this comment.
This will also need the std feature
| } | ||
|
|
||
| #[cfg(target_arch = "wasm32")] | ||
| #[cfg(feature = "js")] |
There was a problem hiding this comment.
This will also need the std feature
lpahlavi
left a comment
There was a problem hiding this comment.
@joncinque Thank you so much for reviewing this PR so quickly!
I am aware that this will unfortunately be breaking... I believe due to Cargo's limitation to opt-in only features, there's not much we can do to mitigate this. I thought about adding the feature to default since the dependencies are anyways gated by the wasm32 target architecture, but this seems a little bit awkward. WDYT?
I am also wondering if it would be theoretically best to add target_arch = "wasm32" to all the places where we have feature = "js" or if we are OK with the code simply not compiling when having the js feature enabled for another target architecture than wasm32. Do you have an opinion on this?
One final question: do you have any idea how long it might take before this is released given that it involves a breaking change?
Thanks again and let me know if you have any further feedback!
| std::string::ToString::to_string(&display).into() | ||
| } | ||
|
|
||
| #[allow(non_snake_case)] |
There was a problem hiding this comment.
Ah I mistakenly seem to have deleted #[allow(non_snake_case)] instead of #[cfg(target_arch = "wasm32")]. Thanks for noticing this! It is now fixed.
|
|
||
| #[allow(non_snake_case)] | ||
| #[cfg(feature = "js")] | ||
| #[cfg(target_arch = "wasm32")] |
| wasm-bindgen = { workspace = true, optional = true } | ||
|
|
||
| [dev-dependencies] | ||
| solana-hash = { path = ".", features = ["dev-context-only-utils"] } |
There was a problem hiding this comment.
As per your suggestion, I've moved all the dependencies for the new js feature back to a section gated by the target_arch = "wasm32".
| @@ -19,7 +19,7 @@ use { | |||
| }, | |||
There was a problem hiding this comment.
Good question! In our particular case, we actually do always use std for non-JS builds, but I think what makes most sense here is probably to change most usages of target_arch = "wasm32" to feature = "js". I tried to have a look in each affected file where the std dependencies where needed and I tried to make it so that when the std imports were only needed in code gated by the js feature, I changed target_arch = "wasm32" to feature = "js", and otherwise I removed the gate on the target architecture. WDYT?
| @@ -31,7 +31,7 @@ use { | |||
| num_traits::{FromPrimitive, ToPrimitive}, | |||
| #[cfg_attr(feature = "js", wasm_bindgen)] | ||
| #[repr(transparent)] | ||
| #[cfg_attr(feature = "frozen-abi", derive(solana_frozen_abi_macro::AbiExample))] | ||
| #[cfg_attr( |
| impl_borsh_serialize!(borsh0_10); | ||
|
|
||
| #[cfg(all(target_arch = "wasm32", feature = "curve25519"))] | ||
| #[cfg(all(feature = "js", feature = "curve25519"))] |
| } | ||
|
|
||
| #[allow(non_snake_case)] | ||
| #[cfg(feature = "js")] |
| } | ||
|
|
||
| #[cfg(target_arch = "wasm32")] | ||
| #[cfg(feature = "js")] |
That would still be breaking for users of The other option, which is unfortunately a lot of work, is to use the semver trick for all these crates: https://github.com/dtolnay/semver-trick. We can have the v2 crates depend on the v3 crates with the correct feature, and then re-export everything, but to be totally honest, I'm not too keen on doing all of that.
Yes, after thinking about it a bit more, let's do what you suggested and also gate all js usage on the wasm32 check.
I'm still figuring out the exact release process, and I need to coordinate with some of our downstream users, so it might take a month before the crates are released. |
|
@joncinque Thanks a lot for your feedback! Given your feedback, I've now:
I believe this way we should minimize the number of affected users (i.e. only the ones using
Thank you for the info! That should be completely fine for us as we can rely on the forked repository in the meantime. Let me know if there's anything I can do to support on that side. Let me know if you have any other feedback concerning the code! |
|
@joncinque May I ask if there is an updated timeline on when the breaking changes should be released? Thanks! |
|
We've been discussing this a bit offline and in Discord (https://discord.com/channels/428295358100013066/476811830145318912/1369891812797452288), and I'm starting to lean towards a slightly different solution, of just removing all of the wasm-bindgen code completely from the component crates, and instead factoring it all into a new crate, as in #138 We've been sidetracked by some other things, so at the latest, the breaking changes should be released by the end of the month. |
|
Thanks a lot for the quick reply!
I see, thanks for the link! Unfortunately it seems I don't have access to the Would someone from Anza then take the lead in this case?
I see no problem, thank you for the update! Do you know if there is somewhere I can follow this more closely? |
|
I'll repost the most relevant comment:
And yes, we'll take the lead on this from here, most likely in #138 -- that'll be the PR to follow. It should end up being much simpler since we're just |
|
Fantastic, thanks a lot! Looking forward to the release! Do you have any idea if the |
Yeah, we should probably do the same thing everywhere |
|
Great, thank you! I will update the issue there to reflect this. |
|
Hi @joncinque! Do you know if there are any updates on the progress for the next release including the breaking changes? Perhaps I could get an invite to the Discord channel to follow the news there? Thanks! |
|
Sorry for the late reply, we'll start landing some breaking changes first thing next week and probably releasing some |
|
Thanks a lot for the heads up! I've been monitoring the repo a bit and see that the first breaking changes look to be soon merged. Looking forward to testing the first release candidates! |
|
@joncinque @LucasSte : I've noticed that the PR to add a WASM crate (here) doesn't quite allow removing all |
|
That's correct, after that PR lands, the idea is to just move all of the wasm logic from the individual crates and into |
Great, thanks a lot for the confirmation @joncinque! Just to clarify the timeline though, is this planned to be all part of the upcoming release? Or is this a longer term plan? |
#### Problem As outlined in anza-xyz#118, the sdk incorrectly assumes that all wasm builds are targeting a js environment, which breaks builds in non-js targets. #### Summary of changes The wasm-js code isn't strictly needed to make things work, so instead, move all of the code into different modules. This causes some copy-pasta, but keeps wasm builds lean. If people are using wasm-js, most likely they'll roll their own code anyway. As part of this, since solana-keypair now uses rand 0.8, we can upgrade the getrandom backend to 0.2.
#### Problem As outlined in anza-xyz#118, the sdk incorrectly assumes that all wasm builds are targeting a js environment, which breaks builds in non-js targets. #### Summary of changes The wasm-js code isn't strictly needed to make things work, so instead, move all of the code into different modules. This causes some copy-pasta, but keeps wasm builds lean. If people are using wasm-js, most likely they'll roll their own code anyway. As part of this, since solana-keypair now uses rand 0.8, we can upgrade the getrandom backend to 0.2.
|
Sorry for the wait! #244 will finally take care of this |
wasm-js: Move all implementations into sdk-wasm-js #### Problem As outlined in #118, the sdk incorrectly assumes that all wasm builds are targeting a js environment, which breaks builds in non-js targets. #### Summary of changes The wasm-js code isn't strictly needed to make things work, so instead, move all of the code into different modules. This causes some copy-pasta, but keeps wasm builds lean. If people are using wasm-js, most likely they'll roll their own code anyway. As part of this, since solana-keypair now uses rand 0.8, we can upgrade the getrandom backend to 0.2.
wasm-js: Move all implementations into sdk-wasm-js As outlined in anza-xyz#118, the sdk incorrectly assumes that all wasm builds are targeting a js environment, which breaks builds in non-js targets. The wasm-js code isn't strictly needed to make things work, so instead, move all of the code into different modules. This causes some copy-pasta, but keeps wasm builds lean. If people are using wasm-js, most likely they'll roll their own code anyway. As part of this, since solana-keypair now uses rand 0.8, we can upgrade the getrandom backend to 0.2.
Previously, the wasm32 target implicitly assumed a browser environment, which caused issues when building for non-browser WASM environments due to the unconditional inclusion of
wasm-bindgen.This commit introduces an explicit
jsfeature flag, makingwasm-bindgenandjs-sysconditional dependencies. This allows greater flexibility for different WASM execution environments.Related to #117