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

Add support for wasm-bindgen #103

Open
wants to merge 4 commits into
base: master
Choose a base branch
from

Conversation

RReverser
Copy link

Adds support for num_cpus for wasm32 when used with wasm-bindgen.

When used, num_cpus will return result of navigator.hardwareConcurrency.

This is an alternative to #89 with few notable differences:

Adds support for num_cpus for wasm32 when used with wasm-bindgen.

wasm-bindgen is added as an optional dependency / feature.

When used, num_cpus will return result of navigator.hardwareConcurrency.
src/lib.rs Show resolved Hide resolved
@RReverser
Copy link
Author

@seanmonstar Can you take a look at the PR please? I don't really understand the errors on old Rust tbh - it doesn't point to any "special" syntax, but maybe I'm missing something that I'm too used to by now but didn't work on Rust 1.13?

src/lib.rs Outdated Show resolved Hide resolved
Co-authored-by: Oliver Wangler <oliver@wngr.de>
@peter17
Copy link

peter17 commented Jan 4, 2023

@seanmonstar Could you please have a look at this PR? Thanks a lot!

@DouglasDwyer
Copy link

DouglasDwyer commented Jul 28, 2023

I would also love to see this merged! It would be great to have a cross-platform solution that works on the web.

@RReverser, is there any chance that the #[cfg(feature = "wasm-bindgen")] blocks could also be gated behind a WASM architecture flag? #[cfg(all(target_arch = "wasm32", feature = "wasm-bindgen"))], for instance. Right now, if I compile your branch to Windows while wasm-bindgen is enabled there are compilation issues (Rust tries to compile the WASM-specific stuff). While I can work around this in my Cargo.toml, it would be easier for cross-compilation purposes if the wasm-bindgen flag didn't preclude compiling on other platforms.

Edit: reading through your code review, I see that this issue was already discussed. But I'd like to make another push for allowing compilation on all platforms while the feature is active. You can't do platform-specific dependencies in Cargo workspaces, which makes using this crate in workspaces harder. Further, there are a lot of crates - like the instant crate or the zstd crate - which have WASM-specific features that still allow for compiling to all platforms. The wasm-bindgen feature causing compilation issues here feels unconventional, at least from my perspective :)

@RReverser
Copy link
Author

while wasm-bindgen is enabled there are compilation issues

Why would you enable wasm-bindgen on other platforms?

Either way, given how long this PR was open and given that there is now a built-in function for number of CPUs in stdlib, I suspect it will never be merged :(

@DouglasDwyer
Copy link

Why would you enable wasm-bindgen on other platforms?

I have a cross-platform application in a Cargo workspace. When I add a dependency, I put it under workspace.dependencies and enable all necessary features - for example, here's the entry for zstd:
zstd = { version = "0.12.3", default-features = false, features = [ "fat-lto", "wasm" ] }
I then can compile my application to desktop or WASM with the appropriate cargo run --target command, and everything works. On desktop, the wasm feature is a no-op. This is simpler; I don't need any target-specific config sections at all. I hope that makes sense.

It's not really a big deal. But I thought to raise the issue because this is contrary to how most other crates deal with WASM-specific features.

Thanks for the great work on this PR @RReverser! I really do appreciate it, and am using your branch in my project at present 😃

@daxpedda
Copy link
Contributor

daxpedda commented Aug 4, 2023

workspace.dependencies doesn't prevent you from using target-specific dependencies.

Usually libraries want to cut down on dependencies as much as possible to reduce compile-time, which is why it's considered quite unpopular to rely on wasm-bindgen on non-Web targets even if it's no-op there.

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.

None yet

6 participants