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

Fix timers in web workers #56

Merged
merged 2 commits into from
Mar 29, 2019

Conversation

samcday
Copy link
Contributor

@samcday samcday commented Mar 28, 2019

@fitzgen / @OddCoincidence

gloo-timers doesn't work in web workers at all right now, because web_sys::window() wants to cast the global namespace into a Window object, which doesn't exist in worker contexts. So I just dropped us back down to raw #[wasm_bindgen] methods.

This might be something better fixed in wasm-bindgen later though. There does exist this thing: WindowOrWorkerGlobalScope which is a nice view over what global methods are available in both Window and Worker contexts. However as stated in that documentation:

Note: WindowOrWorkerGlobalScope is a mixin and not an interface; you can't actually create an object of type WindowOrWorkerGlobalScope.

So I guess this thing is probably not mentioned anywhere in the IDLs that are being used to build the web-sys crate.

Also, the Interval stuff wasn't actually working properly at all, it was consuming the callback from the Option which meant only the first interval callback worked and all subsequent ones failed. See #57

@OddCoincidence
Copy link
Contributor

OddCoincidence commented Mar 28, 2019

Also, the Interval stuff wasn't actually working properly at all, it was consuming the callback from the Option which meant only the first interval callback worked and all subsequent ones failed.

Sorry about this! That's a copy-paste error from timeout (from before it was using Closure::once). I'm surprised / concerned that a test didn't catch this.

Maybe these two issues should be split into separate PRs though since they aren't related?

@samcday
Copy link
Contributor Author

samcday commented Mar 28, 2019

Sorry about this! That's a copy-paste error from timeout (from before it was using Closure::once).

Apology accepted. Don't let it ever happen again. Now go to your room and put away your clean washing. I want to see the socks FOLDED this time.

I'm surprised / concerned that a test didn't catch this.

Yeah interesting you should say that. I haven't looked at the test suite much but I have some concerns with it too. Namely when I made these changes I ran the test suite and everything passed. But then for the lolz I changed the js_name = setTimeout to js_name = spaghetti and ... everything still passed. You might wanna look into that more :)

Maybe these two issues should be split into separate PRs though since they aren't related?

Sure - I was just being lazy ;) I'll split the interval fix out now.

@samcday
Copy link
Contributor Author

samcday commented Mar 28, 2019

But then for the lolz I changed the js_name = setTimeout to js_name = spaghetti and ... everything still passed. You might wanna look into that more :)

Actually no ... ignore me, I was only running cargo test and not the web tests. I'm actually not sure how to run the integration tests, this is what I get:

sam@SamsPC:~/src/gloo/crates/timers$ wasm-pack test --firefox --headless
  
  [1/6] Checking `rustc` version...
  [2/6] Adding WASM target...
  info: component 'rust-std' for target 'wasm32-unknown-unknown' is up to date
  [3/6] Compiling tests to WASM...
     Compiling gloo-timers v0.1.0 (/home/sam/src/gloo/crates/timers)
      Finished dev [unoptimized + debuginfo] target(s) in 0.19s
  [4/6] Installing wasm-bindgen...
  [5/6] Getting geckodriver...
  [6/6] Running tests in Firefox...
      Finished dev [unoptimized + debuginfo] target(s) in 0.07s
       Running /home/sam/src/gloo/target/wasm32-unknown-unknown/debug/deps/gloo_timers-723cf89fd2565a10.wasm
  no tests to run!
       Running /home/sam/src/gloo/target/wasm32-unknown-unknown/debug/deps/web-fbd694454b48e118.wasm
no tests to run!                                    
| 
no tests to run!
no tests to run! 

@OddCoincidence
Copy link
Contributor

Ah, I think this is because the integration tests are gated on the recently-added futures feature. I think wasm-pack test --chrome --headless -- --features futures ought to do the trick (everything after the -- should be passed to cargo test).

@samcday
Copy link
Contributor Author

samcday commented Mar 28, 2019

wasm-pack test --chrome --headless -- --features futures didn't work either:

sam@SamsPC:~/src/gloo/crates/timers$ wasm-pack test --firefox --headless -- --features futures
error: Found argument 'futures' which wasn't expected, or isn't valid in this context

USAGE:
    wasm-pack test --firefox --headless

For more information try --help

@OddCoincidence
Copy link
Contributor

What's your version of wasm-pack? Passing flags to cargo test was added in 0.7

@samcday
Copy link
Contributor Author

samcday commented Mar 28, 2019

Ah good catch, I'm on 0.6 still.

@Pauan
Copy link
Contributor

Pauan commented Mar 28, 2019

gloo-timers doesn't work in web workers at all right now, because web_sys::window() wants to cast the global namespace into a Window object, which doesn't exist in worker contexts.

Relevant: rustwasm/wasm-bindgen#1046

@samcday samcday changed the title Fix timers in web workers + fix Interval Fix timers in web workers Mar 28, 2019
…d of the Window API, since window isn't always present (e.g Web Workers)
Copy link
Member

@fitzgen fitzgen left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks @samcday.

I think given that we don't have a nice way of representing WindowOrWorkerGlobalScope via web-sys (yet), writing the bindings by hand makes sense.

Just a nitpick below that needs to be addressed, and then I think we can land this.

crates/timers/src/lib.rs Outdated Show resolved Hide resolved
@fitzgen
Copy link
Member

fitzgen commented Mar 28, 2019

Regarding the headless browser tests: as of #54 they are running in CI, so we should be better protected going forward.

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

Successfully merging this pull request may close these issues.

4 participants