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

Cargo Features for *WASM* environments are sub-optimal. #39

Open
stephenmartindale opened this issue Oct 5, 2021 · 1 comment
Open

Comments

@stephenmartindale
Copy link
Contributor

Having exploited the instant crate in my toy WebAssembly game that I've been playing around with, I have come to the conclusion that the way that Cargo features are defined for this crate is a little sub-optimal for two reasons:

  1. They're not self documenting. That is to say, if one intends to target wasm32 using wasm-bindgen and expects that instant will, therefore, use the JavaScript performance.now(), one MUST express the wasm-bindgen feature flag for instant (similarly, for stdweb) but the requirement to do so is easy to miss and missing it results in a rather obtuse error related to JavaScript module imports, at runtime.

  2. Cargo feature flags are supposed to be strictly additive. The Cargo Book says: "...enabling a feature should not disable functionality..." The existence of the inaccurate feature directly contravenes this recommendation and is, for me, an issue because my use-case requires accurate, high-precision performance counters and thus requires me to be sure that inaccurate will not be set.

    • Tangent: I didn't go to all that trouble setting COOP / COEP headers just for another Cargo dep. to sink my timer precision!

I'm happy to help fix this and submit a merge request, improving the use of feature flags, but I am not entirely sure how this should or even could be done, without introducing breaking changes to the crate. My first thought was to handle the first problem with a compilation warning that could be dismissed by setting a feature -- perhaps named js-extern-now -- indicating that the user did, indeed, expect instant to find now() as a JS extern. To handle case 2, perhaps introducing a precise feature which overrides (or conflicts) with inaccurate would help but that seems somewhat messy.

@stephenmartindale
Copy link
Contributor Author

Further on this topic, separated into a comment to preserve the purity of the issue OP, here is a link to an external, down-stream issue (rustwasm/wasm-bindgen#2215) which demonstrate that problem number 1 means that users that consume other crates, like parking_lot, in the WASM environment are expected to know about instant's need for either the wasm-bindgen or stdweb features.

I, myself, stumbled upon this while using wgpu which, in turn, uses parking_lot. I later learned that instant is a very useful crate and interesting to me, too (hence item 2 in the OP) but, at the time, I had never heard of it and certainly did not know its usage requirements.

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

No branches or pull requests

1 participant