Feature/wasm

[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)

• Uuid::new_v4() compiles and then panics for wasm32-unknown-unknown target #350
• now() doesn't work

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.

Output:

{ id: 'cefa2919-ff48-48ef-a231-e13697e23ed2' }

[Dylan-DPC-zz]

Thanks. But I don't think it is a good idea to depend on docs for messages that it is supported only on certain targets with a feature gate. I'm looking for a better way of doing this.

[zrzka]

Agreed, I don't like it too. Maybe hide new_v4() & friends ...

    #[cfg(any(
        not(target_arch = "wasm32"),
        all(
            target_arch = "wasm32",
            any(feature = "stdweb", feature = "wasm-bindgen")
        )
    ))]
    pub fn new_v4() -> Self { ... }

... , remove # Panics docstring and add some note to the lib.rs what to do with the wasm32?

[zrzka]

Did update this PR and gated all v3, v4, v5 modules behind features. If you target wasm32-... and you forget to enable either stdweb or wasm-bindgen feature, it fails to compile:

warning: unused manifest key: package.maintenance
   Compiling rand v0.6.1
   Compiling uuid v0.7.1 (https://github.com/zrzka/uuid.git?branch=feature/wasm#0ccfd12f)
   Compiling balena-temen v0.0.20 (/Users/robertvojta/Work/balena/balena-temen)
error[E0599]: no function or associated item named `new_v4` found for type `uuid::Uuid` in the current scope
  --> src/builtin/function/uuidv4.rs:10:22
   |
10 |     Ok(Value::String(Uuid::new_v4().to_hyphenated().to_string()))
   |                      ^^^^^^^^^^^^ function or associated item not found in `uuid::Uuid`

[Dylan-DPC-zz]

Currently this doesn't test the crate on wasm-* but just the features. We need to add another build to the CI so that it will run all the tests (or just the feature-gated ones) on wasm.

[zrzka]

@Dylan-DPC can you elaborate on tests pls? There're two options:

a) make a crate buildable for wasm32-... and that's it, test the features only,
b) make a crate buildable for wasm32-... and provide "NPM package" (JS wrapper) and test it with the wasm-pack test ..., something like this

If a) is the right answer then I don't understand what do you mean with tests. Because you can't do things like cargo test --target wasm32-unknown-unknown --features "v5 stdweb" for example.

Also I don't think b) needs to be added.

[zrzka]

rand is doing this for WASM, it takes quite a long time and the output is:

test result (async): ok. 0 passed; 0 failed
test result (async): ok. 0 passed; 0 failed

[zrzka]

Did add builds for the wasm32 arch with features as a minimum.

[KodrAus]

Thanks for the PR @zrzka. Personally I'm not a fan of adding transitive stdweb or wasm-bindgen features to uuid for the sake of rand. It just passes the problem of figuring out feature gates upstream to the next consumer of uuid.

Right now, I think depending on rand alongside uuid in your project with one of these features set is a better way to go.

If we're going to support random uuids in wasm then I think we should do it without needing any special feature gates.

[Dylan-DPC-zz]

@Dylan-DPC can you elaborate on tests pls? There're two options:

a) make a crate buildable for wasm32-... and that's it, test the features only,
b) make a crate buildable for wasm32-... and provide "NPM package" (JS wrapper) and test it with the wasm-pack test ..., something like this

This will test only the features, it won't test whether uuid works on the wasm architectures or not, which could lead to false truths.

[zrzka]

@KodrAus not sure if you can do it without feature gates now, because there's stdweb and wasm-bindgen, which are different things / approaches. That doesn't mean I like it, because it's weird and it looks like that it's fixing an issue which lies elsewhere.

Anyway, I'm fine with adding rand as a dependency and copy & pasting new_v4.

I'm going to close this PR, trash branch. Decide if you'd like to keep #350 open and if not, feel free to close it.

[Dylan-DPC-zz]

@KodrAus I don't think you can do it without the feature gates, and since the rand features are different depending on whether they want stdweb or bindgen. I don't see another way of doing this at this point.

[zrzka]

@Dylan-DPC is right :)

Anyway, I have fixed my issue with this commit, works for me. Ugly as well, but at least it works.

[KodrAus]

@zrzka So it looks like this is actually the way we're recommending doing this right now, so if you'd like to re-open this PR as-is then I'm on board with the approach.

Sorry about the misdirection here :) I can live with a solution that has drawbacks if it's consistent with what other libraries are doing.

[zrzka]

@KodrAus np, next week.

[zrzka]

Restored & reopened as requested. Summary:

• rand version bumped
• stdweb & wasm-bindgen features added, they just transitively enables same features in the rand crate
• if anyone wants to use v3, v4, v5 on wasm32 arch, stdweb and/or wasm-bindgen feature(s) must be enabled otherwise these modules wont be available
• Travis CI builds these combinations for wasm32-unknown-unknown
• Note added to lib.rs about this behavior

[Dylan-DPC-zz]

This fails on stable & beta with an issue on stdweb:

error[E0554]: #![feature] may not be used on the stable release channel
   --> /home/travis/.cargo/registry/src/github.com-1ecc6299db9ec823/stdweb-0.4.10/src/lib.rs:115:5
    |
115 | /     feature(use_extern_macros)
116 | | )]
    | |_^

[zrzka]

Yep, stdweb requires nightly. I reschuffled .travis.yml little bit. Also 1.22 doesn't have support form wasm32-unknown-unknown, so, I removed it from the main script.

[Dylan-DPC-zz]

Yeah they fixed the issue in this commit (koute/stdweb@f1fc5e3) and it will work on stable once it is released.

[Dylan-DPC-zz]

bors: r+

[bors]

Build succeeded

• continuous-integration/travis-ci/push
• continuous-integration/appveyor/branch