Update CI test for wasm32-unknown-unknown to enable wasmbind feature

[esheppa]

Testing whether this triggers the CI

[djc]

Hah, good call.

[esheppa]

I'm thinking of removing branches: [main] as well, that way PR's to PR's will also run CI which would be great

[djc]

Sounds good! In a separate commit?

[esheppa]

Yep - just squashing them

[djc]

I meant, would be good to have the test.yml changes in a separate commit from the Cargo.toml changes since they're conceptually independent.

[esheppa]

Good point, will push up two separate commits

[djc]

Also I think we'll no longer need @AmateurECE's original commit as part of this PR, right?

[esheppa]

I initially thought we still needed, but then I saw the inconsistency between the #[cfg flags in the Cargo.toml and in the code, so I just used #[cfg(all(target_arch = "wasm32", not(any(target_os = "emscripten", target_os = "wasi"))))] and its negation everywhere which avoids breaking emscripten while still removing use of the wasmbind feature. This should be safe as using rustup target list there are only three wasm targets, wasm32-unknown-unknown, wasm32-unknown-emscripten and wasm32-wasi, so there shouldn't be any other wasm targets getting unexpected wasm-bindgen or js-sys

[morenol]

Hey, now that the wasm-bindgen is in the dependency tree and it is not possible to remove that from wasm targets, it adds a bunch of wasm_bindgen imports that runtimes like wasmtime and wasmer don't provide. Is it ok if I create a PR adding a feature_flag that is off by default that we can use to disable wasm_bingen from being installed?

RIght now I am having difficulties in using chrono crate with wasmtime. In chrono = 0.4.19 I didnt have that issue

[esheppa]

@morenol - this PR was to fix #740 where there is essentially a discoverability problem, however it is not ideal that we have broken your use case. If I'm understanding you correctly, you are proposing to have a feature which disables wasm-bindgen, but this might break the additive nature of features. Instead, it may be possible to make those two dependencies optional and add wasmbind to the default features (and include wasm-bindgen and js-sys in wasmbind feature)?

Also to note, because the features would be additive, if any of your dependencies used the wasmbind feature, then you would get those crates in your tree as well. Is this preventing you from using chrono with wasmtime, eg compile error or runtime error - or is this more of an efficiency concern?

[morenol]

This is preventing me from using chrono with wasmtime. The solution that you propose would also work, the thing that I need is to disable wasm-bindgen from the dependency tree.

The issue that I am having is that when wasm-bindgen is in the dependency tree, it injects some imports that are intended for JavaScripts hosts.

Just as a reference, this is the specific line in wasmtime that is returing an error:

https://github.com/bytecodealliance/wasmtime/blob/c3e31c99461f7f109637528d805dbaa349cf4a22/crates/wasmtime/src/instance.rs#L830

[djc]

Instead, it may be possible to make those two dependencies optional and add wasmbind to the default features (and include wasm-bindgen and js-sys in wasmbind feature)?

This sounds good to me.

[esheppa]

@morenol - are you using the target wasm32-wasi or wasm32-unknown-unknown?

[morenol]

wasm32-unknown-unknown, I am able to compile without issues, the problem arises when trying to instanciate the module in wasmtime

[esheppa]

I've set up #771 allow this. Do you mind checking whether it works for you? I might need to make a few more changes. We can continue the conversation over there