-
Notifications
You must be signed in to change notification settings - Fork 241
Remove all uses of getrandom_backend = "wasm_js"
#771
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: master
Are you sure you want to change the base?
Conversation
dhardy
left a comment
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.
Removal of getrandom_backend="wasm_js" is a breaking change; it makes sense to merge this before 0.4 (I assume this is your intention).
| - { | ||
| description: Web, | ||
| version: stable, | ||
| flags: '-Dwarnings --cfg getrandom_backend="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.
We still want -Dwarnings (unless substituted below)
Cargo.toml
Outdated
| # i.e. avoid unconditionally enabling it in library crates. | ||
| # | ||
| # WARNING: This feature SHOULD be enabled ONLY for binary crates and tests. In other words, | ||
| # avoid enabling it in library crates whether unconditionally or gated on a crate feature. |
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.
or gated on a crate feature
I see no hard reason not to have a "forwarding" feature flag (e.g. wasm_js = ["getrandom/wasm_js"]).
I know you don't like this pattern, but I don't think directing users like this will work particularly well.
I suggest we keep our advice direct and minimally opinionated:
We recommend against enabling this feature in libraries (except for tests) since it is known to break some non-Web WASM builds and further since the usage of
wasm-bindgencauses significant bloat toCargo.lock(on all targets).
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 having a default guidance against forwarding crate features is a reasonable. It's mostly that you run into semver and stability issues when trying to forward the features of other crates. It's possible to do, but really only works if:
- you pay extra close attention to the issues
- you work closely between the forwarding crate and
getrandom(likeranddoes).
josephlr
left a comment
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.
Overall this seems reasonable if it's part of a breaking change for 0.4 (and the changes to tests/documentation might be good to also have in 0.3).
It might be worth mentioning (somewhere) that we used to allow --cfg getrandom_backend="wasm_js" (but don't anymore for 0.4)
Enabling the
wasm_jscrate feature is sufficient.