Skip to content
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

Fix support for WebAssembly #18

Merged
merged 2 commits into from
Apr 4, 2023
Merged

Conversation

CryZe
Copy link
Contributor

@CryZe CryZe commented Apr 1, 2023

The crate previously assumed that wasm-bindgen can be used for every WebAssembly target. However there's essentially the following 4 targets:

  • WebAssembly on the web
  • Freestanding WebAssembly
  • WebAssembly with WASI
  • WebAssembly with Emscripten
    (And possibly more in the future)

Only WebAssembly on the web supports wasm-bindgen. Unfortunately that's not a target on its own for historical reasons, so the only way to properly support that target is through a wasm-web feature. All other WebAssembly targets now fall back onto a new fallback implementation that always returns None. The crate now also should be able to build at all on unsupported targets because of that.

This is technically a breaking change that requires a new "major version" (0.3.0).

The crate previously assumed that `wasm-bindgen` can be used for every
WebAssembly target. However there's essentially the following 4 targets:
 - WebAssembly on the web
 - Freestanding WebAssembly
 - WebAssembly with WASI
 - WebAssembly with Emscripten

Only `WebAssembly on the web` supports `wasm-bindgen`. Unfortunately
that's not a target on its own for historical reasons, so the only way
to properly support that target is through a `wasm-web` feature. All
other WebAssembly targets now fall back onto a new fallback
implementation that always returns `None`. The crate now also should be
able to build at all on unsupported targets because of that.
@complexspaces
Copy link
Collaborator

Hey there, thanks for the PR. To make sure I understand correctly, there are two intents with this?

  • Allow previously unsupported target triples to compile sys-locale
  • Ensure that the "weird" browser WASM target isn't accidentally used in other WASM runtime environments

so the only way to properly support that target is through a wasm-web feature

Do you have strong opinions on the variable name? Other crates, such as getrandom and ring, seem to have settled on having js somewhere in the name to indicate access to wasm-bindgen-like code.

Regarding the others, do you know if WASI or Emscripten support getting the locale information from the runtime? I found this PR for Emscripten that seems to provide it via ENV['LANG'] but WASI does not appear to support this yet.

@CryZe
Copy link
Contributor Author

CryZe commented Apr 2, 2023

Do you have strong opinions on the variable name? Other crates, such as getrandom and ring, seem to have settled on having js somewhere in the name to indicate access to wasm-bindgen-like code.

I don't particularly care about the feature name, so js is fine with me (although web seems more accurate than JS as you are interacting with the Web APIs and not necessarily with JS, especially in the future where WASM doesn't need any glue code anymore).

Regarding the others, do you know if WASI or Emscripten support getting the locale information from the runtime? I found emscripten-core/emscripten#8751 for Emscripten that seems to provide it via ENV['LANG'] but WASI WebAssembly/WASI#167 appear to support this yet.

Yeah so it doesn't look like WASI can be supported yet, but as it turns out, Emscripten counts as a Unix, so the approach in the Unix code should work. However that means that Emscripten satisifes both WASM and Unix, so I'll push a fix that ensures Emscripten doesn't accidentally compile in two implementations.

@CryZe CryZe force-pushed the fix-wasm branch 3 times, most recently from cf693b2 to a088c12 Compare April 2, 2023 13:20
This renames the `wasm-web` feature to `js` like in some more common
crates in the ecosystem and ensures that Emscripten doesn't accidentally
compile in two implementations, one for WASM, one for Unix.
@CryZe
Copy link
Contributor Author

CryZe commented Apr 3, 2023

This should be ready again.

Copy link
Collaborator

@complexspaces complexspaces left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yup, sorry for the gap. I tested the cfg blocks out locally to double check, but this is good to go now. Thanks for working on this!

@complexspaces
Copy link
Collaborator

This is now released as v0.3.0 on crates.io.

@CryZe CryZe deleted the fix-wasm branch April 4, 2023 18:28
@CryZe
Copy link
Contributor Author

CryZe commented Apr 4, 2023

Thank you so much :)

@CryZe CryZe restored the fix-wasm branch April 4, 2023 18:28
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants