Make an optional js feature #86
Conversation
| @@ -16,6 +16,7 @@ repository = "https://github.com/dylanhart/ulid-rs" | |||
| default = ["std"] | |||
| std = ["rand"] | |||
| postgres = ["dep:postgres-types", "dep:bytes"] | |||
| js = ["getrandom/js"] | |||
There was a problem hiding this comment.
Just added a line to the README. Please have a look, thanks!
| @@ -16,6 +16,7 @@ repository = "https://github.com/dylanhart/ulid-rs" | |||
| default = ["std"] | |||
| std = ["rand"] | |||
| postgres = ["dep:postgres-types", "dep:bytes"] | |||
| js = ["getrandom/js"] | |||
There was a problem hiding this comment.
Should this be a default feature? It seems odd to have a feature that needs to be enabled when compiling for JS targets. Does this break the library when turned off?
The existing WASM target was contributed and I'm not really sure on the best practices here.
There was a problem hiding this comment.
I don't think it should be a default feature, because for example tower-request-id currently uses ulid, and for any code that uses towers-request-id, it is impossible to turn off getrandom/js feature due to the additive nature of cargo features.
If on the other hand, this is not default, for those that uses tower-request-id and also wants getrandom/js, they can easily add getrandom to their Cargo.toml with the js feature enabled.
There was a problem hiding this comment.
Actually I think it is best to remove dependency on getrandom. This crate in no way imports the getrandom crate, and I suspect the only reason getrandom/js feature was enabled was due to the need to run wasm-pack in CI. But this is can be easily fixed. I'll submit another PR for you to have a look.
There was a problem hiding this comment.
Another workaround is to move getrandom to a dev dependency #88 , which has the minimum change.
When the target is wasm32, the current configuration would by default enable the "js" feature of getrandom.
But this won't work well with projects that cannot enable the "js" feature (e.g., those use the "custom" feature).
Because cargo features are additive, it is best to avoid setting the "js" feature as default, but instead make it optional.