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

Breaking change due to removal of snippets method in wasm-bindgen-cli-support 0.2.93 #4159

Closed
gbj opened this issue Oct 10, 2024 · 8 comments · Fixed by #4172
Closed

Breaking change due to removal of snippets method in wasm-bindgen-cli-support 0.2.93 #4159

gbj opened this issue Oct 10, 2024 · 8 comments · Fixed by #4172
Labels

Comments

@gbj
Copy link
Contributor

gbj commented Oct 10, 2024

Describe the Bug

PR #4066 removes the methods Output::snippets() and Output::local_modules(), both of which we call in a build tool that depends on wasm-bindgen-cli-support.

Because of wasm-bindgen versioning requirements we need to release new versions of the tool whenever wasm-bindgen updates. We understand this process and can usually just bump the patch release and make a new release ourselves.

I don't see another way to access the fields that were removed in this changed, as the getters are gone and the fields are not pub.

Steps to Reproduce

We have a couple lines that are roughly

    let mut js_changed = false;

    js_changed |= write_snippets(proj, bindgen.snippets()).await?;

    js_changed |= write_modules(proj, bindgen.local_modules()).await?;

They no longer compile due to the methods being removed in the patch release.

Additional Context

This is in https://github.com/leptos-rs/cargo-leptos/

@IceTDrinker
Copy link

IceTDrinker commented Oct 10, 2024

Hitting the same bug notably causing wasm-bindgen-rayon to be missing the helper .js files to make it work when generating bindings for a rust code base to wasm + js

@daxpedda
Copy link
Collaborator

Unfortunately wasm-bindgen-cli-support doesn't have a stable API, as noted in the description: its an "internal dependency".

However I don't see anything wrong with exposing these two functions again.
I wasn't aware that anybody is using those. For the future it might be best to discuss stabilizing at least parts of these crates that are needed elsewhere.

@IceTDrinker
Copy link

In the case of the wasm-bindgen-rayon crate I don't know what they do, but some behavior of wasm-bindgen changed enough that the snippet directory that should be populated has missing files and results in an unusable js package

@benwis
Copy link

benwis commented Oct 10, 2024

Unfortunately wasm-bindgen-cli-support doesn't have a stable API, as noted in the description: its an "internal dependency".

However I don't see anything wrong with exposing these two functions again. I wasn't aware that anybody is using those. For the future it might be best to discuss stabilizing at least parts of these crates that are needed elsewhere.

In the interest of bug resolution, would you be able to push out a hotfix version for this, or would it be better for us to pin our wasm-bindgen to the previous version or remove that optimization from our tooling for now

@daxpedda
Copy link
Collaborator

In the case of the wasm-bindgen-rayon crate I don't know what they do, but some behavior of wasm-bindgen changed enough that the snippet directory that should be populated has missing files and results in an unusable js package

I am assuming you are talking about the workaround Rayon was using described in #3330 (comment). In this case Rayon was depending on a bug in wasm-bindgen. There should be no missing files in the snippet directory with the new version (unless a new bug was introduced).

Unfortunately wasm-bindgen-cli-support doesn't have a stable API, as noted in the description: its an "internal dependency".
However I don't see anything wrong with exposing these two functions again. I wasn't aware that anybody is using those. For the future it might be best to discuss stabilizing at least parts of these crates that are needed elsewhere.

In the interest of bug resolution, would you be able to push out a hotfix version for this, or would it be better for us to pin our wasm-bindgen to the previous version or remove that optimization from our tooling for now

I'm happy to push out a new hotfix for this later today.
But in general I can't recommend anybody to rely on unstable APIs.

@IceTDrinker
Copy link

In the case of the wasm-bindgen-rayon crate I don't know what they do, but some behavior of wasm-bindgen changed enough that the snippet directory that should be populated has missing files and results in an unusable js package

I am assuming you are talking about the workaround Rayon was using described in #3330 (comment). In this case Rayon was depending on a bug in wasm-bindgen. There should be no missing files in the snippet directory with the new version (unless a new bug was introduced).

Unsure about the workaround, all I can say is whatever they are using wasm-bindgen for the snippet directory is not working anymore,, Hyrum's law at work I guess.

In any case I have no idea if it's a wasm-bindgen-rayon or a wasm-bindgen issue but as you were the last depedency to update I searched for an issue here :)

In any case thanks for the work that went into wasm-bindgen, very useful tooling for the rust ecosystem

@gbj
Copy link
Contributor Author

gbj commented Oct 10, 2024

Understood that the policy is "don't use this crate." We are in the process of rewriting our build tool with the possibility of calling out to wasm-bindgen rather than depending on wasm-bindgen-cli-support, to avoid these issues. Unfortunately it's just a matter of legacy code at this point; the original creator of the tool has moved on and time is scarce to rewrite so that we can remove the dependency.

Thanks so much for your work maintaining these tools.

@codeart1st
Copy link

codeart1st commented Oct 13, 2024

Hitting the same bug notably causing wasm-bindgen-rayon to be missing the helper .js files to make it work when generating bindings for a rust code base to wasm + js

I also find this problem in my project. The problem for me is, that wasm-pack not allow to pass --split-linked-modules zu wasm-bindgen for now.

Using WASM_BINDGEN_SPLIT_LINKED_MODULES=1 also not working for me with wasm-pack.

sd2k added a commit to grafana/augurs that referenced this issue Oct 16, 2024
Hopefully temporarily until a solution to [this comment]
is found.

[this comment]: rustwasm/wasm-bindgen#4159 (comment)
codeart1st added a commit to codeart1st/wgpu-layers that referenced this issue Nov 2, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging a pull request may close this issue.

5 participants