fix: Break dependency cycle by not using serde-serialize #239
Conversation
|
Can you report the issue on wasm-bindgen repository? This PR seems like a bandaid over a much bigger issue. I'd much rather solve it at wasm-bindgen level and remove the cyclic dependency That is assuming if the error can solved by a change in wasm-bindgen |
|
According to Alex that’s the correct way:
tkaitchuck/aHash#95 (comment)
There is no way to fix `wasm-bindgen` without splitting it into multiple
crates, there is already an issue for this:
rustwasm/wasm-bindgen#2770
While it seems like a bandaid - many people can’t update their dependencies
because of this. Thread i linked earlier has references to crates that did
the same thing to break cycle.
|
|
One way to fix it is to just remove the serde methods from wasm-bindgen. It'll be breaking change but it's one I'm willing to make. Gloo utils can provide a convert module with the 2 functions that are removed from wasm-bindgen @alexcrichton how does wasm-bindgen 0.3 sound? |
I see. Yeah, that's reasonable. Is it possible to still merge this one (after everything passes) as a stop gap meanwhile? I can copy over those two functions to |
andoriyu
force-pushed
the
remove_serde_serialize
branch
from
23c27af
to
9950213
Compare
|
Personally I don't think that |
|
@alexcrichton but if that feature stays in |
|
I am aware that is the consequence of inaction, yes. |
|
I would appreciate a fix on this too. |
|
Sorry, it took so long for me to get back to this. Things happened and I haven't had the time lately.
@alexcrichton, exactly. The dependency cycle will remain in wasm-bindgen. I'm not aware of any way to complete remove the cycle without removing the feature. I'm personally in favor of 0.3. I'd like to go through and make other breaking changes that need to be made as part of wasm-bindgen 0.3. I'll merge this PR for now
@andoriyu yeah, that would be good. I'm going to merge this once the CI is green so if you're up for this, please do so in a separate PR. Also updating wasm-bindgen documentation to point to these functions would be a good idea. Until we remove the serde feature from wasm-bindgen, documenting the solution to the problem sounds like a good plan to me. |
|
The CI test failures are unrelated so I'll go ahead and merge this. Once again, apologies for the very late response. |
I'm getting this error when loading the Rust project from an external
source that uses wasm
```
package: /Users/nijaar/shinkai/develop/shinkai-node/shinkai-libs/shinkai-baml/Cargo.toml
workspace: /Users/nijaar/shinkai/develop/shinkai-node/Cargo.toml
Updating crates.io index
Updating git repository https://github.com/BoundaryML/baml.git
error: cyclic package dependency: package getrandom v0.2.15 depends on itself. Cycle:
package getrandom v0.2.15
... which satisfies dependency getrandom = "^0.2.0" of package const-random-macro v0.1.16
... which satisfies dependency const-random-macro = "^0.1.16" of package const-random v0.1.18
... which satisfies dependency const-random = "^0.1.17" of package ahash v0.8.11
... which satisfies dependency ahash = "^0.8.7" of package hashbrown v0.14.5
... which satisfies dependency hashbrown = "^0.14.1" of package indexmap v2.5.0
... which satisfies dependency indexmap = "^2.2.3" of package serde_json v1.0.128
... which satisfies dependency serde_json = "^1.0" of package wasm-bindgen v0.2.93
... which satisfies dependency wasm-bindgen = "^0.2.93" of package js-sys v0.3.70
... which satisfies dependency js-sys = "^0.3" of package getrandom v0.2.15
... which satisfies dependency getrandom = "^0.2" of package rand_core v0.6.4
... which satisfies dependency rand_core = "^0.6" of package crypto-common v0.1.6
... which satisfies dependency crypto-common = "^0.1.4" of package aead v0.5.2
```
I created another Rust project that uses baml and it wasn't hitting the
issue, so it seems that for some reason projects that also use wasm
trigger the issue.
it seems to be a very common issue
tkaitchuck/aHash#95
rustwasm/gloo#239
the tldr is that the feature `serde-serializer` seems to be the culprit
(deprecated). it's recommended to move to use `serde_wasm_bindgen` which
baml already uses! so the fix seems to be just removing the feature.
From my limited experience of the project and to try to make sure that
the wasm build still compiles and works, i actually compiled it myself
and tested it in a nodejs project, and it continues to work.
Then I imported this branch to the other project and it worked without
hitting the dependency cycle issue.
There is an issue with cyclic dependency when
serde-serializefeature is enabled inwasm-bindgen. Current suggestion is to usejs_sysinstead to avoid it.I’ve checked and modified every crate that was enabling this feature:
consolewas easy to changestoragedidn’t actually depend on this feature, so I just removed it.netthis one was a bit tricky and this the create that I couldn’t run tests for.I ran non-wasm tests, and wasm tests for console and storage.