[WIP] node-zmq 3 #516

ronkorving · 2016-04-19T03:11:04Z

Hopefully, this makes things a bit more sane.
Please note that this would be a major version bump to 3. We will still need to maintain node-zmq 2 for a while (bugfixes), so we should create a 2.x branch off of master before merging this.

Closes #515
Closes #438
Closes #279
Closes #175

Changes:

All APIs are now documented in the readme.
The module exports no longer is the binding. The binding is now on exports.binding. Users shouldn't need it, but there's no real need to hide it either.
All hardcoded constant lists removed from JS, all now come straight from C++ and feature detection. That means: socket types, send-flags, socket options, context options.
All number referred constants removed from C++. All constants are registered by name now.
All constants may be referred to by their string version (eg: "ZMQ_SUBSCRIBE") or by their lower case version with the "zmq_" prefix removed (eg: "subscribe").
Alias for getsockopt/setsockopt to simply get/set on the Socket class.
All constants are removed from the module exports. No more zmq.FOO_BAR. All getters and setters are gone. That means you now have to use get(sockopt) and set(sockopt).
Exposed zmq.contextOptionExists(option), zmq.socketOptionExists(option) and zmq.socketTypeExists(type) functions.
Context is now a proper JS class, and custom contexts can be created and configured.
Sockets can now be created from a context, or still straight from the module (using the default context).
Some style fixes and added some zmq_ return code < 0 checks that were missing.
Maintained support for Node >=0.10 <=5, and ZMQ >=2 <= 4.1.
Removed legacy send/receive methods from C++ that were no longer used.
Updated all unit tests.
probably more...

TODO:

Incorporate fixes from Should fix missing incoming messages #531, This fixes burst sends on "req" sockets. #536 and probably skip monitoring callback if socket has been closed #540
More style fixes, everywhere.
ESLint (could be post-merge).

cc @kkoopa @reqshark

reqshark · 2016-04-19T21:03:37Z

binding.cc

-      if (zmq_term(context_) < 0) throw std::runtime_error(ErrorMessage());
+      int rc;
+
+      while (true) {


do we need the while loop here?

Yes, for EINTR

ronkorving · 2016-04-20T05:54:36Z

binding.cc

-          return Nan::ThrowError(ErrorMessage());
-        }
-        break;
-      }


IncomingMessage was already doing the init.

reqshark · 2016-04-20T06:26:48Z

pretty impressive cleanup to MessageReference class, i defer to @kkoopa for review there

(outside my pay grade)

ronkorving · 2016-04-20T06:58:33Z

MessageReference I didn't really touch. I just moved it. But yes, lots of things pertaining to native code have been touched or changed, so @kkoopa's review would be most welcome.

reqshark · 2016-04-20T09:12:01Z

test/zmq_proxy.xrep-xreq.js

