fix: clearTimeout illegal invocation with bundler (#187)

[flisky]

It's a regression from #96 introduced by #185, and I have no idea why calling clearTimeout is failed in browser, but my patch definitely resolves the error.

It's a side effect introduced by bundler, and we workaround it. more details in #283 (comment)

[futursolo]

Interestingly, the current timer implementation (in master) does not seem to work with wasm-pack test --node either when I tried it. Maybe this will also fix it?

CI should turn green when if you rebase from the master branch.

[DogLooksGood]

FYI, ran into this with https://github.com/paritytech/jsonrpsee. What weird is, current version (gloo-timers 0.2.5) only works in tests, not in real cases. In wasm-pack test --chrome it works (when I open the browser to run tests). But if the it is compiled to wasm (with wasm-pack) and required by JavaScript, there's an illegal invocation error.

[flisky]

@futursolo good catch. It turns out the return type is different between browser (number) / nodejs (Timeout), and I push another commit for nodejs support.

[flisky]

@DogLooksGood @futursolo I take a deep look at ctm/gloo-timers-regression(thanks @ctm), and reproduce as the following code:

const foo = { bar: clearTimeout }
foo.bar()

It's a clearly true invocation error because clearTimeout is binding this to foo, instead of Window.

---

And setTimeout is not affected because the wasm-pack generates the following code:

function() { return handleError(function (arg0, arg1) {
    const ret = setTimeout(getObject(arg0), arg1);
    _assertNum(ret);
    return ret;
}, arguments) };

while clearTimeout goes to typeof clearTimeout == 'function' ? clearTimeout : notDefined('clearTimeout')

So, it's a bad side-effect introduced by bundler.

---

The currently clearTimeout goes to

function() { return logError(function (arg0, arg1) {
   getObject(arg0).clearTimeout(takeObject(arg1));
}, arguments) };

I think we can hack the binding further into

#[wasm_bindgen(js_name = "clearTimeout")]
fn clear_timeout(handle: JsValue) -> JsValue;

which generates

 function() { return logError(function (arg0) {
    const ret = clearTimeout(takeObject(arg0));
    return addHeapObject(ret);
}, arguments) }

[DogLooksGood]

@flisky I just tried with your master version, the issue hasn't solved in my case. The generated code is still

export const __wbg_clearTimeout_23ee6db72c0ad922 = typeof clearTimeout == 'function' ? clearTimeout : notDefined('clearTimeout');

[flisky]

@DogLooksGood, could you please double check? Make sure that:

• something like git+https://github.com/flisky/gloo.git#74a5769b5e6def8fdcc11179378a1afcd1bacb7a in Cargo.lock
• disable cache in devtools when reproducing

[flisky]

@futursolo, could you please take another review?

There're two binding changes:

• the return type of setTimeout/setInterval from i32 to JsValue to support nodejs, which returns Timeout object.
• add return type of clearTimeout/clearInterval to let wasm-bindgen wrapping the generated code, to avoid a bare function call in a different namespace.

I don't know how to run the same testcases in nodejs & browser with wasm-bindgen test, so I duplicates web.rs into node.rs. Please let me known if there's a better way. Thanks!

[futursolo]

The implementation looks good to me. I will approve this once I hear back from @DogLooksGood.

I don't know how to run the same testcases in nodejs & browser with wasm-bindgen test, so I duplicates web.rs into node.rs. Please let me known if there's a better way. Thanks!

You can apply a feature flag to run_in_browser.

// It might be better to use not(node_tests) than browser_tests because most existing tests are browser tests.

#[cfg(not(feature = "node_tests"))]
wasm_bindgen_test_configure!(run_in_browser);

// Runs browser tests
wasm-pack test --firefox

// Runs node tests
wasm-pack test --features=node_tests --node

In addition, would you mind to also update GitHub Actions to run node tests?

[DogLooksGood]

@futursolo @flisky Sorry, it's my fault. It works.

[flisky]

@futursolo the current CI is heavily based on --all-features, and I don't think web_tests/node_tests works perfectly with other feature flags.

Tests / Node Tests job is setup, and works as expected

[futursolo]

Whilst I think it's still possible to combine tests with a compiler flag, I am fine with merging this pull request as-is.

We can merge the tests together in the future.

[ranile]

Released in gloo-timers 0.2.6 🎉