Make the documentation for WASM targets clearer

[najamelan]

For people who run in a compile error because wasm-bindgen and stdweb features are not enabled, clarify that they can enable them even if getrandom gets pulled in by one of their dependencies.

I think this isn't necessarily obvious. I didn't realize that I could set features for indirect dependencies.

What I do wonder about though is if this is the desired workflow, why does the rand crate forward this feature? And should a user specify getrandom with say wasm-bindgen in their Cargo.toml, or should they set the feature on rand.

Note also that library authors might have to set this at least in dev-dependencies to make tests and examples run.

[dhardy]

Thanks for the PR — sounds like a sensible thing to do. Perhaps add a line like the following:

getrandom = { version = "0.1.9", features = ["dummy"] }

[newpavlov]

I don't think there is the feature to be enabled. Also please take a look at #87. I am not sure if enabling dummy, stdweb or wasm-bindgen feature in an non-application Cargo.toml is a correct recommendation.

[newpavlov]

So I think the problem will mainly arise in dev-builds, e.g. because benchmarks need system entropy, but not tests and CI job runs tests for wasm32-unknown-unknown. I think a better recommendation would be to add the following line to Cargo.toml:

[dev-dependencies]
getrandom = "0.1"

And enable a needed getrandom feature manually in CI job, e.g. by using cargo test --features getrandom/dummy or cargo test --features getrandom/wasm-bindgen.

[najamelan]

@dhardy do you mean add an example line to the docs? I could add that, but I haven't yet really understood the usefulness of the "dummy" feature though. If it's being pulled in, it's because people want it to work, I suppose most users don't want to just turn compile time failure into runtime failure?

Also, I have said it on the futures-rs issue, but one thing that worries me a bit is that when libraries enable this feature, the end use has to change their Cargo.toml, which means it's a breaking change. Currently futures-rs pulls this in with a feature on futures. So if an intermediate library suddenly decides to use that functionality from futures (join or select macro), that might break end user application. Meaning that features like using a join macro now becomes a breaking change, which isn't documented anywhere.

This feature also isn't exactly additive. The docs say if both features are enabled wasm-bindgen is chosen. I suppose that is to make them somewhat additive. If libraries should be transparent to this and only application devs should set it, why would they ever set both though? It would also mean that somewhere along the dependency line a library had enabled stdweb, which as I understand it they shouldn't have, but also means that they are probably counting on it where as the wasm-bindgen feature being enabled as well will silently turn off the functionality of the stdweb feature.

So I think the problem will mainly arise in dev-builds, e.g. because benchmarks need system entropy

@newpavlov I don't know if it's correctly used, but in the futures library, if you turn on the feature that enables the join and select macros, rand gets pulled which pulls getrandom. They use it in order to randomly select which future to poll first. That's not exactly a dev-build only problem.

[dhardy]

From Cargo's point of view, all feature flags are additive. For the getrandom crate, only a single API is needed to provide its implementation, and when multiple are available it will choose the best of those; from the user's point of view this does not matter at all.

Yes, true, adding a dependency on getrandom or rand may now cause build failures on wasm-unknown-unknown. I think we just have to accept that. Eventually this target should be deprecated in favour of wasm-wasi, where we don't have this problem.

Alternatively maybe we should just assume wasm-bindgen for this target now (see #63).

[najamelan]

after the discussion in #63 I will try to rephrase the doc PR to be clearer. Does the following seem right (to come after the paragraph about wasm32-unkown-unknown?

Library authors should not set these features. Application developers should set one of wasm-bindgen, stdweb or dummy even when getrandom gets pulled in by a dependency.

Note For library authors, this means that if you start pulling in getrandom (which happens if you depend on rand with default features), it will be a breaking change on wasm32 and you should notify your users in documentation that setting the above mentioned features is necessary.

[newpavlov]

I am not sure if need the note about breaking change. How about something like this?

Library authors should not set these features in their Cargo.toml outside of dev builds. Meaning, if you test wasm32-unknown-unknown target in your CI, you can add the following line to your Cargo.toml:

[dev-dependencies]
getrandom = { version = "0.1", features = ["wasm-bindgen"] }

Note: features enabled for dev builds leak to non-dev builds (see rust-lang/cargo#1796), but only for the current workspace or path dependencies, meaning you can publish crate with such dependency to crates.io without enabling the feature for all your downstream dependencies.

Alternatively you can add getrandom to your (dev-)dependencies without enabling any features:

[dependencies]
getrandom = "0.1"

And enable one of the features manually:

cargo build --features=getrandom/dummy

You may cascade the features by adding features like stdweb = ["getrandom/stdweb"] to your crate, but generally we would advice against doing so.

[newpavlov]

Closed in favor of #90.