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

Feature flag for the new wasm javascript support? #334

Closed
repi opened this issue Sep 1, 2019 · 10 comments · Fixed by #335
Closed

Feature flag for the new wasm javascript support? #334

repi opened this issue Sep 1, 2019 · 10 comments · Fixed by #335

Comments

@repi
Copy link

repi commented Sep 1, 2019

In the new 0.4.8 release javascript/browser WASM support was added in #331. Would it be possible get a feature flag for this support, instead of having it always on?

This support and the dependencies on wasm-bindgen and js-sys when building for wasm32-unknown-unknown only works in a browser environment and not when running wasm outside of a browser, which is what we (and others also) do.

Togglable javascript wasm support through a feature flag could be nice for those that do not use wasm also as then they can disable that feature flag (can be on by default though) and not have get these extra dependencies (unused by in their Cargo.lock files):

      Adding js-sys v0.3.27
      Adding wasm-bindgen v0.2.50
      Adding wasm-bindgen-backend v0.2.50
      Adding wasm-bindgen-macro v0.2.50
      Adding wasm-bindgen-macro-support v0.2.50
      Adding wasm-bindgen-shared v0.2.50

I guess this is a general issue with the general and non-specific wasm32-unknown-unknown target, it doesn't specify which environment it will be run in (compared to say wasm32-wasi or wasm32-unknown-emscripten) though haven't run into that many other issues like this, likely because quite a few other crates have feature flags for stdweb or similar.

@CryZe
Copy link
Contributor

CryZe commented Sep 1, 2019

I didn't check yet, but my code is probably running into this too. I'm really wondering why there's no stronger push for an actual wasm32-web target. Solving what is essentially a different target through features is just weird and the whole ecosystem is suffering because of it. And it could allow for std to directly use wasm-bindgen as well, such as support for println!(), dbg!(), panic printing and co.

@repi
Copy link
Author

repi commented Sep 1, 2019

Yeah fully agree, that would be better for everyone. Likely are some discussions or plans somewhere as WASM in Rust is a pretty hot and active topic, and with a specific rust wasm working group also.

But in the meantime think having feature flags in the crates is likely the best one can do for now?

@quodlibetor
Copy link
Contributor

Interesting, I really thought that the fact that it was gated to the 'cfg(all(target_arch = "wasm32", not(target_os = "emscripten")))' would mean that it would only get installed when necessary.

I'll publish a feature-flagged version tomorrow, unless folks come up with a better option.

@CryZe
Copy link
Contributor

CryZe commented Sep 2, 2019

Well theoretically there are like 4 different targets that matter:

  • WASM + Emscripten wasm32-unknown-emscripten
  • WASM + wasm-bindgen wasm32-unknown-unknown with wasm-bindgen feature
  • WASM + WASI wasm32-wasi
  • WASM without anything wasm32-unknown-unknown (you can mostly treat this like core + alloc)

@quodlibetor
Copy link
Contributor

@repi @CryZe could you test your setups with #335 ? If that works for you I'll release it as 0.4.9. Feel free to request any changes on it if you think that it can be improved for the wasm ecosystem.

@repi
Copy link
Author

repi commented Sep 2, 2019

@quodlibetor yes can try it out. right now that PR fails on CI though

@quodlibetor
Copy link
Contributor

@repi it's failing because I forgot to add the wasmbind feature flag to the tests, it shouldn't affect you running without wasm-bindgen.

@repi
Copy link
Author

repi commented Sep 3, 2019

All of these type of lines in the crate need to be feature gated behind the new feature also for it to compile:

    #[cfg(all(target_arch = "wasm32", not(target_os = "emscripten")))]
    extern crate wasm_bindgen;

    #[cfg(not(all(target_arch = "wasm32", not(target_os = "emscripten"))))]
    pub fn now() -> DateTime<Utc> {

but with that fixed this should work well for our use case and doesn't pull in the wasm-bindgen dependencies, thx!

@quodlibetor
Copy link
Contributor

Thanks! That's good news.

@quodlibetor
Copy link
Contributor

I've updated that pr with what appears to be actually correct code, I'll release tonight unless I get some pushback that a release is a bad idea.

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 a pull request may close this issue.

3 participants