Skip to content

Consider using web_sys and unifying wasm32-unknown-unknown targets #63

Description

@josephlr

Right now, stdweb depends on wasm-bindgen so it doesn't really make a whole lot of sense to have two seperate implementations in getrandom for this.

A better approach might just be to depend on the web_sys crate from the wasm-bindgen repo and eliminate the need for separate wasm32 features.

I'm not sure how this would work with Node, but for Client Web, we could just use Crypto::get_random_values_with_u8_array

Activity

  1. newpavlov commented on Jul 11, 2019

    @newpavlov
    Member

    I don't see why we should use web_sys while it's built on top of wasm-bindgen, instead of directly using the latter.

    Previously WASM target was discussed in #17 and we decided to keep it as-is for the time being. Also see this comment.

  2. dhardy commented on Aug 15, 2019

    @dhardy
    Member

    @koute would it be reasonable to make wasm-bindgen a hard dependency for wasm32-unknown-unknown now?

  3. najamelan commented on Aug 15, 2019

    @najamelan

    If stdweb does not give any advantage over wasm-bindgen, and if it can't lead to less deps being pulled in, this would greatly simplify issues downstream... I mean to just get the random data out of wasm-bindgen without features.

  4. newpavlov commented on Aug 15, 2019

    @newpavlov
    Member

    I am strongly against making wasm-bindgen enabled by default for wasm32-unknown-unknown. As I wrote several times, this target can not make any assumptions about a target on which it will be executed. It can be a browser, Node.JS, smart-contract, game module system and who knows what else. If we want to simplify WASM builds for browsers/Node.JS, let's push for dedicated targets instead.

  5. najamelan commented on Aug 15, 2019

    @najamelan

    @newpavlov Does setting stdweb work on these other platforms?

    Or are you just thinking other features like wasm-bindgen and stdweb will maybe be added later? Or do these targets just need to use dummy and they are not supported?

  6. newpavlov commented on Aug 15, 2019

    @newpavlov
    Member

    @najamelan
    We don't have those hypothetical new targets yet, so it's hard to tell if stdweb will work on them. IIRC it can work on other existing WASM/asm.js targets, but we don't use it on them. Don't forget that people use wasm32-unknown-unknown outside of browsers, and even on browsers not everyone uses wasm-bindgen in their build pipeline.

    Ideally I would like to drop feature-gated support for wasm32-unknown-unknown altogether, but I think it will have to wait until #4 and #21 gets properly solved.

  7. dhardy commented on Aug 15, 2019

    @dhardy
    Member

    @newpavlov both our wasm-bindgen and stdweb impls support both browsers and Node.js, with run-time failure on other platforms. IMO this is about as good as is possible for the inspecific wasm32-unknown-unknown target. These impls can potentially be extended to support other interfaces — though likely smart contracts and game modules would either use one of the same interfaces or not support randomness at all. So I don't buy your argument.

    Of course I agree that ideally more specific targets will replace the unknown.

  8. newpavlov commented on Aug 16, 2019

    @newpavlov
    Member

    @dhardy
    Yes, wasm32-unknown-unknown is PITA right now for us and our users. (Overall I think that making it an std target was probably a mistake, motivated by a short-term goals...) But I don't think we should make conceptually wrong compromises (otherwise why shouldn't we support some popular RNG perfirial for embedded ARM targets?). Instead, in my opinion, we should use this pain to motivate Rust improvements.

    Yes, this approach is more painful and less convenient in a short-term, but should be more beneficial in a longer term.

  9. TitanThinktank commented on Aug 25, 2019

    @TitanThinktank
  10. dhardy commented on Aug 31, 2019

    @dhardy
    Member

    #102 addresses half of this issue. To address the other half, we could just make the stdweb feature a synonym for wasm-bindgen since I think there will be no remaining reason to keep both implementations?

  11. newpavlov commented on Aug 31, 2019

    @newpavlov
    Member

    @dhardy
    Citing @koute:

    Even though stdweb will be compatible with wasm-bindgen there are still people who do not use wasm-bindgen. <..> and some people target wasm32-unknown-unknown through cargo-web (so without going through wasm-bindgen).

  12. dhardy commented on Aug 31, 2019

    @dhardy
    Member

    Thanks for the clarification @newpavlov.

  13. josephlr commented on Apr 27, 2020

    @josephlr
    MemberAuthor

    Closing this as we've gone with custom RNG crates to handle the issues on the wasm32-unknown-unknown target, see #109. For getrandom 0.2, building for wasm32-unknown-unknown will result in a compile-error by default. However, users can opt-in to using either a wasm-bindgen or stdweb implementation with getrandom.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Metadata

Metadata

Assignees

No one assigned

    Labels

    No labels
    No labels

    Type

    No type

    Projects

    No projects

      Milestone

      No milestone

      Relationships

      None yet

      Development

      No branches or pull requests

      Issue actions