-
Notifications
You must be signed in to change notification settings - Fork 186
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
Add lightweight wasm support to rust-decimal
#650
base: master
Are you sure you want to change the base?
Conversation
rust-decimal
rust-decimal
A new optional feature 'wasm' has been introduced in the codebase. This enables `wasm-bindgen` support, making `Decimal` compatible with the `wasm_bindgen` attribute macro, and exposes `fromNumber()` and `toNumber()` methods to convert between `Decimal` and the primitive `number` type.
I am not quite sure why the actions fail exactly but it feels like an issue with the macros crate referencing the published core crate which doesn't have the wasm support yet. Unfortunately I can't really figure out more than this. In the previous PR there were talks about having to adjust the CI most likely. |
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 think the way this is implemented in Cargo is the primary problem here - it needs to be target dependent as opposed to feature dependent. Currently, the build uses the --all-features
flag which is likely where the issues stem (this is to ensure doc compilation works etc). Because WASM isn't an additive feature but moreso a target based feature we need to upgrade it as such.
For github actions, we also need to create another branch for testing WASM specifically. e.g. something like:
- name: Install rust
uses: dtolnay/rust-toolchain@stable
with:
targets: wasm32-unknown-unknown
- name: Check
run: cargo check --target wasm32-unknown-unknown
I can help put this together over the next week or two.
Cargo.toml
Outdated
@@ -36,6 +36,7 @@ rocket = { default-features = false, optional = true, version = "0.5.0-rc.3" } | |||
serde = { default-features = false, optional = true, version = "1.0" } | |||
serde_json = { default-features = false, optional = true, version = "1.0" } | |||
tokio-postgres = { default-features = false, optional = true, version = "0.7" } | |||
wasm-bindgen = { default-features = false, optional = true, version = "0.2" } |
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'm just looking through how some other libraries introduce this and I think it needs to be done as a target as opposed to a simple feature. It could technically still be a feature however it'd need to be a no-op for std/core.
e.g.
[target.'cfg(target_arch = "wasm32")'.dependencies]
wasm-bindgen = { default-features = false, version = "0.2" }
There may be a requirement to also exclude some other libraries that aren't supported on wasm of course. However that'd require some investigation (e.g. what features are supported in combination with wasm)
@paupino Were you able to set aside some time to investigate how to refactor the workflow in order to make this PR pass the pipeline? |
@ChristianIvicevic I did start looking at it in this branch, however have yet to complete it. I'll see if I can pick it up again to get it over the line - it turns out to be a bit trickier than I anticipated! |
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.
Just adding some comments as to where I got to today and what's next. I refactored the workflow a bit so that we can introduce the wasm32 target easily - however of course the tests need to adhere to the CI_DECIMAL_TEST_TARGET
being passed through.
- name: stable | ||
rust: stable | ||
- name: beta | ||
rust: beta | ||
- name: stable / wasm | ||
rust: stable | ||
target: wasm32-unknown-unknown |
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.
TODO: We need to make the tests adhere to this.
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.
(So the pass in the CI is not accurate)
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.
After looking a bit further into this, I think this is the wrong approach. I believe a wasm target needs to be handled completely separately.
[target.'cfg(target_arch = "wasm32")'.dependencies] | ||
wasm-bindgen = { default-features = false, version = "0.2" } |
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.
It's a good idea to declare these things as optional even with the cfg guard, including wasm-bindgen
/js-sys
. Chrono found this out and reverted it: chronotope/chrono#1164. There's some nasty circular dep situations that can occur otherwise.
In their case it may have been due to it being a default-on feature; I admit I havent checkout out this branch and run cargo tree
, but it's worth investigating for a core-ish datatype crate such as this.
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.
Good call out and thank you for raising this @robjtede
Admittedly, I'm still getting up to speed on this so it's good to find out what not to do also!
A new optional feature 'wasm' has been introduced in the codebase. This enables
wasm-bindgen
support, makingDecimal
compatible with thewasm_bindgen
attribute macro, and exposesfromNumber()
andtoNumber()
methods to convert betweenDecimal
and the primitivenumber
type.Fixes #613