n-api: refactor napi_addon_register_func #15088

boingoing · 2017-08-30T09:58:33Z

As per discussion in abi-stable-node:
nodejs/abi-stable-node#256,
take a refactor to napi_addon_register_func such that
the result from the register function is assigned to
the module exports property. By making this change,
native module can be agnostic about which type of
module the environment supports.

Checklist

make -j4 test (UNIX), or vcbuild test (Windows) passes
tests and/or benchmarks are included
documentation is changed or added
commit message follows commit guidelines

Affected core subsystem(s)

n-api

gabrielschulhof · 2017-08-30T10:38:05Z

test/addons-napi/5_function_factory/binding.c

+napi_value Init(napi_env env) {
+  napi_value exports;
+  NAPI_CALL(env,
+    napi_create_function(env, "exports", CreateFunction, NULL, &exports));


Nit: Four spaces for continuations, IIRC.

Thanks for pointing all these out. Visual Studio strikes again, I guess. I will push an update with these nits fixed. 👍

gabrielschulhof

LGTM

gabrielschulhof · 2017-08-30T10:43:54Z

test/addons-napi/4_object_factory/binding.c

+napi_value Init(napi_env env) {
+  napi_value exports;
+  NAPI_CALL(env,
+    napi_create_function(env, "exports", CreateObject, NULL, &exports));


Four spaces for continuation, IIRC.

gabrielschulhof · 2017-08-30T10:44:19Z

test/addons-napi/7_factory_wrap/binding.cc

-  NAPI_CALL_RETURN_VOID(env, napi_define_properties(env, module, 1, &desc));
+  napi_value exports;
+  NAPI_CALL(env,
+    napi_create_function(env, "exports", CreateObject, NULL, &exports));


Four spaces.

gabrielschulhof · 2017-08-30T10:44:38Z

test/addons-napi/8_passing_wrapped/binding.cc

+  napi_value exports;
+  NAPI_CALL(env, napi_create_object(env, &exports));
+
+  NAPI_CALL(env,
    napi_define_properties(env, exports, sizeof(desc) / sizeof(*desc), desc));


gabrielschulhof · 2017-08-30T10:44:55Z

test/addons-napi/test_array/test_array.c

+  napi_value exports;
+  NAPI_CALL(env, napi_create_object(env, &exports));
+
+  NAPI_CALL(env, napi_define_properties(
    env, exports, sizeof(descriptors) / sizeof(*descriptors), descriptors));


gabrielschulhof · 2017-08-30T10:45:06Z

test/addons-napi/test_async/test_async.cc

+  napi_value exports;
+  NAPI_CALL(env, napi_create_object(env, &exports));
+
+  NAPI_CALL(env, napi_define_properties(


gabrielschulhof · 2017-08-30T10:45:20Z

test/addons-napi/test_buffer/test_buffer.c

@@ -137,8 +138,10 @@ void Init(napi_env env, napi_value exports, napi_value module, void* priv) {
    DECLARE_NAPI_PROPERTY("staticBuffer", staticBuffer),
  };

-  NAPI_CALL_RETURN_VOID(env, napi_define_properties(
+  NAPI_CALL(env, napi_define_properties(


gabrielschulhof · 2017-08-30T10:45:35Z

test/addons-napi/test_constructor/test_constructor.c

@@ -77,14 +77,12 @@ void Init(napi_env env, napi_value exports, napi_value module, void* priv) {
  };

  napi_value cons;
-  NAPI_CALL_RETURN_VOID(env, napi_define_class(env, "MyObject", New,
+  NAPI_CALL(env, napi_define_class(env, "MyObject", New,


gabrielschulhof

Boy, I sure painted myself into a corner finding all the two-space continuations that should've been four spaces :) Anyway, this change is awesome, and brings N-API closer to the JerryScript module initializer.

gabrielschulhof · 2017-08-30T10:45:49Z

test/addons-napi/test_conversions/test_conversions.c

+  napi_value exports;
+  NAPI_CALL(env, napi_create_object(env, &exports));
+
+  NAPI_CALL(env, napi_define_properties(


gabrielschulhof · 2017-08-30T10:46:14Z

test/addons-napi/test_error/test_error.cc

+  napi_value exports;
+  NAPI_CALL(env, napi_create_object(env, &exports));
+
+  NAPI_CALL(env, napi_define_properties(


gabrielschulhof · 2017-08-30T10:46:22Z

test/addons-napi/test_exception/test_exception.c

+  napi_value exports;
+  NAPI_CALL(env, napi_create_object(env, &exports));
+
+  NAPI_CALL(env, napi_define_properties(


gabrielschulhof · 2017-08-30T10:46:40Z

test/addons-napi/test_general/test_general.c

+  napi_value exports;
+  NAPI_CALL(env, napi_create_object(env, &exports));
+
+  NAPI_CALL(env, napi_define_properties(


gabrielschulhof · 2017-08-30T10:46:50Z

test/addons-napi/test_handle_scope/test_handle_scope.c

+  napi_value exports;
+  NAPI_CALL(env, napi_create_object(env, &exports));
+
+  NAPI_CALL(env, napi_define_properties(
    env, exports, sizeof(properties) / sizeof(*properties), properties));


gabrielschulhof · 2017-08-30T10:47:33Z

test/addons-napi/test_properties/test_properties.c

+  napi_value exports;
+  NAPI_CALL(env, napi_create_object(env, &exports));
+
+  NAPI_CALL(env, napi_define_properties(


gabrielschulhof · 2017-08-30T10:47:42Z

test/addons-napi/test_reference/test_reference.c

+  napi_value exports;
+  NAPI_CALL(env, napi_create_object(env, &exports));
+
+  NAPI_CALL(env, napi_define_properties(
    env, exports, sizeof(descriptors) / sizeof(*descriptors), descriptors));


gabrielschulhof · 2017-08-30T10:47:49Z

test/addons-napi/test_string/test_string.c

+  napi_value exports;
+  NAPI_CALL(env, napi_create_object(env, &exports));
+
+  NAPI_CALL(env, napi_define_properties(
    env, exports, sizeof(properties) / sizeof(*properties), properties));


gabrielschulhof · 2017-08-30T10:47:56Z

test/addons-napi/test_symbol/test_symbol.c

+  napi_value exports;
+  NAPI_CALL(env, napi_create_object(env, &exports));
+
+  NAPI_CALL(env, napi_define_properties(
    env, exports, sizeof(properties) / sizeof(*properties), properties));


gabrielschulhof · 2017-08-30T10:48:03Z

test/addons-napi/test_typedarray/test_typedarray.c

+  napi_value exports;
+  NAPI_CALL(env, napi_create_object(env, &exports));
+
+  NAPI_CALL(env, napi_define_properties(
    env, exports, sizeof(descriptors) / sizeof(*descriptors), descriptors));


RReverser · 2017-08-30T11:51:42Z

Would it make sense to update docs together with actual impl changes or you want to do that in a separate PR?

mhdawson · 2017-08-30T15:07:59Z

src/node_api.cc

-    env,
-    v8impl::JsValueFromV8LocalValue(exports),
-    v8impl::JsValueFromV8LocalValue(module),
-    mod->nm_priv);


@kkoopa, @rvagg I'm wondering if you have any insight as to why the module and exports were made available to modules in the past. This change means that the addon won't have access to the module object and will only be able to replace the exports object as opposed to being able to add to it.

No idea why. As far as I know, it is just the way it always has been (at least from Node v0.6 onwards).

I think it was just for compatibility with JS-based CommonJS modules where you have both pre-created exports and module itself available.

Seems like it was done so native modules could also do module.exports = function() {};

Ok from comments/backgroup it does not sound like we think the limitations will be a problem. I guess worst case is we have to add a second register with additional parameters later on and best case we don't need them and stick with it as proposed here. So once updates are added I'll take another look.

One alternative that was discussed was to pre-emptively reserve a parameter in the register func signature in case the lack of module object availability during registration became a problem- that would allow us to deal with that situation in the future if it becomes a problem without causing a breaking change

@digitalinfinity Even if it's needed in the future, it should be easy to add it as a separate function similar to napi_get_global. Although I don't see any good reason to expose such CommonJS implementation details to native modules for now.

@RReverser That makes sense- for some reason I was imagining the fix would involve changing the signature of napi_addon_register_func but your approach makes sense- so I'm fine with this signature as is.

mhdawson · 2017-08-30T15:08:37Z

I think we should update the docs in the same PR.

boingoing · 2017-08-30T17:00:51Z

Thanks for reviewing, folks. I will update the docs along with the code changes.

boingoing · 2017-08-31T01:19:15Z

@mhdawson, @gabrielschulhof Updated to add docs and address the spacing nitpicks (also fixed all the other line continuation indentation mistakes I could find in the addons-napi tests).

digitalinfinity · 2017-08-31T02:50:13Z

doc/api/n-api.md

  napi_status status;
  napi_property_descriptor desc =
    {"hello", Method, 0, 0, 0, napi_default, 0};
+  napi_value exports;
+  status = napi_create_object(env, &exports);
  status = napi_define_properties(env, exports, 1, &desc);


Nit: might want to update the sample to check the status of napi_create_object before calling napi_define_properties- it reads a little better, and helps folks reusing this sample from making a mistake

That's fair. I noticed this sample and the one below both save the status code and then do nothing with it while I was updating the doc. Makes sense to have both of them do something when the status is not ok, especially now that the Init function return value has meaning.

digitalinfinity · 2017-08-31T02:52:49Z

src/node_api.cc

-    env,
-    v8impl::JsValueFromV8LocalValue(exports),
-    v8impl::JsValueFromV8LocalValue(module),
-    mod->nm_priv);


One alternative that was discussed was to pre-emptively reserve a parameter in the register func signature in case the lack of module object availability during registration became a problem- that would allow us to deal with that situation in the future if it becomes a problem without causing a breaking change

gabrielschulhof · 2017-09-01T21:58:28Z

After thinking about it for a while, I realized that, since most often modules consist of an object that has several functions, perhaps we should pass through exports as the second parameter (after the napi_env), while retaining the napi_value return value.

Thus, modules which populate exports need not create a new object, since Node has already done that.

Modules that overwrite exports can simply return the value with which they wish to overwrite.

On the implementation side, if the init callback returns NULL, we assume it has populated exports and we do nothing. If it returns non-NULL, we set the exports property on the module.

If Node ever went into a mode where it no longer supplies exports we can just create it from scratch and pass it in. Then, if the module returns NULL, the object we passed in is the result of having loaded the module, otherwise the return value is the result. That way it would be a convenience we provide which modules need not make use of.

I think the essence of the improvement in this PR is not the removal of the arguments, but rather the use of a return value. Setting module.exports is a really round-about way of saying the same thing that a return value says - i.e. that this is the result of having loaded the module.

mhdawson · 2017-09-05T20:11:02Z

In the last N-API meeting we agreed that we should add a reserved parameter as suggested by @boingoing above.

@gabrielschulhof I like the idea of avoiding forcing modules to create their own object. Once we figure out ES6 support we might even be able to recognize what type of module is being loaded and then provide the right object regardless of the type of module.

I wonder if it should be that we provide an object to which exports "can" be added (at least for existing CS modules for now) but that its up to the code to either return that object after they have made additions or return a different object. ie. the module must always return an object, but that it can be a modified version of the exports object that was passed in.

gabrielschulhof · 2017-09-05T20:27:48Z

@mhdawson

I wonder if it should be that we provide an object to which exports "can" be added (at least for existing CS modules for now) but that its up to the code to either return that object after they have made additions or return a different object. ie. the module must always return an object, but that it can be a modified version of the exports object that was passed in.

👍
One possible future addition might be to examine the structure of the object such that if the object has a property named default, remove it from the object and it becomes the default export, and the rest of the object's properties become explicit exports.

mhdawson · 2017-09-06T21:59:51Z

@nodejs/n-api lets make sure to discuss this in our meeting this week so we can agree/close on the way forward.

mhdawson · 2017-09-07T18:03:29Z

We have agreed:

to we won't include a reserved field
We will require that an object is returned
We will pass in an exports object to which exports can be added and that object returned in order to avoid modules having to create their own object.

As per discussion in abi-stable-node: nodejs/abi-stable-node#256, take a refactor to napi_addon_register_func such that the result from the register function is assigned to the module exports property. By making this change, native module can be agnostic about which type of module the environment supports.

boingoing · 2017-09-14T09:45:24Z

Implemented pending feedback and rebased to resolve conflicts.

mhdawson

LGTM

mhdawson · 2017-09-14T19:03:11Z

Landed as 92e5f5c

As per discussion in abi-stable-node: nodejs/abi-stable-node#256, take a refactor to napi_addon_register_func such that the result from the register function is assigned to the module exports property. By making this change, native module can be agnostic about which type of module the environment supports. PR-URL: #15088 Reviewed-By: Gabriel Schulhof <[email protected]> Reviewed-By: Michael Dawson <[email protected]> Reviewed-By: Hitesh Kanwathirtha <[email protected]>

As per discussion in abi-stable-node: nodejs/abi-stable-node#256, take a refactor to napi_addon_register_func such that the result from the register function is assigned to the module exports property. By making this change, native module can be agnostic about which type of module the environment supports. PR-URL: nodejs/node#15088 Reviewed-By: Gabriel Schulhof <[email protected]> Reviewed-By: Michael Dawson <[email protected]> Reviewed-By: Hitesh Kanwathirtha <[email protected]>

As per discussion in abi-stable-node: nodejs/abi-stable-node#256, take a refactor to napi_addon_register_func such that the result from the register function is assigned to the module exports property. By making this change, native module can be agnostic about which type of module the environment supports. PR-URL: #15088 Reviewed-By: Gabriel Schulhof <[email protected]> Reviewed-By: Michael Dawson <[email protected]> Reviewed-By: Hitesh Kanwathirtha <[email protected]>

As per discussion in abi-stable-node: nodejs/abi-stable-node#256, take a refactor to napi_addon_register_func such that the result from the register function is assigned to the module exports property. By making this change, native module can be agnostic about which type of module the environment supports. PR-URL: nodejs/node#15088 Reviewed-By: Gabriel Schulhof <[email protected]> Reviewed-By: Michael Dawson <[email protected]> Reviewed-By: Hitesh Kanwathirtha <[email protected]>

As per discussion in abi-stable-node: nodejs/abi-stable-node#256, take a refactor to napi_addon_register_func such that the result from the register function is assigned to the module exports property. By making this change, native module can be agnostic about which type of module the environment supports. PR-URL: nodejs#15088 Reviewed-By: Gabriel Schulhof <[email protected]> Reviewed-By: Michael Dawson <[email protected]> Reviewed-By: Hitesh Kanwathirtha <[email protected]>

As per discussion in abi-stable-node: nodejs/abi-stable-node#256, take a refactor to napi_addon_register_func such that the result from the register function is assigned to the module exports property. By making this change, native module can be agnostic about which type of module the environment supports. Backport-PR-URL: #19447 PR-URL: #15088 Reviewed-By: Gabriel Schulhof <[email protected]> Reviewed-By: Michael Dawson <[email protected]> Reviewed-By: Hitesh Kanwathirtha <[email protected]>

nodejs-github-bot added c++ Issues and PRs that require attention from people who are familiar with C++. lib / src Issues and PRs related to general changes in the lib or src directory. labels Aug 30, 2017

boingoing mentioned this pull request Aug 30, 2017

ES6 module loader compatibility nodejs/abi-stable-node#256

Closed

gabrielschulhof reviewed Aug 30, 2017

View reviewed changes

gabrielschulhof approved these changes Aug 30, 2017

View reviewed changes

gabrielschulhof added the node-api Issues and PRs related to the Node-API. label Aug 30, 2017

gabrielschulhof reviewed Aug 30, 2017

View reviewed changes

gabrielschulhof approved these changes Aug 30, 2017

View reviewed changes

mhdawson reviewed Aug 30, 2017

View reviewed changes

digitalinfinity mentioned this pull request Aug 30, 2017

n-api: remove n-api module loading flag #14902

Closed

4 tasks

boingoing force-pushed the refactor_napi_addon_register_func branch from ea5c16d to 4fd3578 Compare August 30, 2017 23:36

digitalinfinity approved these changes Aug 31, 2017

View reviewed changes

boingoing force-pushed the refactor_napi_addon_register_func branch from fb68fc7 to 746af62 Compare September 14, 2017 09:38

mhdawson approved these changes Sep 14, 2017

View reviewed changes

mhdawson closed this Sep 14, 2017

jasnell mentioned this pull request Sep 20, 2017

Node.js 8.6.0 Release Proposal #15509

Merged

MylesBorins added the dont-land-on-v6.x label Oct 17, 2017

MylesBorins mentioned this pull request Apr 16, 2018

v6.14.2 proposal #19996

Merged

adamlaska mentioned this pull request May 13, 2022

[Snyk] Fix for 1 vulnerabilities adamlaska/node#67

Open

snyk-bot mentioned this pull request Apr 27, 2023

[Snyk] Fix for 20 vulnerabilities aliscco/alisco-node#253

Open

This was referenced Apr 28, 2023

[Snyk] Fix for 16 vulnerabilities aliscco/alisco-node#304

Open

[Snyk] Fix for 18 vulnerabilities aliscco/alisco-node#310

Open

snyk-bot mentioned this pull request Apr 28, 2023

[Snyk] Fix for 20 vulnerabilities aliscco/alisco-node#314

Open

This was referenced Apr 28, 2023

[Snyk] Fix for 9 vulnerabilities aliscco/alisco-node#330

Open

[Snyk] Fix for 19 vulnerabilities aliscco/alisco-node#334

Open

snyk-bot mentioned this pull request Apr 28, 2023

[Snyk] Fix for 114 vulnerabilities aliscco/alisco-node#338

Open

aliscco mentioned this pull request Apr 28, 2023

[Snyk] Fix for 8 vulnerabilities aliscco/alisco-node#342

Open

n-api: refactor napi_addon_register_func #15088

n-api: refactor napi_addon_register_func #15088

Conversation

boingoing commented Aug 30, 2017 • edited Loading

Checklist

Affected core subsystem(s)

Choose a reason for hiding this comment

Choose a reason for hiding this comment

gabrielschulhof left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

gabrielschulhof left a comment • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

RReverser commented Aug 30, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

RReverser Aug 31, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mhdawson commented Aug 30, 2017

boingoing commented Aug 30, 2017

boingoing commented Aug 31, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

gabrielschulhof commented Sep 1, 2017

mhdawson commented Sep 5, 2017

gabrielschulhof commented Sep 5, 2017

mhdawson commented Sep 6, 2017

mhdawson commented Sep 7, 2017

boingoing commented Sep 14, 2017

mhdawson left a comment

Choose a reason for hiding this comment

mhdawson commented Sep 14, 2017

boingoing commented Aug 30, 2017 •

edited

Loading

gabrielschulhof left a comment •

edited

Loading

RReverser Aug 31, 2017 •

edited

Loading