Proposal for bindgen changes

[josephlr]

This is my idea for improving the wasm32-unknown-unknown with bindgen target.

Note: This is a Work in Progress and contains many ideas for improving the bindgen target. We don't necessarily have to do all of them.

Changes and Motivation

Using js-sys and web-sys

These crates are maintained by the same group that maintains wasm-bindgen and are intended to work similar to how the libc crate works today. Strictly speaking, the libc crate is never necessary. You could always just declare extern "C" functions yourself; however, having the libc crate ensures that these bindings are correct for your platform.

Similarly, you could always declare interfaces to the JS APIs and Browser APIs manually via wasm-bindgen. However, using these binding crates makes our job easier:

• It allows us to properly obtain the global object (doing this correctly is quite complex). See Fix CSP error for wasm bindgen #92
• When host bindings are supported, we don't have to do the work of wiring everything up.
• These bindings are extremely subtle (even more so than libc) due to the complex mapping between Rust and JS types. It's better to just have someone else deal with them and have them fix bindgen bugs.

Furthermore, the web-sys crate is configurable, so you only need to build what you need to use (we only need Crypto, Window, and WorkerGlobalScope).

I think that if we're OK depending on crates like libc that are maintained by the Rust team and just declare platform bindings, we should be OK with using a similar crate for wasm.

Better error handling with catch bindings

This change also moves to using catch bindings (the default in js-sys and web-sys) for all of our bindings. This allows us to properly return an error when certain APIs fail (or fail to exist) at runtime.

This also means we no longer need things like get_random_values_fn. If the API is not there, the function will just return an error (undefined I think). This change does not depend on using web-sys/js-sys.

Removing std::thread_local

Right now, the stdweb implementation only checks if we are in Node vs a Browser in its init function. This does the same for bindgen. Given that the cost of retrieving these objects is mostly just the bindgen overhead (which we incur regardless), it doesn't make a whole lot of sense to hold onto a thread_local NodeCrypto. We can just get the object each time we need it.

Future changes

We could additionally add error! calls to actually print out the underlying JS errors.

We could also try to reduce the duplication between the stdweb and bindgen code, but that's better for a future PR.

[newpavlov]

Can you provide a motivation section? IIUC they are abstraction layers built on top of wasm-bindgen, so bringing-in additional dependencies to simplify code a tiny bit goes against the spirit of getrandom in my opinion.

[josephlr]

Can you provide a motivation section? IIUC they are abstraction layers built on top of wasm-bindgen, so bringing-in additional dependencies to simplify code a tiny bit goes against the spirit of getrandom in my opinion.

Done, PTAL. I tried to split up the proposed changes into sections.

[newpavlov]

Hm, you argumentation looks reasonable. I don't have a strong opinion here, so I will leave decision to @dhardy.

Note: if this change will get merged, in rand's Cargo.toml we probably should use:

# deprecated feature, use `bindgen` instead
wasm-bindgen = ["bindgen"]
binidgen = ["getrandom/bindgen"]

[dhardy]

Thanks for the clear motivation section @josephlr! Sounds like a good rationale to me, though perhaps it's worth inviting @alexcrichton / @fitzgen to comment.

[dhardy]

Why not rename the package, like I did here? This removes what is otherwise a breaking change to the API (probably resulting in a compile error if someone tries using the wasm-bindgen feature).

To add: might also be worth adding a comment against the optional crates which users should not use as features.

[dhardy]

Better not to re-use error codes (except where equivalent)? We aren't short on them, and doing so could cause confusion.

[dhardy]

Why .unwrap() and not .ok_or(..)? ?

[alexcrichton]

This seems fine to me either way personally. Writing extern "C" shorthand is fine most of the time if you're only importing one or two functions, so I would consider using #[wasm_bindgen] directly to be basically equivalent for one or two functions. I agree that getting the global object is pretty tricky, though, and using js-sys for that is probably worth it.

[newpavlov]

BTW I think we should not rename the feature. I think the first association with bindgen will be a wrong one. We still use wasm-bindgen under the hood, so it should be fine to continue to use the old name. Plus it will reduce amount of v0.2 breakage.

[josephlr]

Closing this for now. Thanks everyone for the feedback. My idea for solving #4 conflicts with this, so we should resolve those issues before dealing with this one.

BTW I think we should not rename the feature. I think the first association with bindgen will be a wrong one.

That's a fair point, sticking with the name wasm-bindgen (for whatever approach we go with) seems like the best bet.