Skip to content
This repository has been archived by the owner on Oct 31, 2024. It is now read-only.

Don't set the js feature for getrandom #40

Merged
merged 4 commits into from
Mar 28, 2024
Merged

Don't set the js feature for getrandom #40

merged 4 commits into from
Mar 28, 2024

Conversation

dvdplm
Copy link
Contributor

@dvdplm dvdplm commented Mar 26, 2024

Libraries should avoid setting features that may or may not be required when used.

The getrandom docs put it better: "This feature should only be enabled for binary, test, or benchmark crates. Library crates should generally not enable this feature, leaving such a decision to users of their library. Also, libraries should not introduce their own js features just to enable getrandom’s js feature."

chore: update criterion to latest
@dvdplm dvdplm requested a review from a team as a code owner March 26, 2024 09:48
@dvdplm dvdplm requested review from hratoanina and Nashtare March 26, 2024 09:48
Copy link
Member

@Nashtare Nashtare left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is a required feature to enable no-std compilation.

@dvdplm
Copy link
Contributor Author

dvdplm commented Mar 26, 2024

This is a required feature to enable no-std compilation.

Nah. Being a library, the decision about being no-std or not is up to the code that uses it. As explained in the docs, an application compiled to wasm (as an example of a no-std use case), can add the feature.

In this specific case, the js feature assumes a Javascript context, which does not encompass all no-std cases.

@Nashtare
Copy link
Member

The CI error for targeting wasm-32-unknown-unknown seems weirdly specific to this feature though. It's the same we encountered years ago when porting librustzcash to no-std for substrate.

@dvdplm
Copy link
Contributor Author

dvdplm commented Mar 27, 2024

seems weirdly specific to this feature though

You are correct (as always). I read through rust-lang/cargo#1197 and found this workaround. Do you like it better?

Copy link
Member

@Nashtare Nashtare left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah it's cleaner that way, thanks!

@dvdplm dvdplm merged commit 562b64e into main Mar 28, 2024
7 checks passed
@dvdplm dvdplm deleted the dp-dont-impose-js branch March 28, 2024 10:10
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants