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

feat(websocket-websys): add support for different WASM environments #4889

Merged
merged 2 commits into from
Nov 21, 2023

Conversation

jsdanielh
Copy link
Contributor

@jsdanielh jsdanielh commented Nov 17, 2023

Description

We introduce a WebContext enum that abstracts and detects the Window vs the WorkerGlobalScope API.

Related: rustwasm/wasm-bindgen#1046.

Change checklist

  • I have performed a self-review of my own code
  • I have made corresponding changes to the documentation
  • I have added tests that prove my fix is effective or that my feature works
  • A changelog entry has been made in the appropriate crates

Copy link
Contributor

@sisou sisou left a comment

Choose a reason for hiding this comment

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

For some added context: this uses the same approach as for gloo-timers: rustwasm/gloo#185, extended already by the codegen fix of rustwasm/gloo#283.

This makes it possible to run this transport in environments that support setInterval, but don't have a window global. This way it also works in NodeJS, Deno and Bun (although Deno provides a window for compatibility).

@thomaseizinger thomaseizinger changed the title fix(websocket-websys): Add support for workers fix(websocket-websys): use global setInterval and clearInterval functions Nov 19, 2023
@thomaseizinger
Copy link
Contributor

For some added context: this uses the same approach as for gloo-timers: rustwasm/gloo#185, extended already by the codegen fix of rustwasm/gloo#283.

This makes it possible to run this transport in environments that support setInterval, but don't have a window global. This way it also works in NodeJS, Deno and Bun (although Deno provides a window for compatibility).

I've updated the PR description to reflect what I think is what is happening here, let me know if that is correct.

Copy link
Contributor

@thomaseizinger thomaseizinger left a comment

Choose a reason for hiding this comment

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

Thanks!

I am not entirely opposed to this change but it isn't particularly clean. The idea of libp2p-websocket-websys was that we provide WASM bindings via web-sys.

We are now working around the web-sys crate which is a bit odd. Would it make sense to perhaps upstream this change to web-sys instead by offering setInterval and clearInterval as top-level functions instead of always going through window?

transports/websocket-websys/CHANGELOG.md Outdated Show resolved Hide resolved
@sisou
Copy link
Contributor

sisou commented Nov 19, 2023

Unfortunately, there was no interest in web-sys last time this was brought up, and not since: rustwasm/wasm-bindgen#1046. As you can see at the bottom of that PR, lots of people had to implement this themselves already.

Another method to do it is with an enum over window vs. worker, like this: Globidev/reqwest@9898e62

A third way is unchecked-casting web_sys::global() to whichever env is detected. This might be the best solution using web-sys: https://github.com/gfx-rs/wgpu/pull/2858/files

@thomaseizinger
Copy link
Contributor

Another method to do it is with an enum over window vs. worker, like this: Globidev/reqwest@9898e62

I think I'd prefer this. It is unfortunate that web-sys doesn't provide something out of the box here but at least, it makes it obvious what we are supporting here instead of binding to global functions.

Mind updating the PR and the description to explain the solution?

@jsdanielh jsdanielh changed the title fix(websocket-websys): use global setInterval and clearInterval functions fix(websocket-websys): Add support for different WASM environments Nov 19, 2023
@jsdanielh
Copy link
Contributor Author

Another method to do it is with an enum over window vs. worker, like this: Globidev/reqwest@9898e62

I think I'd prefer this. It is unfortunate that web-sys doesn't provide something out of the box here but at least, it makes it obvious what we are supporting here instead of binding to global functions.

Mind updating the PR and the description to explain the solution?

Sure. Done. And also implemented the requested changes in the review.

@jsdanielh
Copy link
Contributor Author

Another method to do it is with an enum over window vs. worker, like this: Globidev/reqwest@9898e62

I think I'd prefer this. It is unfortunate that web-sys doesn't provide something out of the box here but at least, it makes it obvious what we are supporting here instead of binding to global functions.
Mind updating the PR and the description to explain the solution?

Sure. Done. And also implemented the requested changes in the review.

Ah wait, I thought this was referring to the current implementation but now that I read it again with the quote I think you meant this. In this case, I'll update the PR with this.

@thomaseizinger
Copy link
Contributor

Another method to do it is with an enum over window vs. worker, like this: Globidev/reqwest@9898e62

I think I'd prefer this. It is unfortunate that web-sys doesn't provide something out of the box here but at least, it makes it obvious what we are supporting here instead of binding to global functions.
Mind updating the PR and the description to explain the solution?

Sure. Done. And also implemented the requested changes in the review.

Ah wait, I thought this was referring to the current implementation but now that I read it again with the quote I think you meant this. In this case, I'll update the PR with this.

Yeah I was referring to an implementation that uses an enum and detects the context accordingly! :)

@jsdanielh
Copy link
Contributor Author

Another method to do it is with an enum over window vs. worker, like this: Globidev/reqwest@9898e62

I think I'd prefer this. It is unfortunate that web-sys doesn't provide something out of the box here but at least, it makes it obvious what we are supporting here instead of binding to global functions.
Mind updating the PR and the description to explain the solution?

Sure. Done. And also implemented the requested changes in the review.

Ah wait, I thought this was referring to the current implementation but now that I read it again with the quote I think you meant this. In this case, I'll update the PR with this.

Yeah I was referring to an implementation that uses an enum and detects the context accordingly! :)

Ok, now it's updated with this implementation. Let me know if you have more comments.

@thomaseizinger thomaseizinger changed the title fix(websocket-websys): Add support for different WASM environments feat(websocket-websys): add support for different WASM environments Nov 20, 2023
Copy link
Contributor

@thomaseizinger thomaseizinger left a comment

Choose a reason for hiding this comment

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

Thank you!

Overall concept looks great, just a few nits :)

transports/websocket-websys/CHANGELOG.md Outdated Show resolved Hide resolved
transports/websocket-websys/src/web_context.rs Outdated Show resolved Hide resolved
transports/websocket-websys/src/web_context.rs Outdated Show resolved Hide resolved
transports/websocket-websys/src/web_context.rs Outdated Show resolved Hide resolved
transports/websocket-websys/src/lib.rs Outdated Show resolved Hide resolved
transports/websocket-websys/src/lib.rs Outdated Show resolved Hide resolved
Add support different WASM environments (such as workers, NodeJS)
that don't have a `window` global. This is done by introducing a
`WebContext` `enum` that abstracts and detects the `Window` vs the
`WorkerGlobalScope` API.
This is done due to the `web-sys` lack of interest to support this
(see discussion in
[this issue](rustwasm/wasm-bindgen#1046)).
Copy link
Contributor

@thomaseizinger thomaseizinger left a comment

Choose a reason for hiding this comment

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

Great work!

Thank you for iterating on this with me :)

@thomaseizinger
Copy link
Contributor

We don't have any automated tests for web workers, mind testing this PR manually on your end before we merge it?

@jsdanielh
Copy link
Contributor Author

We don't have any automated tests for web workers, mind testing this PR manually on your end before we merge it?

Already did for web workers :)

@mergify mergify bot merged commit 5a4a462 into libp2p:master Nov 21, 2023
71 checks passed
@jsdanielh jsdanielh deleted the pr branch November 21, 2023 00:06
jsdanielh added a commit to nimiq/core-rs-albatross that referenced this pull request Dec 12, 2023
Go back to upstream libp2p. With the release of `libp2p` 0.53.2, the
`websocket-websys` transport was upgraded to `0.3.1` which also ships
[this fix](libp2p/rust-libp2p#4889) that
forced us to fork `libp2p` temporarily.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants