Skip to content
This repository has been archived by the owner on Apr 24, 2020. It is now read-only.
This repository has been archived by the owner on Apr 24, 2020. It is now read-only.

Massive refactoring underway #515

Open
ronkorving opened this issue Apr 18, 2016 · 5 comments · May be fixed by #516
Open

Massive refactoring underway #515

ronkorving opened this issue Apr 18, 2016 · 5 comments · May be fixed by #516

Comments

@ronkorving
Copy link
Collaborator

ronkorving commented Apr 18, 2016

Just an FYI (to those interested, eg @kkoopa, @reqshark).

I'm completely refactoring what we export, as it's such a complete and utter mess right now, I want to prepare you for what's coming:

Removal of all constants on module.exports

Yes, all constants are gone. This is relevant, because some constants actually conflict with API (subscribe and unsubscribe for example are socket options, but also API). As ZMQ gets more and more socket options, this has become an unmanageable mess. Time for some clean up. That means that people will have to use getsockopt and setsockopt to access options. They may still use shorthand (last_endpoint) and full constant string names (ZMQ_LAST_ENDPOINT), as well as the actual integer representing the option.

No more module.exports = zmq; // the binding

The binding should be wrapped, not infested with all kinds of separate API.

A rewrite of how defined constants are registered

Currently, they're stored by number. In the upcoming PR they will be:

#ifdef ZMQ_LAST_ENDPOINT
  OPT(binary, ZMQ_LAST_ENDPOINT);
#endif

This macro will inject it into opts_binary and JS land: zmq.options (string -> int). All the options are gone from JS land. It's all driven from C++. JS just exposes it.
I'm taking this opportunity to add all missing options (there are a few).

Once all this is done, this will obviously mean major version bump.

@ronkorving ronkorving linked a pull request Apr 19, 2016 that will close this issue
@reqshark
Copy link
Contributor

this is awesome, i've already had a chance to review some of these changes! great job!

@ralphtheninja
Copy link

It's all driven from C++. JS just exposes it.

+1

@rgbkrk
Copy link

rgbkrk commented Apr 24, 2016

Let me know if you want to adapt the prebuilt work over to here or if there are other bits you'd like to tackle. @minrk stated an interest in cleaning up some of the race conditions present in the current code base too.

@ronkorving
Copy link
Collaborator Author

What race conditions? I don't think I'm aware of any...
The prebuilt work would be great to have and seems independent to #516, so go for it. I'm no expert in this area, so could definitely use some help from as many people as possible there.

@minrk
Copy link

minrk commented Apr 25, 2016

Race conditions were just in tests, not library code. Changes were here.

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging a pull request may close this issue.

5 participants