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.

Sign up for GitHub

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

Make an optional js feature #86

Closed
wants to merge 3 commits into from
Closed

Make an optional js feature #86

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
5a72985
Make an optional js feature
ninegua Jan 2, 2025
1adf07e
Add --features js to CI test
ninegua Jan 2, 2025
d986bcd
Add `js` feature to README
ninegua Jan 7, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion .github/workflows/ci-build.yml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -34,4 +34,4 @@ jobs:
https://github.com/rustwasm/wasm-pack/releases/download/v${WASM_PACK_VERSION}/${file_name}.tar.gz \
| tar --strip-components=1 -xzvf- "${file_name}/wasm-pack"

- run: wasm-pack test --node
- run: wasm-pack test --node --features js
3 changes: 2 additions & 1 deletion Cargo.toml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -16,6 +16,7 @@ repository = "https://github.com/dylanhart/ulid-rs"
default = ["std"]
std = ["rand"]
postgres = ["dep:postgres-types", "dep:bytes"]
js = ["getrandom/js"]
Copy link
Owner

Choose a reason for hiding this comment

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

Needs documentation in README

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Just added a line to the README. Please have a look, thanks!

Copy link
Owner

Choose a reason for hiding this comment

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

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

PR is here #87

I'll close this one if you think #87 is more appropriate. Thanks!

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Another workaround is to move getrandom to a dev dependency #88 , which has the minimum change.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I'm closing this and #87 in favor of #88. Please feel free to re-open if you think otherwise.


[dependencies]
serde = { version = "1.0", optional = true }
Expand All @@ -25,7 +26,7 @@ postgres-types = { version = "0.2.6", optional = true }
bytes = { version = "1.4.0", optional = true }

[target.wasm32-unknown-unknown.dependencies]
getrandom = { version = "0.2", features = ["js"] }
getrandom = "0.2"
web-time = "1"

[dev-dependencies]
Expand Down
1 change: 1 addition & 0 deletions README.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -31,6 +31,7 @@ assert_eq!(ulid, res.unwrap());
* **`std` (default)**: Flag to toggle use of `std` and `rand`. Disable this flag for `#[no_std]` support.
* **`serde`**: Enables serialization and deserialization of `Ulid` types via `serde`. ULIDs are serialized using their canonical 26-character representation as defined in the ULID standard. An optional `ulid_as_u128` module is provided, which enables serialization through an `Ulid`'s inner `u128` primitive type. See the [documentation][serde_mod] and [serde docs][serde_docs] for more information.
* **`uuid`**: Implements infallible conversions between ULIDs and UUIDs from the [`uuid`][uuid] crate via the [`std::convert::From`][trait_from] trait.
* **`js`**: Flag that turns on the `getrandom/js` feature, which is only used for wasm32 targets.

[serde_mod]: https://docs.rs/ulid/latest/ulid/serde/index.html
[serde_docs]: https://serde.rs/field-attrs.html#with
Expand Down