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

Merge ZeroMQ-NG back into ZeroMQ.js #343

Merged
merged 9 commits into from
Nov 3, 2019
Merged

Merge ZeroMQ-NG back into ZeroMQ.js #343

merged 9 commits into from
Nov 3, 2019

Conversation

rolftimmermans
Copy link
Member

@rolftimmermans rolftimmermans commented Oct 15, 2019

This is a continuation of #189.

This is the status of my rewrite, ZeroMQ-NG, which I'd like to get merged back into ZeroMQ.js:

  • Rewrite from the ground up with modern JavaScript, TypeScript, Node.js & ZeroMQ semantics in mind, exposing an API for async/await and async iterators, and using the binary stable N-API to integrate with Node.js.
  • Recreate an API that addresses fundamental issues with ZeroMQ.js as previously outlined in My work on a next-generation proof of concept for ZMQ bindings for Node.js #189.
  • Automate testing, building of prebuilt binaries which are included in the NPM package for all platforms currently supported by ZeroMQ.js. Targets Node 10+ and Electron 3+.
  • Implement a compatibility layer for existing versions 4.x and 5.x of ZeroMQ.js aimed at existing users.
  • Document the new public API surface, for use by IDEs such as VSCode and for consumption with TypeDoc (see: https://rolftimmermans.github.io/zeromq-ng/).
  • Implement DRAFT sockets which can be enabled with a compile time flag.
  • Tested in production.

Based on #189 (comment), the following needs to be done.

Edit: I have added a more complete list in #347, so that we can also track activities after the PR.

Regarding the version tags mentioned above, this PR tags the next version as 6.0.0-beta.1.

I'd like to invite everyone to browse, review, test and comment! You can browse the code in this pull request at https://github.com/rolftimmermans/zeromq.js/tree/upstream. If you wish to use the prebuilt binaries, it is available from NPM as [email protected].

@rolftimmermans
Copy link
Member Author

Just to be clear on this: the AppVeyor check is failing because there is no need for AppVeyor anymore. Travis will also test & build from source on Windows x64 & x86.

@rgbkrk
Copy link
Member

rgbkrk commented Oct 18, 2019

Ok great, we can just turn AppVeyor off for now.

Copy link
Member

@rgbkrk rgbkrk left a comment

Choose a reason for hiding this comment

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

We need the project to be able to move forward with this API, so I'm in favor of this plan. Thank you @rolftimmermans.

Copy link
Member

@lgeiger lgeiger left a comment

Choose a reason for hiding this comment

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

@rolftimmermans This is great! Thanks you so much for moving things forward.

The feedback in #189 was very positive and I am very much in favour of this plan. Especially the compatibility layer should make upgrading quite easy.

@lgeiger lgeiger mentioned this pull request Oct 20, 2019
@rolftimmermans
Copy link
Member Author

@lgeiger @rgbkrk Perfect!

Before we move forward and merge this PR, I have two questions:

  1. Are there any other maintainers we should request review/approval from before merging?

  2. To automatically build & deploy new prebuilt binaries for tags pushed to the repository we need to add an encrypted API key for Github & NPM. I have created the zeromqjs-integration user on Github & NPM for both purposes (instead of a personal account, to avoid access to any additional repos/packages). I don't have permissions to add it to the Github repo (with commit access in order to publish documentation to gh-pages) or to the NPM package, however. Do you agree with this approach, and if so could any of you take care of adding the integration users? I will set up the deployment keys once that is done.

@rolftimmermans
Copy link
Member Author

I have made a separate issue to track all activities (and added a few new ones, especially for after this PR lands). Right now I'm blocked by my two questions in my previous comment; would be great to get some feedback!

@rgbkrk
Copy link
Member

rgbkrk commented Oct 28, 2019

Are there any other maintainers we should request review/approval from before merging?

@reqshark @ronkorving @kkoopa might have some thoughts. I don't know what their bandwidth is these days.

I'm leaning towards helping you take on more of the reins @rolftimmermans and driving this forward. It works well and is the direction we need to take for future node versions.

@kkoopa
Copy link
Contributor

kkoopa commented Oct 28, 2019

I think this is great.

@ronkorving
Copy link

ronkorving commented Oct 29, 2019

Agreed, this is awesome! :)

(not a lot of bandwidth, but FWIW I love the API)

@rolftimmermans
Copy link
Member Author

I'm leaning towards helping you take on more of the reins @rolftimmermans and driving this forward. It works well and is the direction we need to take for future node versions.

I would love to push this forward further, with input from everyone that is able to provide it. In the short term I'm mostly blocked by not being able to add the integration users in Github & NPM to automatically build & release a beta to get some feedback from users.

@rgbkrk
Copy link
Member

rgbkrk commented Oct 31, 2019

To automatically build & deploy new prebuilt binaries for tags pushed to the repository we need to add an encrypted API key for Github & NPM. I have created the zeromqjs-integration user on Github & NPM for both purposes (instead of a personal account, to avoid access to any additional repos/packages). I don't have permissions to add it to the Github repo (with commit access in order to publish documentation to gh-pages) or to the NPM package, however. Do you agree with this approach, and if so could any of you take care of adding the integration users? I will set up the deployment keys once that is done.

@bluca or @somdoron -- do you know how to facilitate the above request?

@bluca
Copy link
Member

bluca commented Oct 31, 2019

If you need me to set a key, send it encrypted with gpg to [email protected] and I'll set it

@somdoron
Copy link
Member

I can give the new user a write permission on the repo if needed

@rolftimmermans
Copy link
Member Author

rolftimmermans commented Oct 31, 2019

I can give the new user a write permission on the repo if needed

That would be excellent. If zeromqjs-integration has repository write access I can integrate the build process of the binaries and documentation with Travis & Github. (Security-wise it would be equivalent with write access for me, but this account is single-purpose, and if preferred I can share or transfer the credentials with/to someone else.)

@lgeiger
Copy link
Member

lgeiger commented Nov 2, 2019

@rolftimmermans Sorry for the delay. I added the integration user to the GitHub zeromq.js team and to the npm package. Please let me know if this is enough.

If Travis is tricky to get setup with the correct secrets, we can also try GitHub actions which is a bit easier when it comes to secrets.

@rolftimmermans
Copy link
Member Author

rolftimmermans commented Nov 3, 2019

@lgeiger Perfect! I have added the encrypted keys and will wait for Travis to pass before merging.

Github actions does seem like an interesting alternative for the future. But with the API keys in place I think Travis should be enough for now to automate the build & release process.

@lgeiger
Copy link
Member

lgeiger commented Nov 3, 2019

Cool 👍
I properly configured Appveyor to only build branches that have an appveyor.yml, so we can ignore the failing build here.

@rolftimmermans rolftimmermans merged commit 88b2490 into zeromq:master Nov 3, 2019
@rolftimmermans rolftimmermans deleted the upstream branch November 3, 2019 20:01
@rgbkrk
Copy link
Member

rgbkrk commented Nov 4, 2019

Wahoo!!!

@somdoron
Copy link
Member

@lgeiger @rolftimmermans did you release a version to npm? I want to use the new async API

@rolftimmermans
Copy link
Member Author

@lgeiger @rolftimmermans did you release a version to npm? I want to use the new async API

Yep! Try npm i --save [email protected] or yarn add [email protected]

@somdoron
Copy link
Member

thanks!

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.

7 participants