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

Feature gate web-sys #904

Closed
wants to merge 2 commits into from
Closed

Conversation

Jake-Shadle
Copy link

The web-sys crate brings in a ton of extra dependencies unconditionally, this just feature gates web-sys behind the wasm feature so that users can opt in to those extra dependencies, if they are even targeting wasm at all.

@Jake-Shadle
Copy link
Author

Probably makes sense to rename the feature from wasm to wasm-web or something to take into account the WASI use case coming.

@briansmith
Copy link
Owner

@Jake-Shadle Thanks for filing the issue. However, I don't understand the problem.

You wrote:

The web-sys crate brings in a ton of extra dependencies unconditionally, this just feature gates web-sys behind the wasm feature so that users can opt in to those extra dependencies, if they are even targeting wasm at all.

The Cargo.toml already has:

[target.'cfg(all(target_arch = "wasm32", target_vendor = "unknown", target_os = "unknown", target_env = ""))'.dependencies]	[target.'cfg(all(target_arch = "wasm32", target_vendor = "unknown", target_os = "unknown", target_env = ""))'.dependencies]
web-sys = { version = "0.3.25", default-features = false, features = ["Crypto", "Window"] }

So, doesn't the fact that this is a target-specific dependency already do what is proposed here?

@Jake-Shadle
Copy link
Author

It doesn't build them unless you build for wasm, but it does impact dependency resolution and is included as a part of the lock file.

@briansmith
Copy link
Owner

It doesn't build them unless you build for wasm, but it does impact dependency resolution and is included as a part of the lock file.

OK, but why does that matter? I would prefer a messy Cargo.lock over messy code/configuration logic, if all other considerations are equal.

In another project I do have an issue where we're trying to generate "dependency reports" for security auditing purposes and we have to be careful that we are doing that kind of stuff on a per-target (per-configuration, really) basis instead of just using a tool that reads Cargo.lock and assumes every dependency mentioned there is actually used. I think some tools need to improve in this regard. Is your issue similar to this?

@Jake-Shadle
Copy link
Author

Yes, our situation is basically the same with cargo-deny, where features are the cleanest way to keep your dependency graph simple. However, the target configuration will be accessible more readily from cargo metadata in 1.41, so we'll be fine once that lands.

@briansmith
Copy link
Owner

Yes, our situation is basically the same with cargo-deny, where features are the cleanest way to keep your dependency graph simple. However, the target configuration will be accessible more readily from cargo metadata in 1.41, so we'll be fine once that lands.

Great. I commented in the cargo-deny issue: EmbarkStudios/cargo-deny#63.

I'm going to close this on the assumption that the improvements to cargo and other tools will make this unnecessary to do.

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.

2 participants