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

Split up to wasm-bindgen and to a new wasm-bindgen-core #2770

Closed
05storm26 opened this issue Jan 16, 2022 · 7 comments · Fixed by #3031
Closed

Split up to wasm-bindgen and to a new wasm-bindgen-core #2770

05storm26 opened this issue Jan 16, 2022 · 7 comments · Fixed by #3031

Comments

@05storm26
Copy link

Motivation

Break the ahash dependency cycle:

error: cyclic package dependency: package ahash v0.7.4 depends on itself. Cycle:
package ahash v0.7.4
... which is depended on by hashbrown v0.11.2
... which is depended on by indexmap v1.7.0
... which is depended on by serde_json v1.0.64
... which is depended on by wasm-bindgen v0.2.74
... which is depended on by js-sys v0.3.51
... which is depended on by getrandom v0.2.3
... which is depended on by ahash v0.7.4

Currently getrandom uses wasm-bindgen just to be able to use:

The contents of:

Due to feature unification, several crates end up enabling features inside wasm-bindgen and its downstream dependencies that causes a dependency cycle that affects various parts of the rust community:

tkaitchuck/aHash#95

Proposed Solution

This: tkaitchuck/aHash#95 (comment)

js-sys would then only depend on wasm-bindgen-core, the full wasm-bindgen would just reexport wasm-bindgen-core, and getrandom would just depend on wasm-bindgen-core and js-sys.

Alternatives

Break the ahash dependency cycle somewhere else.

But creating a minimal wasm crate that can be used by low-level dependencies like getrandom without the risk of pulling in too many dependencies due to feature unification seems to be the most elegant solution.

Additional Context

Crates like wasm-bindgen, getrandom are seemingly cornerstones to this ecosystem, it would be great if fundamental crates would compile.

@maxcountryman
Copy link
Contributor

Are there any workarounds in the meantime?

@andoriyu
Copy link

@maxcountryman lock indexmap like this: indexmap = "~1.6.2"

@ctron
Copy link

ctron commented Aug 9, 2022

So I ended up here too. It looks like the only working solution right now is to pin inedxmap to 1.6.2 as described. Which is pretty annoying. I also found no way to prevent wasm-bindgen from importing serde_json. But I guess there are several ways this could triggered.

@Liamolucko
Copy link
Collaborator

js-sys would then only depend on wasm-bindgen-core, the full wasm-bindgen would just reexport wasm-bindgen-core, and getrandom would just depend on wasm-bindgen-core and js-sys.

Unfortunately, that won't work - from_serde/into_serde are inherent methods of JsValue, which can't be added on by wasm-bindgen because of the orphan rule. Also, wasm-bindgen is pretty much already a 'core' crate; there wouldn't be much to remove in wasm-bindgen-core, so having the distinction would be weird even if it did work.

I was hoping to try and replace serde_json with something else like serde-wasm-bindgen, but from_serde/into_serde both return serde_json::Result, so it's impossible to drop the serde_json dependency without it being a breaking change.

So, I think the best thing we can do is deprecate the serde-serialize feature. It's pretty easily replaced by serde-wasm-bindgen or using serde_json manually, so it should be easy for users of it to switch away, and then there hopefully won't be any crates that enable the feature and trigger this dependency cycle.

@alexcrichton What do you think?

@alexcrichton
Copy link
Contributor

Yes I think deprecation is fine myself.

@ctron
Copy link

ctron commented Aug 16, 2022

It might make sense to create a test for this, just to ensure that this indeed works. Now and in the future.

@ranile
Copy link
Collaborator

ranile commented Aug 21, 2022

gloo-utils v0.1.5 has been released that provides an alternate (with identical behavior) to JsValue::{from_serde,into_serde}

Thanks to @andoriyu for their contributions to make this happen.

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.

7 participants