piccolo icon indicating copy to clipboard operation
piccolo copied to clipboard

Replace `rand` crate with `fastrand`

Open Gordon-F opened this issue 9 months ago • 8 comments

These changes allow piccolo to compile for wasm32-unknown-unknown target.

Gordon-F avatar Sep 26 '23 20:09 Gordon-F

There's no reason to swap to the fastrand crate just to get wasm32 support, it works just as easily with the rand crate and the getrandom dependency. In fact, no changes to piccolo are required at all, except that a downstream project might need to add a false dependency on getrandom with the js feature to make it work. I use piccolo in a browser right now and that's what I do.

I'm not sure what is the most correct thing for piccolo to do though, if it's more correct to enable the getrandom js feature here for the wasm32-unknown-unknown target or let the final application do it.

kyren avatar Sep 27 '23 10:09 kyren

add a false dependency on getrandom with the js feature to make it work

Nice! Sorry, I misread rand docs. I pushed updated pull request with getrandom dependency for wasm32 targets.

Gordon-F avatar Sep 28 '23 14:09 Gordon-F

Yeah, that's more like what I was thinking, but now the question is... is this the correct thing to do, or should we just let the end user do it?

Do we enable the "js" feature unconditionally when the target arch is wasm32? If so, that just kind of raises the question of why doesn't getrandom do this already?

My guess is that the answer is because the target is wasm32-unknown-unknown, javascript isn't really strictly implied.

So, if we accept that, we can EITHER have our own "js" feature which seems pretty heavyweight, or maybe just... leave the status quo and let the end user manually inject the "js" feature into getrandom? Or do something else where we ask the user to seed the global random number generator?

I dunno, that's kind of why things were left like they are now actually, because I didn't know the correct answer.

kyren avatar Sep 30 '23 00:09 kyren

From getrandom doc:

This crate fully supports the wasm32-wasi and wasm32-unknown-emscripten targets. However, the wasm32-unknown-unknown target (i.e. the target used by wasm-pack) is not automatically supported since, from the target name alone, we cannot deduce which JavaScript interface is in use (or if JavaScript is available at all).

About js feature:

This feature should only be enabled for binary, test, or benchmark crates. Library crates should generally not enable this feature, leaving such a decision to users of their library. Also, libraries should not introduce their own js features just to enable getrandom’s js feature. This feature has no effect on targets other than wasm32-unknown-unknown.

Gordon-F avatar Sep 30 '23 17:09 Gordon-F

Just my 2¢:

If the only wasm32-unknown-unknown platform Piccolo supports is Web, then I would argue enabling getrandoms js feature when on this target makes sense.

But if Piccolo intends to support other platforms, e.g. running on Wasmtime (requires an API to supply a random function by the user), it should not be enabled and left to the user instead. Though I don't see anything particularly wrong with re-exposing a js crate feature in Piccolo either.

daxpedda avatar Jan 03 '24 08:01 daxpedda

I definitely would want to run piccolo on wasmtime. I have a wasm based plugin system, so running Lua scripts via piccolo would be great.

CryZe avatar Jan 03 '24 08:01 CryZe

(Wasmtime does support wasm32-wasi, so my comment only pertains to running wasm32-unknown-unknown on Wasmtime, which might be a requirements for some use-cases)

daxpedda avatar Jan 03 '24 08:01 daxpedda

I'm planning on addressing this in the near future when I add explicit support for no_std. There should definitely be a version that just accepts a random seed from the user.

I think at that point just having a "getrandom" feature would be enough, and that can provide a math lib load function that doesn't require a random seed.

We could also provide maybe both "getrandom" and "getrandom-js" features so the user doesn't have to manually inject the "js" feature, but this is quite a lot of ceremony for what amounts to a single function call.

kyren avatar Jan 03 '24 12:01 kyren