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

bevy_asset: Add missing web-sys feature and cleanup unused ones #13281

Merged
merged 3 commits into from May 12, 2024

Conversation

snendev
Copy link
Contributor

@snendev snendev commented May 8, 2024

Objective

  • Describe the objective or issue this PR addresses.

bevy_asset includes code here that references web_sys::WorkerGlobalScope. However, bevy_asset does not enable this feature, see here. Running examples does not catch this problem because the feature is implicitly included by wgpu when bevy_render is also a dependency, see bevy_render and wgpu. This results in compile errors for environments that are not using bevy_render.

To reproduce the problem, try to build the crate individually for wasm targets by running cargo build -p bevy_asset --target wasm32-unknown-unknown.

Running cargo tree -e features --target wasm32-unknown-unknown helped diagnose the issue.

Solution

  • Describe the solution used to achieve the objective above.

This PR adds the WorkerGlobalScope feature to the web-sys portion of bevy_asset's Cargo.toml.

It also seems to be the case that bevy_asset no longer needs the Request feature, since no code for Request is present anymore. I confirmed that building the crate individually for wasm succeeds without the feature, so that change is also included here.

This is a little off-topic, but the repository would probably benefit from some automation around these types of changes, but I'm not sure what would work there. For example, building each crate individually for some key targets would work, but is...well, a lot. Happy to follow up if there is agreement on a good direction.

Testing

  • Did you test these changes? If so, how?
  • How can other people (reviewers) test your changes? Is there anything specific they need to know?

Building the crate individually for wasm by running cargo build -p bevy_asset --target wasm32-unknown-unknown.

  • Are there any parts that need more testing?

I don't believe so.

Copy link
Contributor

github-actions bot commented May 8, 2024

Welcome, new contributor!

Please make sure you've read our contributing guide and we look forward to reviewing your pull request shortly ✨

@snendev snendev changed the title Add missing web-sys feature and cleanup unused ones bevy_asset: Add missing web-sys feature and cleanup unused ones May 8, 2024
@james7132 james7132 added A-Assets Load files from disk to use for things like images, models, and sounds C-Dependencies A change to the crates that Bevy depends on D-Trivial Nice and easy! A great choice to get started with Bevy X-Uncontroversial This work is generally agreed upon labels May 12, 2024
@james7132 james7132 added this pull request to the merge queue May 12, 2024
Merged via the queue into bevyengine:main with commit a4597a9 May 12, 2024
35 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-Assets Load files from disk to use for things like images, models, and sounds C-Dependencies A change to the crates that Bevy depends on D-Trivial Nice and easy! A great choice to get started with Bevy X-Uncontroversial This work is generally agreed upon
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants