wasm-bindgen-futures: use queueMicrotask for next tick runs (#3203)

[flumm]

but use catch to wrap it in a Result to be able to fall back to the previous Promsise.then variant.

since this is my first pull request here, i hope the code is ok this way, but a few questions remain:

• i simply imported queueMicrotask in global scope, since this should work for both non workers (window) and workers AFAICT
• i tried to run the tests mentioned here: https://rustwasm.github.io/docs/wasm-bindgen/contributing/testing.html but
  the first one failed (regardless of my changes) and cargo test -p ui-tests also did not work (the remaining tests worked, as well as the ones directly in wasm-bindgen-futures
• i tested Can't catch async Wasm traps in JS #2392 with this, and this now seems to work
• is the way i fall back to the old implementation ok? i couldn't find a way to import bindings 'optionally' or test during runtime if a function exists?
• also is embedding such functionality from javascript directly ok? orwould it better be to add that to web-sys/js-sys first and use that?

Fixes #2392.

[daxpedda]

• i simply imported queueMicrotask in global scope, since this should work for both non workers (window) and workers AFAICT

That's fine.

• i tried to run the tests mentioned here: https://rustwasm.github.io/docs/wasm-bindgen/contributing/testing.html but
  the first one failed (regardless of my changes) and cargo test -p ui-tests also did not work (the remaining tests worked, as well as the ones directly in wasm-bindgen-futures

The contributor experience is awful in this regard and should be improved/fixed.

• i tested Can't catch async Wasm traps in JS #2392 with this, and this now seems to work

Thanks, I added it to the OP.

• is the way i fall back to the old implementation ok? i couldn't find a way to import bindings 'optionally' or test during runtime if a function exists?

We could use OnceLock, but v1.70 seems high right now and we don't have a proper MSRV policy. Alternatively we could use once_cell, but that would be adding another dependency.

@Liamolucko what do you think of adding once_cell to the dependency tree of wasm-bindgen-futures, personally I'm fine with it.

• also is embedding such functionality from javascript directly ok? orwould it better be to add that to web-sys/js-sys first and use that?

Probably, but our story for WindowOrGlobalWorker is pretty weak, so I think it's better to leave it like that.

[daxpedda]

Would like to figure out first how we could cache queueMicrotask availability first, but otherwise LGTM.

Would you mind adding a note to the changelog as well?

[Liamolucko]

• i tried to run the tests mentioned here: https://rustwasm.github.io/docs/wasm-bindgen/contributing/testing.html but
  the first one failed (regardless of my changes) and cargo test -p ui-tests also did not work (the remaining tests worked, as well as the ones directly in wasm-bindgen-futures

The contributor experience is awful in this regard and should be improved/fixed.

The specific two problems there are that the main wasm32-unknown-unknown tests need you to to specify WASM_BINDGEN_SPLIT_LINKED_MODULES=1, as there's one test that relies on --split-linked-modules being enabled, and that the UI tests need to be run with a specific version of Rust since error messages change between versions (currently v1.69.0).

The WASM_BINDGEN_SPLIT_LINKED_MODULES one is actually quite annoying; it might be a good idea to add that to .cargo/config so that you don't have to specify it every time.

But yeah, those should both be either documented or fixed.

@Liamolucko what do you think of adding once_cell to the dependency tree of wasm-bindgen-futures, personally I'm fine with it.

I'm fine with that too.

[flumm]

We could use OnceLock, but v1.70 seems high right now and we don't have a proper MSRV policy. Alternatively we could use once_cell, but that would be adding another dependency.

@Liamolucko what do you think of adding once_cell to the dependency tree of wasm-bindgen-futures, personally I'm fine with it.

maybe i'm missing part of the puzzle, but wouldn't it be enough to try executing it once (with e.g. an empty closure) in new() and save the result of .is_err() of that into the queue as struct member?

then we could do:

if <...> && (self.needs_fallback || queueMicrotask(...)) {

?
then it's a simple boolean check without OnceLock or once_cell ?
or how did you mean to use them?

Would like to figure out first how we could cache queueMicrotask availability first, but otherwise LGTM.

Would you mind adding a note to the changelog as well?

sure no problem, do you prefer i push another commit, or should i force push my branch?
(what is preferred in general? AFAIR github mangles reviews when force pushing, but adding commits can be cumbersome when a pull request has many changes? Sorry for the stupid questions, but I'm used to
mailing list workflow, and seldom do github pull requests)

The specific two problems there are that the main wasm32-unknown-unknown tests need you to to specify WASM_BINDGEN_SPLIT_LINKED_MODULES=1, as there's one test that relies on --split-linked-modules being enabled, and that the UI tests need to be run with a specific version of Rust since error messages change between versions (currently v1.69.0).

The WASM_BINDGEN_SPLIT_LINKED_MODULES one is actually quite annoying; it might be a good idea to add that to .cargo/config so that you don't have to specify it every time.

But yeah, those should both be either documented or fixed.

thanks that worked for the first test, but running cargo test -p ui-tests results here (rustc 1.69.0) with

error: package ID specification ui-tests did not match any packages

[daxpedda]

We could use OnceLock, but v1.70 seems high right now and we don't have a proper MSRV policy. Alternatively we could use once_cell, but that would be adding another dependency.
@Liamolucko what do you think of adding once_cell to the dependency tree of wasm-bindgen-futures, personally I'm fine with it.

maybe i'm missing part of the puzzle, but wouldn't it be enough to try executing it once (with e.g. an empty closure) in new() and save the result of .is_err() of that into the queue as struct member?

then we could do:

if <...> && (self.needs_fallback || queueMicrotask(...)) {

? then it's a simple boolean check without OnceLock or once_cell ?
or how did you mean to use them?

That would be better indeed!

Instead of calling queueMicrotask() with an empty closure, you should probably be able to do:

#[wasm_bindgen]
extern "C" {
    #[wasm_bindgen(getter)]
    fn queueMicrotask() -> JsValue;
}

Which returns undefined if not present. Never tried it with global, but should work just fine. Then we should also be able to remove the catch from the actualy queueMicrotask() bindings, which AFAIK can't actually throw.

thanks that worked for the first test, but running cargo test -p ui-tests results here (rustc 1.69.0) with

I'm assuming this needs to be called from inside some package.
Documentation has to be improved in this regard.

[flumm]

That would be better indeed!

Instead of calling queueMicrotask() with an empty closure, you should probably be able to do:

#[wasm_bindgen]
extern "C" {
    #[wasm_bindgen(getter)]
    fn queueMicrotask() -> JsValue;
}

Which returns undefined if not present. Never tried it with global, but should work just fine. Then we should also be able to remove the catch from the actualy queueMicrotask() bindings, which AFAIK can't actually throw.

it seems that does not work, AFAICS the 'getter' property in the macro is ignored for non struct members

possible alternatives i saw were using the Window::get but i guess that does not work for workers anymore (and i couldn't find an alternative for workers...)

or using something like:

#[wasm_bindgen(inline_js = "export function queueMicrotaskExists() { return typeof queueMicrotask === 'function' }")]
extern "C" {
    fn queueMicrotaskExists() -> bool;
}

but that seems also overly complicated...

(also you did not answer my question about the pull request update, should i force push or add commits?)

Thanks!

[daxpedda]

possible alternatives i saw were using the Window::get but i guess that does not work for workers anymore (and i couldn't find an alternative for workers...)

We would want to avoid that because of the overhead from the string.
The solution is probably something like this:

#[wasm_bindgen]
extern "C" {
	type Global;

	#[wasm_bindgen(method, getter, js_name = queueMicrotask)]
	fn hasQueueMicrotask(this: &Global) -> JsValue;
}

let global: Global = js_sys::global().unchecked_into();
let has_queue_microtask = global.hasQueueMicrotask();

(also you did not answer my question about the pull request update, should i force push or add commits?)

Apologies!
The PR will be squashed before merging anyway, so you can do w/e you like. Personally I only need a "nice" commit history if it makes reviewing easier, which for such a small PR is irrelevant to me.

[flumm]

ah thanks, the 'Global' part was what i was missing, big thanks :)

i'll update my branch shortly

[daxpedda]

Thank you!