Implemented Nan and aligned to segfault_handler naming convention #14
Conversation
Implemented Nan Aligned to segfault_handler naming convention
|
I'm not sure I understand what these new NaN macros do in 0.11. Is there a short explanation? |
|
Nan is an external project https://github.com/rvagg/nan not part of node v.11 or 0.12. The nan macros provide insulation from the different versions of V8 used within the different versions of node.js (0.8 0.10 and 0.11) Where there are differences in implementation such as the one that was raised in this issue The Nan project is contributed to by Ben Noordhuis and TJ Fontaine from core so it is well regarded. |
|
Hmm. very interesting. C++ compat between node versions has been a huge mess. So are you saying that this PR is ready to merge, safe, tested, and should't regress or break any scenarios? If so, I can take your word for it and hit "merge". It's on my TODO list to go through this module and test all these damned versions and cut an update that addresses all the know issues. I feel like a terrible maintainer, but right now, I don't even have 1 version of Node installed on my own dev box! |
| @@ -6,7 +6,7 @@ Using the module is as simple as: | |||
|
|
|||
| ```javascript | |||
|
|
|||
| var SegfaultHandler = require('segfault-handler'); | |||
| var SegfaultHandler = require('segfault_handler'); | |||
There was a problem hiding this comment.
Why does this need to change? Does a dash break something?
There was a problem hiding this comment.
It makes it consistent and I chose to go this way as this is how it's defined in
https://github.com/ddopson/node-segfault-handler/blob/master/src/segfault-handler.cpp#L97
It is recommended that the addon name is consistent with the name of the module.
More details here
http://nodejs.org/api/addons.html#addons_addon_patterns
Specifically
The module_name needs to match the filename of the final binary (minus the .node suffix).
|
I am happy that this pull works as expected on my environment. On the multiple versions of node I would suggest we drop this in at some point. |
Implemented Nan and aligned to segfault_handler naming convention
|
I took your pull request, but added a patch to switch the name back to 'segfault-handler'. I tested and with the dash in the name, the build still works. At least on my machine. Let me know if you have trouble building it on your machine. I like the idea of testing, but testing a segfault is tricky. Perhaps I can take the callback patch and use that for testing. I VERY much like the idea of testing multiple node versions. |
|
Thanks for taking the patch :) If you install docker then DNT you can checkout my master and see it working The good news is that this patch compiles on v0.11.14 and v0.10.32 and passes OK. |
|
Sweet. Thanks for the contributions. Btw, are you using this for work or If you are curious, https://www.linkedin.com/in/davedopson. I use 0 Also, do you use anything to keep track of Github pull requests? I never --- Dave On Wed, Oct 1, 2014 at 5:47 PM, Anton Whalley [email protected]
|
|
oh, this posts on the bug doesn't it. Sigh. whatever. On Fri, Oct 3, 2014 at 10:20 PM, Dave Dopson [email protected] wrote:
|
|
Oh, spawning a child process for the test. That seems so obvious now that On Fri, Oct 3, 2014 at 10:20 PM, Dave Dopson [email protected] wrote:
|
This is a breaking change as it names the module segfault_handler with an underscore to make the build and sample code work.
On the upside it implements Nan change and so should run on later version of node.
Would recommend publishing in npm as segfault_handler too for consistency and maybe removing/deprecating the current 2.1 in there.