mapbox / node-cpp-skel

Skeleton for bindings to C++ libraries for Node.js using node-addon-api
Creative Commons Zero v1.0 Universal
72 stars 10 forks source link

remove magic NAN macros #33

Closed mikemorris closed 7 years ago

mikemorris commented 8 years ago

Fixes #17

And adds a few modern NAN best practices from https://nodesource.com/blog/cpp-addons-for-nodejs-v4/

/cc @mapsam @gretacb @springmeyer

codecov-io commented 8 years ago

Current coverage is 98.73% (diff: 87.50%)

Merging #33 into master will not change coverage

@@             master        #33   diff @@
==========================================
  Files             2          2          
  Lines            79         79          
  Methods          13         13          
  Messages          0          0          
  Branches          0          0          
==========================================
  Hits             78         78          
  Misses            1          1          
  Partials          0          0          

Powered by Codecov. Last update b95d6d7...fd650e4

mikemorris commented 8 years ago

It looks like this doesn't run on Node.js 0.10 (master doesn't seem to either), although it probably could. Is this something worth fixing?

module.js:356
Module._extensions[extension](this, filename);
                               ^
Error: Symbol hello_world_module not found.
mapsam commented 8 years ago

Nice @mikemorris! Thanks for putting this together. It's super helpful to see what these macros are actually doing, and how v8 truly comes into play here.

For the sake of the skel, I think it would be useful to keep the macros as comments above, just as a reference for later. What do you think?

springmeyer commented 7 years ago

I've reviewed this and the https://nodesource.com/blog/cpp-addons-for-nodejs-v4/. I feel like the NAN_METHOD macros are fine: they are still supported, more concise, and work fine. So I'm not feeling like this improves the code significantly. However the use of Nan::Utf8String is smart and worthwhile to land. Much refactoring has happened recently, so this PR is stale. I'm going to close this now and have created a ticket for tracking moving to Nan::Utf8String at #54