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

Pin web-sys to 0.3.67 since we are using unstable APIs #5224

Closed
wants to merge 1 commit into from

Conversation

hecrj
Copy link
Contributor

@hecrj hecrj commented Feb 8, 2024

Connections

Description
wgpu relies on unstable APIs of the web-sys crate and the latest 0.3.68 version breaks types related to WebGPU.

Since semantic versioning isn't enforced for unstable APIs in web-sys, we should rely instead on a specific crate version to avoid broken builds in the future.

Testing
N/A

Checklist

  • Run cargo fmt.
  • Run cargo clippy. If applicable, add:
    • --target wasm32-unknown-unknown
    • --target wasm32-unknown-emscripten
  • Run cargo xtask test to run tests.
  • Add change to CHANGELOG.md. See simple instructions inside file.

@hecrj hecrj requested a review from a team as a code owner February 8, 2024 17:49
@kpreid
Copy link
Contributor

kpreid commented Feb 8, 2024

= version requirements are fragile. This will irreparably break builds for any dependent who has another dependency requiring a newer version of web_sys, and isn't even using the unstable apis part of wgpu (which could be webgl backend only, or even a non-web build that happens to also have an unused web_sys dep somewhere else).

This would be worthwhile if wgpu always used web_sys_unstable_apis, but it doesn't.

@hecrj
Copy link
Contributor Author

hecrj commented Feb 8, 2024

It certainly will break builds for some users, but they will break upfront.

The current approach allows for Wasm builds to break after the fact, in code that has already been released.

In fact, 0.19 is actually broken right now.

@kpreid
Copy link
Contributor

kpreid commented Feb 8, 2024

This imposes a very large cost on all users of only stable features ("don't upgrade your other dependencies") in order to benefit users of unstable features.

@kpreid
Copy link
Contributor

kpreid commented Feb 8, 2024

Hmm, would it be possible to have two web-sys dependencies declared, one pinned and one not, and the pinned one only enabled when webgpu is enabled? If Cargo accepts that, it would handle both cases well.

@ErichDonGubler
Copy link
Member

This doesn't solve the immediate problem, but: We currently plan on upgrading web-sys to address (see #5148, #5188). Notice that I was the one who filed rustwasm/wasm-bindgen#3816.

@@ -160,7 +160,7 @@ js-sys = "0.3.67"
wasm-bindgen = "0.2.87"
wasm-bindgen-futures = "0.4.40"
wasm-bindgen-test = "0.3"
web-sys = "0.3.67"
web-sys = "=0.3.67"
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It would be good to have a comment here explaining why we are pinning to a specific version.

@hecrj
Copy link
Contributor Author

hecrj commented Feb 17, 2024

Hmm, would it be possible to have two web-sys dependencies declared, one pinned and one not, and the pinned one only enabled when webgpu is enabled? If Cargo accepts that, it would handle both cases well.

It may be possible with a bit of conditional compilation. I'll look into it.

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 this pull request may close these issues.

None yet

4 participants