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

NAN_MODULE_INIT #131

Closed artemp closed 6 years ago

artemp commented 6 years ago

https://github.com/mapbox/node-cpp-skel/blob/2883650fb1948273447d6eddf26eb516cc670e0c/src/module.cpp#L11

^ Why not NAN_MODULE_INIT as we're already using NAN macros across code base? For example : https://github.com/mapbox/node-or-tools/blob/master/src/main.cc#L4

Consistency is important in achieving readability and reducing learning curve.

springmeyer commented 6 years ago

Consistency is important in achieving readability and reducing learning curve.

Agree.

^ Why not NAN_MODULE_INIT as we're already using NAN macros across code base?

Our thinking was (@GretaCB and I looked into this and decided against using it):

  1. We saw NAN_MODULE_INIT in use there, and wanted to figure out what it did but could not see it in the official guides: https://nodejs.org/api/addons.html
  2. So, then we looked inside Nan.h and saw:
#define NAN_MODULE_INIT(name)                                                  \
    void name(Nan::ADDON_REGISTER_FUNCTION_ARGS_TYPE target)
  1. I felt like that did not offer enough value to warrant using the macro.
artemp commented 6 years ago

We saw NAN_MODULE_INIT in use there, and wanted to figure out what it did but could not see it in the official guides: https://nodejs.org/api/addons.html

There is a link to NAN examples on that page https://github.com/nodejs/nan/tree/master/examples/ with NAN_MODULE_INIT usage.

I felt like that did not offer enough value to warrant using the macro.

NAN_METHOD is a one-liner, too : https://github.com/nodejs/nan/blob/master/nan.h#L1443

I think we're in an agreement - consistency wins :)