Add module initializer with context corresponding to NODE_MODULE_INITIALIZER #844

Activity

1. NickNaso commented on Mar 27, 2019

   

   NickNaso

   on Mar 27, 2019

   Last edited by NickNaso

   Member

   More actions

   Hi @murgatroid99,
   I think that NAN_MODULE_WORKER_ENALBED is the macro that you need. https://github.com/nodejs/nan/blob/master/nan.h#L158
   Can someone say if I'm right or not?
2. murgatroid99 commented on Mar 27, 2019

   

   murgatroid99

   on Mar 27, 2019

   Author

   More actions

   That does look relevant, but it looks like it just ignores the context parameter, instead of providing some kind of shim or fallback. So, it looks like it doesn't help with actually writing an addon that is context-aware in Node 10+.
3. ctavan commented on Oct 25, 2019

   

   ctavan

   on Oct 25, 2019

   More actions

   @NickNaso are there any plans to resolve this issue or is there anything I could contribute?

   This one is currently blocking grpc/grpc-node#778 which in turn is causing other trouble further down stream (vercel/next.js#7894) so I would be interested in getting this resolved.

   👍React with 👍1Reacted by Matt Burman
4. murgatroid99 commented on Aug 4, 2020

   

   murgatroid99

   on Aug 4, 2020

   Author

   More actions

   According to electron/electron#18397, as of Electron 11 this will be required to load all native addons in Electron. Therefore, changing this should be a priority.
5. bnoordhuis commented on Aug 5, 2020

   

   bnoordhuis

   on Aug 5, 2020

   Member

   More actions

   That does look relevant, but it looks like it just ignores the context parameter, instead of providing some kind of shim or fallback.

   What should that shim/fallback do in your opinion? Add-ons can already obtain the context in their initializer with Nan::GetCurrentContext().
6. murgatroid99 commented on Aug 5, 2020

   

   murgatroid99

   on Aug 5, 2020

   Author

   More actions

   Look, you have this definition:

   #if NODE_MAJOR_VERSION >= 10 || \
       NODE_MAJOR_VERSION == 9 && NODE_MINOR_VERSION >= 3
   #define NAN_MODULE_WORKER_ENABLED(module_name, registration)                   \
       extern "C" NODE_MODULE_EXPORT void                                         \
         NAN_CONCAT(node_register_module_v, NODE_MODULE_VERSION)(                 \
           v8::Local<v8::Object> exports, v8::Local<v8::Value> module,            \
           v8::Local<v8::Context> context)                                        \
       {                                                                          \
           registration(exports);                                                 \
       }
   

   That applies to the version range we are interested in, and it discards the context parameter instead of passing it to the registration function. This macro appears to correspond to the NODE_MODULE_INIT macro described in the Context-aware addons documentation, and that documentation states that the context variable is available in the function body passed to that macro. Based on this information, it seems like NAN_MODULE_WORKER_ENABLED does not facilitate the creation of a context-aware addon the way NODE_MODULE_INIT does.

   Honestly, I was unaware of Nan::GetCurrentContext() and nobody mentioned it in this issue before, and the documentation doesn't clearly indicate that it can be used in that function as a replacement for the context variable.
7. bnoordhuis commented on Aug 6, 2020

   

   bnoordhuis

   on Aug 6, 2020

   Member

   More actions

   @murgatroid99 Do you want to update the API or the docs? In both cases a pull request is the best way forward. :-)
8. murgatroid99 commented on Aug 6, 2020

   

   murgatroid99

   on Aug 6, 2020

   Author

   More actions

   I don't know what the right change here is. I'm just a user of this library; that doesn't mean I'm knowledgeable about the nuances of the Node API or its changes across versions.

   In particular, you say that Nan::GetCurrentContext() can access the current context in the module initializer. If that is the case, why does Node's API also pass it in as an argument to the initializer? I think someone who knows the answer to that question is in a better position to determine what change should be made here.
9. bnoordhuis commented on Aug 9, 2020

   

   bnoordhuis

   on Aug 9, 2020

   Member

   More actions

   why does Node's API also pass it in as an argument to the initializer?

   As the one who introduced the original multi-context support in Node: because it seemed like a good idea at the time. :-)

   ❤️React with ❤️1Reacted by David Siegel