@@ -29,7 +29,7 @@ describe('proxy.xrep-xreq', function() {
    req.connect(frontendAddr);
    rep.connect(backendAddr);

-    req.on('message',function(msg){
+    req.on('message',function (msg) {


space btwn comma and function

More style fixes

eljefedelrodeodeljefe · 2016-05-07T15:02:44Z

Deifinitely makes the module more interesting. Good effort.

ronkorving · 2016-05-11T13:49:50Z

README.md

+
+### Constants
+
+ZeroMQ is full of constants for various that are [well documented](http://api.zeromq.org/). It would be overkill to


*for various what?
TODO: fix

ronkorving · 2016-06-13T04:21:23Z

@kkoopa @reqshark @ralphtheninja
Does it make sense if I pursue this PR before #537? That way, if that proposal is successful, we don't have to backport it and can move on with node-zmq v3. If I go with #537, the branches will drift further apart and I will have to radically update this (v3) PR as well. Perhaps limit the experimentation to v3? Does that sound good?

kkoopa · 2016-06-13T12:15:00Z

Yes, that sounds reasonable.

ralphtheninja · 2016-06-14T11:27:26Z

Does it make sense if I pursue this PR before #537? That way, if that proposal is successful, we don't have to backport it and can move on with node-zmq v3. If I go with #537, the branches will drift further apart and I will have to radically update this (v3) PR as well. Perhaps limit the experimentation to v3? Does that sound good?

Yep!

ralphtheninja · 2016-06-14T11:30:00Z

Maintained support for Node >=0.10 <=5, and ZMQ >=2 <= 4.1.

What about node 6?

ronkorving · 2016-06-14T15:36:40Z

When I made this PR, either there was no Node 6, or we didn't support it yet. Of course if I merge this, it will be in a state where it works on Node 6, no worries :)

JamesMGreene · 2016-06-27T12:10:27Z

Excited for many of the changes coming in this PR! 👍

ronkorving · 2016-06-27T12:43:35Z

Thanks :) I really need to pick it up again, have been quite busy lately.

reqshark · 2016-06-30T05:49:26Z

IMO node 5 and node 6 are the same thing in terms of binding compiles... at least using [email protected]

nicwsm · 2016-07-11T06:09:17Z

May I know when will node-zmq 3 be ready? Or has it already been launched?

ronkorving · 2016-07-11T06:16:13Z

It's not ready yet. But I'm thinking within about 2-3 weeks it should be.

nicwsm · 2016-07-11T06:19:46Z

I'm also facing the same issue as #438 at the moment, but since it will require me to wait for another 2-3 weeks for the issue to be closed until version 3 comes out, may I ask if I can get any temporal help for now to get the ZMQ_IMMEDIATE issue solved?

ronkorving · 2016-07-11T06:21:43Z

Time is my problem right now (else I would've shipped node-zmq 3 by now) so if anyone wants to help reinvigorate #438, raise your hands please.

nicwsm · 2016-07-11T06:32:39Z

@ronkorving Sure. Hopefully there are someone who will help in this, and in the meantime I'll wait for the next release. Thanks!

tlbtlbtlb · 2016-08-15T19:10:44Z

To address #552 (asking to be able to share a zmq context between two different node C++ add-ins), I think the right strategy is to extract the class declarations of zmq::Context and zmq::Socket from binding.cc into a binding.h file that can be included from the other node module. They can use Nan::ObjectWrap::Unwrap<Context>(...)->context_ to get what they need (the zmq_context handle) from a JS Context argument.

I'm happy to submit a patch if people like the idea.

Ron Korving added 2 commits April 18, 2016 18:11

Fixes batch isClosed check

669f80c

Massive refactoring of JS-land

37cfbbc

ronkorving changed the title ~~This refactors options, binding exposure, and more~~ [WIP] This refactors options, binding exposure, and more Apr 19, 2016

Merge branch 'master' into refactor

27f6b2d

ronkorving force-pushed the refactor branch from 3d8c100 to 8392ce6 Compare April 19, 2016 12:04

ronkorving changed the title ~~[WIP] This refactors options, binding exposure, and more~~ [WIP] node-zmq 3 Apr 19, 2016

reqshark reviewed Apr 19, 2016
View reviewed changes

Round 2 of refactoring

6074aac

ronkorving force-pushed the refactor branch from 8392ce6 to 6074aac Compare April 20, 2016 02:39

ronkorving reviewed Apr 20, 2016
View reviewed changes

binding.cc

return Nan::ThrowError(ErrorMessage());

}

break;

}

Copy link

Collaborator Author

ronkorving Apr 20, 2016

Choose a reason for hiding this comment

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

IncomingMessage was already doing the init.

Round 3 of refactoring

3eec6c7

ronkorving force-pushed the refactor branch from 75aa7e2 to 3eec6c7 Compare April 20, 2016 06:00

Added Socket#get/set as alias for getsockopt/setsockopt

ac501a5

reqshark reviewed Apr 20, 2016
View reviewed changes

Exposed zmq.socketOptionExists and zmq.socketTypeExists

4df3e56

More style fixes

ronkorving force-pushed the refactor branch from 32aaec7 to 4df3e56 Compare April 20, 2016 11:02

Ron Korving added 3 commits April 21, 2016 12:57

More style nits

0f162e3

Renamed zmq.zmq to zmq.binding

453ebdd

ReadMe!

ec917b0

ronkorving mentioned this pull request Apr 25, 2016

Massive refactoring underway #515

Open

ronkorving reviewed May 11, 2016
View reviewed changes

ronkorving mentioned this pull request Jun 11, 2016

socket.close() is not really synchronous but there is no event to notify when it is really closed #214

Closed

ronkorving mentioned this pull request Aug 15, 2016

Context from C++ #552

Closed

ronkorving mentioned this pull request Sep 23, 2016

Send from a request endpoint can trigger receive callbacks #560

Open

ronkorving mentioned this pull request Nov 7, 2016

Fix Appveyor builds #562

Open

ronkorving mentioned this pull request Mar 31, 2017

What's the difference from npm's zmq and zeromq? zeromq/zeromq.js#123

Closed

rgbkrk mentioned this pull request Jun 19, 2017

socket.pause() does not work at message level - is this by design? zeromq/zeromq.js#139

Closed

interpretor mentioned this pull request Aug 17, 2017

Mark this project as unmaintained in favor of zeromq/zeromq.js #594

Closed

ronkorving mentioned this pull request Sep 20, 2017

Add getter for socket closed state zeromq/zeromq.js#181

Merged

ronkorving mentioned this pull request Oct 5, 2017

My work on a next-generation proof of concept for ZMQ bindings for Node.js zeromq/zeromq.js#189

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[WIP] node-zmq 3 #516

[WIP] node-zmq 3 #516

ronkorving commented Apr 19, 2016 •

edited

Loading

reqshark Apr 19, 2016

ronkorving Apr 20, 2016

ronkorving Apr 20, 2016

reqshark commented Apr 20, 2016

ronkorving commented Apr 20, 2016

reqshark Apr 20, 2016

eljefedelrodeodeljefe commented May 7, 2016 •

edited

Loading

ronkorving May 11, 2016

ronkorving commented Jun 13, 2016 •

edited

Loading

kkoopa commented Jun 13, 2016

ralphtheninja commented Jun 14, 2016

ralphtheninja commented Jun 14, 2016

ronkorving commented Jun 14, 2016

JamesMGreene commented Jun 27, 2016

ronkorving commented Jun 27, 2016

reqshark commented Jun 30, 2016

nicwsm commented Jul 11, 2016

ronkorving commented Jul 11, 2016

nicwsm commented Jul 11, 2016

ronkorving commented Jul 11, 2016

nicwsm commented Jul 11, 2016

tlbtlbtlb commented Aug 15, 2016


		### Constants

		ZeroMQ is full of constants for various that are [well documented](http://api.zeromq.org/). It would be overkill to

[WIP] node-zmq 3 #516

Are you sure you want to change the base?

[WIP] node-zmq 3 #516

Conversation

ronkorving commented Apr 19, 2016 • edited Loading

reqshark Apr 19, 2016

Choose a reason for hiding this comment

ronkorving Apr 20, 2016

Choose a reason for hiding this comment

ronkorving Apr 20, 2016

Choose a reason for hiding this comment

reqshark commented Apr 20, 2016

ronkorving commented Apr 20, 2016

reqshark Apr 20, 2016

Choose a reason for hiding this comment

eljefedelrodeodeljefe commented May 7, 2016 • edited Loading

ronkorving May 11, 2016

Choose a reason for hiding this comment

ronkorving commented Jun 13, 2016 • edited Loading

kkoopa commented Jun 13, 2016

ralphtheninja commented Jun 14, 2016

ralphtheninja commented Jun 14, 2016

ronkorving commented Jun 14, 2016

JamesMGreene commented Jun 27, 2016

ronkorving commented Jun 27, 2016

reqshark commented Jun 30, 2016

nicwsm commented Jul 11, 2016

ronkorving commented Jul 11, 2016

nicwsm commented Jul 11, 2016

ronkorving commented Jul 11, 2016

nicwsm commented Jul 11, 2016

tlbtlbtlb commented Aug 15, 2016

ronkorving commented Apr 19, 2016 •

edited

Loading

eljefedelrodeodeljefe commented May 7, 2016 •

edited

Loading

ronkorving commented Jun 13, 2016 •

edited

Loading