Skip to content
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 description about Cargo js feature for WebAssembly section #280

Merged
merged 6 commits into from
Aug 30, 2022

Conversation

Kazhuu
Copy link
Contributor

@Kazhuu Kazhuu commented Aug 20, 2022

Documentation improvement regarding #267.

@newpavlov
Copy link
Member

You should add a clarification that the js feature SHOULD be enabled only in "application" crates. For library tests and benchmarks the feature should be enabled only for tests and benchmarks, not for the whole library.

@Kazhuu
Copy link
Contributor Author

Kazhuu commented Aug 26, 2022

Okay I updated the doc specifically stating what you mentioned. I'm quite new to Rust in general so had to do some digging with this. Is it possible to enable specific feature when the crate type is binary for example? If yes, then example of this could be added to the docs.

For one of my own projects I've enabled js feature like this:

[dependencies.getrandom]
features = ["js"]

Signed-off-by: Joe Richey <[email protected]>
This is now redundent, now that we've claried documentation in the
WebAssembly support section

Signed-off-by: Joe Richey <[email protected]>
@josephlr
Copy link
Member

@Kazhuu I've made some updates to the wording here. Hopefully this will help emphasize that this feature should generally be enabled on non-library crates (binary, benchmark, or test crates). I also removed the "Indirect Dependencies" Section as its now redundant.

I'll give @newpavlov a chance to review this. Note that the CI failure is caused by an unrelated nightly bug, see:

@josephlr josephlr requested review from josephlr and newpavlov August 29, 2022 00:31
@Kazhuu
Copy link
Contributor Author

Kazhuu commented Aug 29, 2022

Thanks @josephlr! Now the wording is a lot better and conveys more clearly what is needed. Even the original text I wrote is not there anymore I learned from this experience 👍

src/lib.rs Outdated Show resolved Hide resolved
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants