-
Notifications
You must be signed in to change notification settings - Fork 1.3k
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
fix!: wasm compatibility for fuels-types
#839
Conversation
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! Thanks for sorting this out!
…magnet/wasm_friendly_abigen
`fuel-core` feature was created by mentioning `fuel-core` inside the `fuel-core-lib` feature without using `dep:`. This has a consequence that if somebody should enable `fuel-core` as a feature, thinking that would get them the fuel-core-lib, it would only do half the job because the real feature for that is `fuel-core-lib`.
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!
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.
👍
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.
Aaaah, very nice work. This makes a lot of sense.
closes: #837
closes: #854
A big thank you to @hal3e for help with the CI.
The wasm offending functionality is now hidden behind the feature
std
infuels-types
and infuels-core
as well.Breakage:
@digorithm
It is part of the crate's default features, so most users shouldn't feel the change -- especially since there weren't other other features that would have made somebody do
default-features = false
in theirCargo.toml
.But if they did for whatever reason, then they might see parts of
fuels-types
disappear since thestd
feature would not be enabled. So that makes it a theoretically breaking change.Also
fuels
had two feature flags:fuel-core
andfuel-core-lib
.fuel-core
is removed since it was implicitly added because the optional dependencyfuel-core
had not been referred to by prependingdep:
to it.fuel-core
used to enable the fuel-core-lib only partially and was not the right flag to use.About the implementation
Cargo features should be additive by design. This means that you cannot remove dependencies by adding features.
This further means that wasm support is generally implemented by hiding wasm-offending code behind a feature (such as
std
).In our case, if we want to be WASM compatible, we must not enable the
std
feature on anyfuels-*
crates our project might depend on.Feature unification makes this a bit difficult. If there is even one dependency that enabled
std
on somefuels-*
crate, all other crates that are part of that compilation will also have thestd
feature enabled for that crate.Because of this, I've made it default for our workspace dependencies to start off without defaults (and thus without the
std
flag).If any of our crates need it, they will have to enable the feature explicitly.