Add getrandom crate feature=js for KeyExpr #1720
Conversation
|
PR missing one of the required labels: {'internal', 'bug', 'enhancement', 'documentation', 'breaking-change', 'dependencies', 'new feature'} |
commons/zenoh-keyexpr/Cargo.toml
Outdated
| @@ -36,6 +36,7 @@ schemars = { workspace = true, optional = true } | |||
| serde = { workspace = true, features = ["alloc"] } | |||
| token-cell = { workspace = true } | |||
| zenoh-result = { workspace = true } | |||
| getrandom = { workspace = true, features = ["js"] } | |||
There was a problem hiding this comment.
Wouldn't be better having a js feature in zenoh-keyexpr that optionally enables js on getrandom instead of always enabling it?
There was a problem hiding this comment.
I agree with adding a feature,
It was mentioned here https://docs.rs/getrandom/latest/getrandom/#webassembly-support
That libraries should not introduce their own js features just to enable getrandom’s js feature.
regardless i think its cleaner if we do add it.
| @@ -102,6 +102,7 @@ form_urlencoded = "1.2.1" | |||
| futures = "0.3.30" | |||
| futures-util = { version = "0.3.30", default-features = false } # Default features are disabled due to some crates' requirements | |||
| git-version = "0.3.9" | |||
| getrandom = { version = "0.2" } | |||
There was a problem hiding this comment.
getrandom is added as dependency but no change in this PR seems to use it.
What's the role of this dependency alone, which makes fail the CI?
There was a problem hiding this comment.
rand uses getrandom under the hood, and get random has the library feature that we need to enable to get the keyexpr package to compile to wasm successfully.
Ive added an ignore for cargo machete which should fix the CI check
Used to enable compiling key expressions to WASM for zenoh-ts