search

anp / moxie

lightweight platform-agnostic tools for declarative UI
https://moxie.rs
Apache License 2.0
828 stars 27 forks source link

Add web features to the `parking_lot` dependency #225

Closed zetanumbers closed 3 years ago

zetanumbers commented 3 years ago

Resolves #218.

The parking_lot for wasm32 on stable Rust will panic on attempt of parking thread. This makes it possible to rely on stable version of parking_lot either with or without the atomics wasm32 target feature present. Therefore it is a step towards the stable toolchain (#221). Then the last step would be to disable (maybe conditionally) the named-profiles cargo feature.

While similar PR #219 conflicts with #223, this PR won't.

zetanumbers commented 3 years ago

I think the "proper" way to do this in cargo is for each of these crates to have their own wasm-bindgen features, and then to depend on those features only when building for wasm32-unknown-unknown. That's going to be really painful though...

Well this feature wasm-bindgen is forwarded to the instant crate. They specify that using these features on native platforms does nothing (means no conditional compilation is required), but not using any of them on wasm32 without wasi will cause linking of some functions from js environment (which was in the name of related issue btw). And I think wasm-bindgen is preferable to stdweb because of using the same deps, but i could be wrong.

...I'm thinking that we should probably switch from parking_lot's locks to std::sync's locks and avoid the issue altogether.

parking_lot has RwLock fair for writers policy, but std::sync's one is unspecified. This is going to matter with #223, bc with std::sync it would not guarantee (for example wasm32 target) that on Var::enqueue_commit could starve the Runtime::run_once at the start. While this is maybe not plausible, it would not be a guarantee. (Note that i should look into that in my PR.)

Also i don't see how conditional compilation in rust is too complicated (please don't listen to me, I had C++ template meta-programing in my homework)

anp commented 3 years ago

Thank you again for this! It does locally resolve the chromedriver issue that is happening in CI, so I've cherry-picked these changes onto #226. Hopefully that will get CI unblocked.

Replying:

Well this feature wasm-bindgen is forwarded to the instant crate.

Exactly -- since dyn-cache, moxie, and topo don't know whether they're being run on the web or not, forwarding the feature seems right. While the feature is a no-op on non-web targets right now, that could change.

Don't worry about this, I'll take care of it in commits atop yours in #226. EDIT: done

And I think wasm-bindgen is preferable to stdweb because of using the same deps, but i could be wrong.

Yep, I don't think stdweb would even work here.

with std::sync it would not guarantee (for example wasm32 target) that on Var::enqueue_commit could starve the Runtime::run_once at the start

I'm not sure starvation is actually what we want, but it's straightforward enough to pull this fix in and revisit later!

anp commented 3 years ago

Thank you again for finding this! I've landed #226 which includes these changes.