From c6f71d80f457e6a4c1b26a93a5f99450b9c930a2 Mon Sep 17 00:00:00 2001 From: Yin Jifeng Date: Tue, 10 Jan 2023 18:23:21 +0800 Subject: [PATCH 1/4] fix: `clearTimeout` illegal invocation in browser (#187) --- crates/timers/src/callback.rs | 48 ++++++++++++++++++++--------------- 1 file changed, 28 insertions(+), 20 deletions(-) diff --git a/crates/timers/src/callback.rs b/crates/timers/src/callback.rs index fbc89bd6..ada99c13 100644 --- a/crates/timers/src/callback.rs +++ b/crates/timers/src/callback.rs @@ -6,17 +6,19 @@ use wasm_bindgen::{JsCast, JsValue}; #[wasm_bindgen] extern "C" { - #[wasm_bindgen(js_name = "setTimeout", catch)] - fn set_timeout(handler: &Function, timeout: i32) -> Result; + type GlobalThis; - #[wasm_bindgen(js_name = "setInterval", catch)] - fn set_interval(handler: &Function, timeout: i32) -> Result; + #[wasm_bindgen(method, js_name = "setTimeout", catch)] + fn set_timeout(this: &GlobalThis, handler: &Function, timeout: i32) -> Result; - #[wasm_bindgen(js_name = "clearTimeout")] - fn clear_timeout(handle: i32); + #[wasm_bindgen(method, js_name = "setInterval", catch)] + fn set_interval(this: &GlobalThis, handler: &Function, timeout: i32) -> Result; - #[wasm_bindgen(js_name = "clearInterval")] - fn clear_interval(handle: i32); + #[wasm_bindgen(method, js_name = "clearTimeout")] + fn clear_timeout(this: &GlobalThis, handle: i32); + + #[wasm_bindgen(method, js_name = "clearInterval")] + fn clear_interval(this: &GlobalThis, handle: i32); } /// A scheduled timeout. @@ -37,7 +39,8 @@ impl Drop for Timeout { /// `clearTimeout` directly. fn drop(&mut self) { if let Some(id) = self.id { - clear_timeout(id); + let global = js_sys::global().unchecked_into::(); + global.clear_timeout(id); } } } @@ -61,11 +64,13 @@ impl Timeout { { let closure = Closure::once(callback); - let id = set_timeout( - closure.as_ref().unchecked_ref::(), - millis as i32, - ) - .unwrap_throw(); + let global = js_sys::global().unchecked_into::(); + let id = global + .set_timeout( + closure.as_ref().unchecked_ref::(), + millis as i32, + ) + .unwrap_throw(); Timeout { id: Some(id), @@ -139,7 +144,8 @@ impl Drop for Interval { /// `clearInterval` directly. fn drop(&mut self) { if let Some(id) = self.id { - clear_interval(id); + let global = js_sys::global().unchecked_into::(); + global.clear_interval(id); } } } @@ -162,11 +168,13 @@ impl Interval { { let closure = Closure::wrap(Box::new(callback) as Box); - let id = set_interval( - closure.as_ref().unchecked_ref::(), - millis as i32, - ) - .unwrap_throw(); + let global = js_sys::global().unchecked_into::(); + let id = global + .set_interval( + closure.as_ref().unchecked_ref::(), + millis as i32, + ) + .unwrap_throw(); Interval { id: Some(id), From 2857a77ea673acf789e6c8b07d3dbf23b68982ac Mon Sep 17 00:00:00 2001 From: Yin Jifeng Date: Thu, 19 Jan 2023 15:51:45 +0800 Subject: [PATCH 2/4] fix: return type of `Timeout/Interval` accepts both number and `NodeJS.Timeout` --- crates/timers/src/callback.rs | 25 +++++--- crates/timers/tests/node.rs | 114 ++++++++++++++++++++++++++++++++++ crates/timers/tests/web.rs | 4 +- 3 files changed, 130 insertions(+), 13 deletions(-) create mode 100644 crates/timers/tests/node.rs diff --git a/crates/timers/src/callback.rs b/crates/timers/src/callback.rs index ada99c13..983170c3 100644 --- a/crates/timers/src/callback.rs +++ b/crates/timers/src/callback.rs @@ -9,16 +9,21 @@ extern "C" { type GlobalThis; #[wasm_bindgen(method, js_name = "setTimeout", catch)] - fn set_timeout(this: &GlobalThis, handler: &Function, timeout: i32) -> Result; + fn set_timeout(this: &GlobalThis, handler: &Function, timeout: i32) + -> Result; #[wasm_bindgen(method, js_name = "setInterval", catch)] - fn set_interval(this: &GlobalThis, handler: &Function, timeout: i32) -> Result; + fn set_interval( + this: &GlobalThis, + handler: &Function, + timeout: i32, + ) -> Result; #[wasm_bindgen(method, js_name = "clearTimeout")] - fn clear_timeout(this: &GlobalThis, handle: i32); + fn clear_timeout(this: &GlobalThis, handle: JsValue); #[wasm_bindgen(method, js_name = "clearInterval")] - fn clear_interval(this: &GlobalThis, handle: i32); + fn clear_interval(this: &GlobalThis, handle: JsValue); } /// A scheduled timeout. @@ -30,7 +35,7 @@ extern "C" { #[derive(Debug)] #[must_use = "timeouts cancel on drop; either call `forget` or `drop` explicitly"] pub struct Timeout { - id: Option, + id: Option, closure: Option>, } @@ -38,7 +43,7 @@ impl Drop for Timeout { /// Disposes of the timeout, dually cancelling this timeout by calling /// `clearTimeout` directly. fn drop(&mut self) { - if let Some(id) = self.id { + if let Some(id) = self.id.take() { let global = js_sys::global().unchecked_into::(); global.clear_timeout(id); } @@ -95,7 +100,7 @@ impl Timeout { /// // Do stuff... /// }).forget(); /// ``` - pub fn forget(mut self) -> i32 { + pub fn forget(mut self) -> JsValue { let id = self.id.take().unwrap_throw(); self.closure.take().unwrap_throw().forget(); id @@ -135,7 +140,7 @@ impl Timeout { #[derive(Debug)] #[must_use = "intervals cancel on drop; either call `forget` or `drop` explicitly"] pub struct Interval { - id: Option, + id: Option, closure: Option>, } @@ -143,7 +148,7 @@ impl Drop for Interval { /// Disposes of the interval, dually cancelling this interval by calling /// `clearInterval` directly. fn drop(&mut self) { - if let Some(id) = self.id { + if let Some(id) = self.id.take() { let global = js_sys::global().unchecked_into::(); global.clear_interval(id); } @@ -198,7 +203,7 @@ impl Interval { /// // Do stuff... /// }).forget(); /// ``` - pub fn forget(mut self) -> i32 { + pub fn forget(mut self) -> JsValue { let id = self.id.take().unwrap_throw(); self.closure.take().unwrap_throw().forget(); id diff --git a/crates/timers/tests/node.rs b/crates/timers/tests/node.rs new file mode 100644 index 00000000..1c18353a --- /dev/null +++ b/crates/timers/tests/node.rs @@ -0,0 +1,114 @@ +#![cfg(all(target_family = "wasm", feature = "futures"))] + +use futures_channel::{mpsc, oneshot}; +use futures_util::{ + future::{select, Either, FutureExt}, + stream::StreamExt, +}; +use gloo_timers::{ + callback::{Interval, Timeout}, + future::{sleep, IntervalStream, TimeoutFuture}, +}; +use std::cell::Cell; +use std::rc::Rc; +use std::time::Duration; +use wasm_bindgen_test::*; + +#[wasm_bindgen_test] +async fn timeout() { + let (sender, receiver) = oneshot::channel(); + Timeout::new(1, || sender.send(()).unwrap()).forget(); + receiver.await.unwrap(); +} + +#[wasm_bindgen_test] +async fn timeout_cancel() { + let cell = Rc::new(Cell::new(false)); + + let t = Timeout::new(1, { + let cell = cell.clone(); + move || { + cell.set(true); + panic!("should have been cancelled"); + } + }); + t.cancel(); + + let (sender, receiver) = oneshot::channel(); + + Timeout::new(2, move || { + sender.send(()).unwrap(); + assert_eq!(cell.get(), false); + }) + .forget(); + + receiver.await.unwrap(); +} + +#[wasm_bindgen_test] +async fn timeout_future() { + TimeoutFuture::new(1).await; +} + +#[wasm_bindgen_test] +async fn timeout_future_cancel() { + let cell = Rc::new(Cell::new(false)); + + let a = TimeoutFuture::new(1).map({ + let cell = cell.clone(); + move |_| { + assert_eq!(cell.get(), false); + 1 + } + }); + + let b = TimeoutFuture::new(2).map({ + let cell = cell.clone(); + move |_| { + cell.set(true); + 2u32 + } + }); + + let (who, other) = match select(a, b).await { + Either::Left(x) => x, + Either::Right(_) => panic!("Timer for 2 ms finished before timer for 1 ms"), + }; + assert_eq!(who, 1); + // Drop `b` so that its timer is canceled. + drop(other); + TimeoutFuture::new(3).await; + // We should never have fired `b`'s timer. + assert_eq!(cell.get(), false); +} + +#[wasm_bindgen_test] +async fn interval() { + let (mut sender, receiver) = mpsc::channel(1); + let i = Interval::new(1, move || { + if !sender.is_closed() { + sender.try_send(()).unwrap() + } + }); + + let results: Vec<_> = receiver.take(5).collect().await; + drop(i); + assert_eq!(results.len(), 5); +} + +#[wasm_bindgen_test] +async fn interval_cancel() { + let i = Interval::new(10, move || { + panic!("This should never be called"); + }); + i.cancel(); + + // This keeps us live for long enough that if any erroneous Interval callbacks fired, we'll have seen them. + sleep(Duration::from_millis(100)).await; +} + +#[wasm_bindgen_test] +async fn interval_stream() { + let results: Vec<_> = IntervalStream::new(1).take(5).collect().await; + assert_eq!(results.len(), 5); +} diff --git a/crates/timers/tests/web.rs b/crates/timers/tests/web.rs index 8349310b..588b7f32 100644 --- a/crates/timers/tests/web.rs +++ b/crates/timers/tests/web.rs @@ -1,6 +1,4 @@ -//! Test suite for the Web and headless browsers. - -#![cfg(all(target_arch = "wasm32", feature = "futures"))] +#![cfg(all(target_family = "wasm", feature = "futures"))] use futures_channel::{mpsc, oneshot}; use futures_util::{ From 74a5769b5e6def8fdcc11179378a1afcd1bacb7a Mon Sep 17 00:00:00 2001 From: Yin Jifeng Date: Thu, 19 Jan 2023 17:20:34 +0800 Subject: [PATCH 3/4] chore: simplify clearTimeout/clearInterval binding refer https://github.com/rustwasm/gloo/pull/283#issuecomment-1396668213 for background --- crates/timers/src/callback.rs | 53 +++++++++++++---------------------- 1 file changed, 20 insertions(+), 33 deletions(-) diff --git a/crates/timers/src/callback.rs b/crates/timers/src/callback.rs index 983170c3..fab18785 100644 --- a/crates/timers/src/callback.rs +++ b/crates/timers/src/callback.rs @@ -6,24 +6,17 @@ use wasm_bindgen::{JsCast, JsValue}; #[wasm_bindgen] extern "C" { - type GlobalThis; + #[wasm_bindgen(js_name = "setTimeout", catch)] + fn set_timeout(handler: &Function, timeout: i32) -> Result; - #[wasm_bindgen(method, js_name = "setTimeout", catch)] - fn set_timeout(this: &GlobalThis, handler: &Function, timeout: i32) - -> Result; + #[wasm_bindgen(js_name = "setInterval", catch)] + fn set_interval(handler: &Function, timeout: i32) -> Result; - #[wasm_bindgen(method, js_name = "setInterval", catch)] - fn set_interval( - this: &GlobalThis, - handler: &Function, - timeout: i32, - ) -> Result; + #[wasm_bindgen(js_name = "clearTimeout")] + fn clear_timeout(handle: JsValue) -> JsValue; - #[wasm_bindgen(method, js_name = "clearTimeout")] - fn clear_timeout(this: &GlobalThis, handle: JsValue); - - #[wasm_bindgen(method, js_name = "clearInterval")] - fn clear_interval(this: &GlobalThis, handle: JsValue); + #[wasm_bindgen(js_name = "clearInterval")] + fn clear_interval(handle: JsValue) -> JsValue; } /// A scheduled timeout. @@ -44,8 +37,7 @@ impl Drop for Timeout { /// `clearTimeout` directly. fn drop(&mut self) { if let Some(id) = self.id.take() { - let global = js_sys::global().unchecked_into::(); - global.clear_timeout(id); + clear_timeout(id); } } } @@ -69,13 +61,11 @@ impl Timeout { { let closure = Closure::once(callback); - let global = js_sys::global().unchecked_into::(); - let id = global - .set_timeout( - closure.as_ref().unchecked_ref::(), - millis as i32, - ) - .unwrap_throw(); + let id = set_timeout( + closure.as_ref().unchecked_ref::(), + millis as i32, + ) + .unwrap_throw(); Timeout { id: Some(id), @@ -149,8 +139,7 @@ impl Drop for Interval { /// `clearInterval` directly. fn drop(&mut self) { if let Some(id) = self.id.take() { - let global = js_sys::global().unchecked_into::(); - global.clear_interval(id); + clear_interval(id); } } } @@ -173,13 +162,11 @@ impl Interval { { let closure = Closure::wrap(Box::new(callback) as Box); - let global = js_sys::global().unchecked_into::(); - let id = global - .set_interval( - closure.as_ref().unchecked_ref::(), - millis as i32, - ) - .unwrap_throw(); + let id = set_interval( + closure.as_ref().unchecked_ref::(), + millis as i32, + ) + .unwrap_throw(); Interval { id: Some(id), From acf9be67fbd16f5e94afe2c0536277567aab6866 Mon Sep 17 00:00:00 2001 From: Yin Jifeng Date: Fri, 20 Jan 2023 22:51:30 +0800 Subject: [PATCH 4/4] CI: setup node tests --- .github/workflows/tests.yml | 37 +++++++++++++++++++++++++++++++++++++ CONTRIBUTING.md | 9 +++++++++ 2 files changed, 46 insertions(+) diff --git a/.github/workflows/tests.yml b/.github/workflows/tests.yml index ecc6f2cc..f7fddcc1 100644 --- a/.github/workflows/tests.yml +++ b/.github/workflows/tests.yml @@ -76,6 +76,43 @@ jobs: wasm-pack test --headless --firefox --chrome crates/$x --no-default-features done + node_tests: + name: Node Tests + runs-on: ubuntu-latest + steps: + - uses: actions/checkout@v3 + - uses: actions-rs/toolchain@v1 + with: + toolchain: stable + target: wasm32-unknown-unknown + override: true + profile: minimal + + - name: Install wasm-pack + run: curl https://rustwasm.github.io/wasm-pack/installer/init.sh -sSf | sh + + - uses: actions/cache@v3 + with: + path: | + ~/.cargo/registry + ~/.cargo/git + target + key: cargo-${{ runner.os }}-node-tests-${{ hashFiles('**/Cargo.toml') }} + restore-keys: | + cargo-${{ runner.os }}-node-tests- + cargo-${{ runner.os }}- + + - name: Run tests + run: | + for x in $(ls crates); do + # gloo-net is tested separately + if [[ "$x" == "net" ]]; then + continue + fi + wasm-pack test --node crates/$x --all-features + wasm-pack test --node crates/$x --no-default-features + done + test-worker: name: Test gloo-worker runs-on: ubuntu-latest diff --git a/CONTRIBUTING.md b/CONTRIBUTING.md index 61fac3bc..92c9dcf2 100644 --- a/CONTRIBUTING.md +++ b/CONTRIBUTING.md @@ -17,6 +17,7 @@ yourself. - [Building](#building) - [Testing](#testing) - [Wasm Headless Browser Tests](#wasm-headless-browser-tests) + - [Wasm Node Tests](#wasm-node-tests) - [Non-Wasm Tests](#non-wasm-tests) - [Formatting](#formatting) - [Updating `README.md`s](#updating-readmemds) @@ -81,6 +82,14 @@ To run headless browser tests for a particular crate: wasm-pack test crates/my-particular-crate --headless --firefox # or --safari or --chrome ``` +#### Wasm Node Tests + +To run tests in Node.js: + +```shell +wasm-pack test crates/my-particular-crate --node +``` + #### Non-Wasm Tests You can run the non-Wasm tests (e.g. doc tests) for every Gloo crate with: