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(swarm): Add wasm-bindgen executor #3115

Merged
merged 53 commits into from Nov 24, 2022

Conversation

umgefahren
Copy link
Contributor

@umgefahren umgefahren commented Nov 13, 2022

Description

Add a wasm-bindgen executor to perform concurrent connections on browser or node background tasks.

Links to any relevant issues

Result of: #3097 (comment)

Open Questions

None

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

Commit message body

umgefahren and others added 30 commits November 8, 2022 17:46
Co-authored-by: Thomas Eizinger <thomas@eizinger.io>
Co-authored-by: Thomas Eizinger <thomas@eizinger.io>
Co-authored-by: Thomas Eizinger <thomas@eizinger.io>
Co-authored-by: Thomas Eizinger <thomas@eizinger.io>
Co-authored-by: Thomas Eizinger <thomas@eizinger.io>
Co-authored-by: Thomas Eizinger <thomas@eizinger.io>
Co-authored-by: Thomas Eizinger <thomas@eizinger.io>
Co-authored-by: Thomas Eizinger <thomas@eizinger.io>
@thomaseizinger thomaseizinger changed the title swarm: Add Wasm bindgen executor feat(swarm): Add wasm-bindgen executor Nov 14, 2022
@umgefahren umgefahren marked this pull request as ready for review November 16, 2022 12:15
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.

Thanks @umgefahren for the work here!

From my minimal WASM knowledge, this looks good to me.

@vincev curious what your thoughts are here given your example #2617 (comment).

@vincev
Copy link

vincev commented Nov 16, 2022

Thanks @umgefahren for the work here!

From my minimal WASM knowledge, this looks good to me.

@vincev curious what your thoughts are here given your example #2617 (comment).

@mxinden looks nice it makes it easier to create the Swarm I will update the example to use with_wasm_executor.

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.

Rustfmt is not happy, otherwise this LGTM!

@mergify
Copy link

mergify bot commented Nov 18, 2022

This pull request has merge conflicts. Could you please resolve them @umgefahren? 🙏

swarm/src/executor.rs Outdated Show resolved Hide resolved
@mxinden
Copy link
Member

mxinden commented Nov 23, 2022

After merging #3083 which touched the mergify configuration, Mergify is complaining that "The current Mergify configuration is invalid", see also https://github.com/libp2p/rust-libp2p/pull/3115/checks?check_run_id=9673900404.

warning The subscription needs to be updated to enable this feature.

Unfortunately I don't have access to the libp2p Mergify subscription page, thus I am not sure what it entails to "update the subscribtion".

@galargh do you have access? Can you check what we would need to do to "update the subscription"?

//CC @thomaseizinger

@thomaseizinger
Copy link
Contributor

Oh my god :(

https://docs.mergify.com/actions/post_check/#enforcing-conventional-commits

post_check's are a premium feature! I completely overlooked that! Sorry about that.

I suggest we revert #3083 for now.

@thomaseizinger
Copy link
Contributor

Unfortunately I don't have access to the libp2p Mergify subscription page

You do have access to the dashboard though, right?

@mxinden
Copy link
Member

mxinden commented Nov 24, 2022

Unfortunately I don't have access to the libp2p Mergify subscription page

You do have access to the dashboard though, right?

I do. Is there some way that is helpful?

@mxinden
Copy link
Member

mxinden commented Nov 24, 2022

I suggest we revert #3083 for now.

👍 done via #3165.

@thomaseizinger
Copy link
Contributor

Unfortunately I don't have access to the libp2p Mergify subscription page

You do have access to the dashboard though, right?

I do. Is there some way that is helpful?

Only to inspect the current merge queue. I just wanted to make sure this isn't tied to my account only for some reason.

@mergify mergify bot merged commit cff84f1 into libp2p:master Nov 24, 2022
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.

None yet

4 participants