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

Use js_sys to enable node.js compatibility #18

Open
wants to merge 3 commits into
base: master
Choose a base branch
from

Conversation

PhilippGackstatter
Copy link

@PhilippGackstatter PhilippGackstatter commented Aug 4, 2021

This is motivated by us wanting to use libp2p in our Wasm bindings for web and node.js. This also follows from discussion in issue 2143 in rust-libp2p. The main idea for this comes from the instant crate.

I have tested the changes manually within our project and no problems came up. Is this something I should add automatic tests for? And if so, any hints on how to do that?

Thanks!

@PhilippGackstatter PhilippGackstatter marked this pull request as draft August 16, 2021 14:27
Co-authored-by: Thoralf-M <thoralf.mue@gmail.com>
@PhilippGackstatter PhilippGackstatter marked this pull request as ready for review August 17, 2021 15:42
@PhilippGackstatter
Copy link
Author

We had an issue with node.js where the process would repeatedly schedule_callbacks and never exit because of it. This issue is also present in the current master branch, so it is not related to the js_sys changes. If one adds a logging statement in that function and then runs a libp2p Wasm app in the browser, the console shows a never ending stream of log statements. This shouldn't happen, as the script has finshed execution, and all references to the objects should have been garbage collected.

In the browser this is not really noticeable, but it prevents node.js apps from exiting. @Thoralf-M came up with a fix, which still prolongs the actual execution, but it exits after some delay. An idea for a proper fix would be welcome, of course.

I marked the PR as ready for review, @tomaka or @mxinden - if you have time, we would appreciate it!

Copy link

@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.

Sorry for the delay here. I am not watching this repository.

I am not familiar enough with Rust's WASM ecosystem to give a proper review here. @tomaka (or maybe @expenses?) are the experts.

src/timer/global/wasm.rs Show resolved Hide resolved
Co-authored-by: Max Inden <mail@max-inden.de>
@mxinden
Copy link

mxinden commented Aug 23, 2021

Again, thanks for the work you are putting into this @PhilippGackstatter. Before we continue here, I would be curious what your input on libp2p/rust-libp2p#2143 (comment) would be.

@mxinden
Copy link

mxinden commented Oct 5, 2021

Crossreferencing related pull request here libp2p/rust-libp2p#2245

@Thoralf-M
Copy link

@mxinden the linked approach only works for browser, but not for node.js, so we can't use the same :(

@Thoralf-M
Copy link

@mxinden can we go forward with this PR or what's holding it up?

@mxinden
Copy link

mxinden commented Dec 15, 2021

@Thoralf-M libp2p/rust-libp2p#2245 removed wasm-timer from rust-libp2p. With that in mind, is this still needed?

@Thoralf-M
Copy link

We use it in our crate independent of libp2p

@mxinden
Copy link

mxinden commented Dec 15, 2021

I am sorry, but I am afraid I won't be able to help you here. I am not maintaining this library, nor do I have the capacity to do so in the future. You might want to consider switching to instant and futures-timer as well.

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.

None yet

3 participants