-
Notifications
You must be signed in to change notification settings - Fork 191
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
Uuid::new_v4() compiles and then panics for wasm32-unknown-unknown target #350
Comments
@Dylan-DPC thanks, just tried ... Browser
Node
Testing with:
|
Not sure if this comment is related to my issue somehow:
Will check tomorrow if it's gated behind this feature. |
@Dylan-DPC it's not enough to update to 0.6. One has to use
Just checked the
One can use |
Hmm, this doesn't seem ideal to me... We'd either have to pick one of the We don't currently claim any @zrzka Do you have any thoughts? |
@KodrAus there's one way.
My crate Cargo.toml:
This works for me. And anyone can still use I also tried to add ...
... to enable these features for Maybe @alexcrichton can sched a light on this, I mean, what's the recommended approach. IIRC I saw Alex in all these Another option, just for me, is to disable |
It is a good idea to add wasm support for UUID. @zrzka since you have already started digging into this, are you open to sending a PR that adds the feature? we will mentor you. If you don't have time, I'll look into adding it later. Thanks |
@Dylan-DPC yup, going to do it now. |
Did the PR, but not sure if it's the right thing to do. It works for me and I have a temporary fix for my issue. No need to hurry. What bothers me is this |
For now we don't have a great way to solve this, but the recommended approach would be to have an feature name like |
@alexcrichton Hm, this isn't going to play particularly nicely with dependencies that are optional to begin with though, right? So with a But if this is the way we're recommending folks do this then we should follow it here, so at least |
@KodrAus there's another, hypothetical, but still, issue with this approach. Imagine that
... and here's the hypothetical issue. What if An example in opposite direction. In our crate, we're depending on |
@zrzka That's a good spot. Though having feature names that are named differently from their purpose might mislead some users. We don't need wasm-bindgen, so adding a dependency just for wasm-support feels like overkill. The current approach seems fine to me and we have no plans for npm support right now, so we can go the original way or you can rename them to something that relates to them. |
@Dylan-DPC I agree, that's the reason why I wrote that this is a hypothetical example, because there're lot of UUID NPM packages and there's no need for another one :) It's just something to be aware of. |
Yeah thanks. Also on the PR, you should get a "Restore branch" option and I can reopen the PR after that. |
@KodrAus yeah it's not a great situation unfortunately :(. It's currently a local optimum we've figured out, but if there's better ideas I'm all ears! |
351: Feature/wasm r=Dylan-DPC a=zrzka # Description This PR adds two new features: * `stdweb` * `wasm-bindgen` These features are kind of _passthrough_ features, because they do nothing in the `uuid` crate itself. They're just passed to the `rand` crate to make the `uuid` crate working for the `wasm32-unknown-unknown` target. # Motivation I'm unable to generate random UUID (v4) when this crate is compiled for the `wasm32-unknown-unknown` target. # Tests I just added these features ... ``` - cargo test --features "v3" - cargo test --features "v3 stdweb" - cargo test --features "v3 wasm-bindgen" ``` ... for all `v3` & `v4` & `v5` (`rand` crate is used in all these features) to the `script` section in the `.travis.yml`. Not sure if it makes sense, but it can demonstrate that it's buildable at least. I don't think that more tests are required unless you'd like to bring the whole `wasm-bindgen`, `wasm-pack`, ... machinery here. And it has no sense to do it, because goal of this PR is not to publish `uuid-rs` NPM package, just add the ability to compile & use it from the `wasm32-unknown-unknown` target. # Related Issue(s) * #350 * [now() doesn't work](balena-io-modules/balena-temen#37) # Manual tests My Cargo.toml: ``` [dependencies] uuid = { features = ["v4"], git = "https://github.com/zrzka/uuid.git", branch = "feature/wasm" } [target.wasm32-unknown-unknown.dependencies] uuid = { features = ["wasm-bindgen"], git = "https://github.com/zrzka/uuid.git", branch = "feature/wasm" } ``` My `index.js`: ``` const bt = require('balena-temen'); console.log( bt.evaluate({ "id": { "$$eval": "uuidv4()" } }) ); ``` [uuidv4() implementation](https://github.com/balena-io-modules/balena-temen/blob/master/src/builtin/function/uuidv4.rs#L9-L11). Output: ``` { id: 'cefa2919-ff48-48ef-a231-e13697e23ed2' } ``` Co-authored-by: Robert Vojta <[email protected]>
@alexcrichton Yeh, I guess something along the same lines could involve some fancier way to link up enabled cargo features on-the-fly: [dependencies.rand]
optional = true
[dependencies.wasm-bindgen]
optional = true
[when.'cfg(all(feature = "wasm-bindgen", feature = "rand"))']
features = ["rand/wasm-bindgen"] Actually, that might be nice for a bunch of other cases too, but is probably not quite like using |
@KodrAus ah yes indeed! I think that's this old RFC perhaps? |
Is your feature request related to a problem? Please describe.
I do use
uuid
crate in my crate, which can be used as a pure Rust library or as an isomorphic NPM package (node & browser). The only feature I do use is random UUID (v4) generation.It panics:
Example of what we do is in this issue. Our
uuidv4()
function source code.Describe the solution you'd like
Generate UUID v4 even when compiled for the
wasm32-unknown-unknown
target.Is it blocking?
Kind of. Not a high priority.
Describe alternatives you've considered
Thinking about using
Math.random
in my function if the target iswasm32-...
.Other
Not sure if this is the
uuid
crate problem or it lies somehwere deeper inside this whole bleeding edge stuff.The text was updated successfully, but these errors were encountered: