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

"Just works" WASM (browser) experience #2245

Closed
wants to merge 22 commits into from
Closed

Conversation

wngr
Copy link
Contributor

@wngr wngr commented Sep 22, 2021

These are the changes I needed to make in order to get libp2p to run via wasm32-unknown-unknown in the browser (both main thread and inside web workers).

Looking for comments, whether this is something that can get merged, or whether there are alternative approaches.

related #2134 #2166 #2143

@wngr wngr changed the title "Just works" WASM experience "Just works" WASM (browser) experience Sep 22, 2021
@wngr wngr marked this pull request as ready for review September 27, 2021 12:45
@mxinden
Copy link
Member

mxinden commented Sep 29, 2021

@wngr very very very much appreciated! I am sorry for the delay here. I will give this a review. I just need a bit more time.

Copy link
Member

@mxinden mxinden left a comment

Choose a reason for hiding this comment

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

Just a couple of comments for now. Let me know if you need help pinging folks for sebcrozet/instant#9.

Again, thanks for this contribution!

.github/workflows/ci.yml Outdated Show resolved Hide resolved
Cargo.toml Outdated Show resolved Hide resolved
Cargo.toml Outdated Show resolved Hide resolved
core/src/lib.rs Outdated Show resolved Hide resolved
@@ -80,6 +87,11 @@ jobs:
# TODO: ideally we would build `--workspace`, but not all crates compile for WASM
run: cargo build --target=wasm32-wasi

- name: Build on wasm32-wasi
# TODO: also run `cargo test`
Copy link
Member

Choose a reason for hiding this comment

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

Yes please, that would be great!

Copy link
Contributor Author

Choose a reason for hiding this comment

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

That's a bigger undertaking, which might be more geared towards wasm32-wasi rather than wasm32-unknown-unknown.
I guess for wasm32-unknown-unknown a wasm-bindgen based browser smoke test might be something, but I'll leave that to future-us :-)

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!

Some comments inline, nothing big :)

.github/workflows/ci.yml Outdated Show resolved Hide resolved
Cargo.toml Outdated Show resolved Hide resolved
core/src/lib.rs Outdated Show resolved Hide resolved
transports/wasm-ext/src/websockets.js Outdated Show resolved Hide resolved
Cargo.toml Outdated Show resolved Hide resolved
@wngr
Copy link
Contributor Author

wngr commented Oct 16, 2021

I think the test failure is unrelated to this PR.

@wngr
Copy link
Contributor Author

wngr commented Oct 18, 2021

Should be good to go for another 👀 @mxinden @thomaseizinger

@mxinden
Copy link
Member

mxinden commented Oct 18, 2021

I think the test failure is unrelated to this PR.

Yes. Fixed once #2295 is merged.

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!

Made a few suggestions but nothing blocking.

.github/workflows/ci.yml Outdated Show resolved Hide resolved
Cargo.toml Outdated Show resolved Hide resolved
protocols/gossipsub/src/interval.rs Outdated Show resolved Hide resolved
@PhilippGackstatter
Copy link
Contributor

Great PR! Getting rid of wasm-timer in favor of instant is much appreciated. Do these changes rely on any browser-exclusive APIs (haven't checked what futures-timer/wasm-bindgen binds to) or would this also work in node.js?

@wngr
Copy link
Contributor Author

wngr commented Oct 21, 2021

I think futures-timer(uses gloo-timers under the hood) indeed does not work inside nodejs, but I've never tried it.

@mxinden
Copy link
Member

mxinden commented Oct 22, 2021

Thanks for the pull request @wngr!

As you might have seen, we already cut a v0.40.0 release candidate, thus this pull request would be included in v0.41.0. In case you need a release quickly, I am happy to prepare v0.41.0 shortly after v0.40.0 is released.

@mxinden
Copy link
Member

mxinden commented Oct 30, 2021

Merged via #2320. Thanks @wngr for the work!

@mxinden mxinden closed this Oct 30, 2021
@wngr wngr deleted the wasm-support branch October 30, 2021 11:26
@wngr wngr restored the wasm-support branch October 30, 2021 11:26
@wngr
Copy link
Contributor Author

wngr commented Nov 12, 2021

Thanks for the pull request @wngr!

As you might have seen, we already cut a v0.40.0 release candidate, thus this pull request would be included in v0.41.0. In case you need a release quickly, I am happy to prepare v0.41.0 shortly after v0.40.0 is released.

Would appreciate if you could cut create a v0.41.0 soon-ish!

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