Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

napi: change napi_callback to return napi_value #12248

Closed

Conversation

boingoing
Copy link
Contributor

@boingoing boingoing commented Apr 6, 2017

Change napi_callback to return napi_value directly instead of
requiring module code to call napi_set_return_value.

When we invoke the callback, we will check the return value and call
SetReturnValue ourselves. If the callback returns NULL, we don't
set the return value in v8 which would have the same effect as
previously if the callback didn't call napi_set_return_value. Seems
to be a more natural way to handle return values from callbacks. As a
consequence, remove napi_set_return_value.

Add a napi_value to napi_property_descriptor to support
string values which couldn't be passed in the utf8name parameter
or symbols as property names. Class names, however, cannot be symbols
so this napi_value must be a string type in that case.

Remove all of the napi_callback_info helpers except for
napi_get_cb_info and make all the parameters to napi_get_cb_info
optional. If argv is provided, argc will become non-optional.

Update all the test collateral according to these changes. Also add
test/addons-napi/common.h to house some common macros for wrapping
N-API calls and error handling.

Checklist
  • make -j4 test (UNIX), or vcbuild test (Windows) passes
  • tests and/or benchmarks are included
  • commit message follows commit guidelines
Affected core subsystem(s)

napi

@nodejs-github-bot nodejs-github-bot added the c++ Issues and PRs that require attention from people who are familiar with C++. label Apr 6, 2017
@boingoing
Copy link
Contributor Author

@jasongin @digitalinfinity Please take a look at this change if you have the time.

@mscdex mscdex added the node-api Issues and PRs related to the Node-API. label Apr 6, 2017
@@ -1,58 +1,37 @@
#include <node_api.h>
#include "..\common.h"
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

/

@@ -1,51 +1,43 @@
#include <node_api.h>
#include "..\common.h"
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ditto, same everywhere

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ah yup. Sorry, dev'd this on Windows. :) Thanks for catching.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Addressed in a new commit - 03c0644

#define NAPI_RETVAL_NOTHING // Intentionally blank #define

#define GET_AND_THROW_LAST_ERROR(env) \
const napi_extended_error_info* error; \
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should this check if the error is reporting an Exception. IF so we probably want to rethrow that instead of throwing a new Error.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That is true though none of the tests we have are checking for this. If an exception is pending, we could clear and rethrow it or do nothing and leave it pending. Is that equivalent?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

updating to leave as pending is the equivalent and makes the most sense to me.

napi_callback_info cbinfo,
napi_value* result);
napi_value* this_arg, // [out] Receives the JS 'this' arg for the call
void** data); // [out] Receives the data pointer for the callback.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'll have to check so this is mostly a note to myself. We don't need napi_get_cb_args_length here because there is another version ?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We added the napi_get_cb_info API so that modules could avoid making several calls to get the callback details and do everything via a single method. The next question, of course, is why would we offer the other several APIs if there's already one which is a super set? Since this PR already changed all the test files to remove napi_set_return_value, it seemed to make sense to roll-in the change to remove all the napi_get_cb_* calls as well.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ok makes sense in that context. The only reason might be if its faster to get one versus several of the values and its a common case where you only need one of them.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I believe the overhead of making the call itself greatly outweighs the actual cost of accessing these members of the callback info object.

src/node_api.cc Outdated
CHECK_NEW_FROM_UTF8(env, name, p->utf8name);
v8::Local<v8::Name> property_name;

if (p->utf8name != nullptr) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Consider refactoring this into a helper method to get the property name from property descriptor

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Spent a bit doing this now. I want this helper to live in the v8impl namespace which is located above the helper macros like CHECK_NEW_FROM_UTF8 in the source. Looks like we'll need to move the helper macros above v8impl. Any reservations?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sounds fine to me. That's probably a better place for the macros anyway. Though you'll probably also need to move up some helper functions like napi_set_last_error() (or just declare them).

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Added this helper via e2519d7

Copy link
Contributor

@digitalinfinity digitalinfinity left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

src/node_api.cc Outdated

if (!define_maybe.FromMaybe(false)) {
return napi_set_last_error(env, napi_generic_failure);
}
} else if (p->getter || p->setter) {
} else if (p->getter != nullptr || p->setter != nullptr) {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I made this same change at #12240, though I also moved around some lines at the same time. Whoever goes in second will have to merge.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah, I noticed this change there, too. Fingers crossed it'll merge automatically.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

#12240 landed, so now this PR needs to be rebased.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Rebased.

@mhdawson
Copy link
Member

mhdawson commented Apr 7, 2017

This will need a rebase as well.

Change ```napi_callback``` to return ```napi_value``` directly instead of requiring ```napi_set_return_value```.

When we invoke the callback, we will check the return value and call ```SetReturnValue``` ourselves. If the callback returns ```NULL```, we don't set the return value in v8 which would have the same effect as previously if the callback didn't call ```napi_set_return_value```. Seems to be a more natural way to handle return values from callbacks. As a consequence, remove ```napi_set_return_value```.

Add a ```napi_value``` to ```napi_property_descriptor``` to support string values which couldn't be passed in the ```utf8name``` parameter or symbols as property names. Class names, however, cannot be symbols so this ```napi_value``` must be a string type in that case.

Remove all of the ```napi_callback_info``` helpers except for ```napi_get_cb_info``` and make all the parameters to ```napi_get_cb_info``` optional except for argc.

Update all the test collateral according to these changes. Also add ```test/addons-napi/common.h``` to house some common macros for wrapping N-API calls and error handling.
@boingoing boingoing force-pushed the napi_callback_return_napi_value branch from e2519d7 to 8f34553 Compare April 7, 2017 21:58
@boingoing
Copy link
Contributor Author

Rebased on top of node/master

Copy link
Member

@addaleax addaleax left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

src/ changes LGTM, somewhat rubber-stamp-y LGTM for the test parts ;)

src/node_api.cc Outdated
auto str_maybe = v8::String::NewFromUtf8( \
(env)->isolate, (str), v8::NewStringType::kInternalized, (len)); \
CHECK_MAYBE_EMPTY((env), str_maybe, napi_generic_failure); \
result = str_maybe.ToLocalChecked(); \
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can you also wrap result in parentheses here?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sure. We probably didn't do that originally because result is the LHS but looks like that works fine so I'll make the change.

src/node_api.cc Outdated
@@ -123,6 +187,22 @@ v8::Local<v8::Value> V8LocalValueFromJsValue(napi_value v) {
return local;
}

napi_status V8NameFromPropertyDescriptor(napi_env env,
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

How about either adding static inline here too (and for helpers in general), or – and I think I would prefer that – turning v8impl into an anonymous namespace?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I understand the benefit of static inline but why do you prefer an anonymous namespace? I think there's some value in clumping together the v8 helpers and naming that clump v8impl (though name could be more descriptive 👍). We might have other namespaces for other clumps of helpers like uvimpl, etc so having a relevant name helps to reason about these groups, no?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@boingoing Having an anon namespace would be nice because none of the symbols for the utilities here need to be exported, like static would indicate too. You could keep the v8impl/uvimpl namespaces and just wrap their entire content in a anon namespace if you like that for readability.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Also, just noticed, if you go with static inline can you re-align the arguments here? ;)

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Oh I see what you meant about the namespaces. I inferred your comment earlier to mean that you just preferred an unnamed namespace. I'd prefer to punt that particular change into a later PR since I want to get this and #12250 wrapped up early this week.

Is the argument alignment here good? With static inline at the beginning of the line, it's overall too long to have the arguments on subsequent lines align with the left-paren so I just pushed the next lines over to the right. Could also take some of static inline napi_status onto it's own line or align all the arguments by moving the first onto it's own line. Doesn't seem to be a hard rule about how this works and we're pretty inconsistent about it, in general, so I just picked what looked nice to me. If you feel strongly about this, though, I don't mind changing it. 👍

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If you feel strongly about this, though

I don’t, keep anything for later PRs that you want. :)

src/node_api.cc Outdated
v8::PropertyAttribute attributes =
v8impl::V8PropertyAttributesFromDescriptor(p);
v8impl::V8PropertyAttributesFromDescriptor(p);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is this intentional? We usually do 4-space indents for statement continuations

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Oh thanks for letting me know about that. I just thought this was an oversight.

src/node_api.cc Outdated
@@ -809,7 +838,7 @@ napi_status napi_define_class(napi_env env,
attributes);
} else if (p->method != nullptr) {
v8::Local<v8::Object> cbdata =
v8impl::CreateFunctionCallbackData(env, p->method, p->data);
v8impl::CreateFunctionCallbackData(env, p->method, p->data);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

(ditto)

size_t argc = 1;
napi_value args[1];
napi_value _this;
NAPI_CALL(env, napi_get_cb_info(env, info, &argc, args, &_this, NULL));
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can you prefer nullptr over NULL in C++ sources?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, makes sense.

@addaleax addaleax added this to the 8.0.0 milestone Apr 9, 2017
@boingoing
Copy link
Contributor Author

@addaleax I pushed 7d899eb which I believe addresses your feedback. Thanks for taking a look.

Copy link
Member

@mhdawson mhdawson left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@mhdawson
Copy link
Member

@mhdawson
Copy link
Member

lint errors

[node-test-linter] $ /bin/sh -xe /tmp/hudson8160929318737150329.sh
+ gmake lint-ci
./node tools/jslint.js -j 2 -f tap -o test-eslint.tap \
	benchmark lib test tools
test/addons-napi/6_object_wrap/myobject.cc:82:  Line ends in whitespace.  Consider deleting these extra spaces.  [whitespace/end_of_line] [4]
test/addons-napi/6_object_wrap/myobject.cc:110:  Line ends in whitespace.  Consider deleting these extra spaces.  [whitespace/end_of_line] [4]
test/addons-napi/7_factory_wrap/myobject.cc:82:  Line ends in whitespace.  Consider deleting these extra spaces.  [whitespace/end_of_line] [4]
test/addons-napi/7_factory_wrap/binding.cc:18:  Line ends in whitespace.  Consider deleting these extra spaces.  [whitespace/end_of_line] [4]
test/addons-napi/8_passing_wrapped/binding.cc:39:  Line ends in whitespace.  Consider deleting these extra spaces.  [whitespace/end_of_line] [4]
test/addons-napi/8_passing_wrapped/binding.cc:40:  Line ends in whitespace.  Consider deleting these extra spaces.  [whitespace/end_of_line] [4]
Total errors found: 6
gmake: *** [Makefile:884: cpplint] Error 1

please fix up and make sure to run make test

@boingoing
Copy link
Contributor Author

@mhdawson Did run vcbuild.cmd test but must have missed these latest whitespace issues. Can you please kick back off the CI for b47c9aa?

@addaleax
Copy link
Member

Fresh CI: https://ci.nodejs.org/job/node-test-commit/9006/

I assume this is ready to land once CI comes back okay?

@mhdawson
Copy link
Member

@addaleax yes should be ready to go. Would have landed earlier if the first CI run had passed.

@addaleax
Copy link
Member

Landed in ca786c3

(@nodejs/build … any idea what’s up with the osx CI not running?)

Also, for future reference: It would be cool if you could use n-api as the subsystem label in the commit message and wrap lines at 72 characters. And for inline code a single backtick is sufficient, the full ``` is only needed for multi-line blocks. ;)

@addaleax addaleax closed this Apr 10, 2017
addaleax pushed a commit that referenced this pull request Apr 10, 2017
Change `napi_callback` to return `napi_value` directly instead of
requiring `napi_set_return_value`.

When we invoke the callback, we will check the return value and
call `SetReturnValue` ourselves. If the callback returns `NULL`,
we don't set the return value in v8 which would have the same
effect as previously if the callback didn't call
`napi_set_return_value`. Seems to be a more natural way
to handle return values from callbacks. As a consequence,
remove `napi_set_return_value`.

Add a `napi_value` to `napi_property_descriptor` to support string
values which couldn't be passed in the `utf8name` parameter or
symbols as property names. Class names, however, cannot be symbols
so this `napi_value` must be a string type in that case.

Remove all of the `napi_callback_info` helpers except for
`napi_get_cb_info` and make all the parameters to
`napi_get_cb_info` optional except for argc.

Update all the test collateral according to these changes.
Also add `test/addons-napi/common.h` to house some common macros
for wrapping N-API calls and error handling.

PR-URL: #12248
Reviewed-By: Michael Dawson <michael_dawson@ca.ibm.com>
Reviewed-By: Anna Henningsen <anna@addaleax.net>
@boingoing
Copy link
Contributor Author

@addaleax Thanks very much for your help with this PR!

I used napi to mark the subsystem because I saw another PR use the same before and didn't know if we had settled on a proper name. Will adjust future PRs, thanks. 🆗

I tried really hard to make the commit message match the requirements so sorry if I got that wrong - does the 72 character limit include some spaces at the beginning which I didn't add? Do the commit message rules apply to all of the commits or just the final squashed commit (which is what I imagine you actually pushed) and did you pull the message from the PR or the commits themselves?

Also, yeah, I actually didn't realize one backtick worked for inline code until recently since I tend to use multi-line code blocks with syntax highlighting.

@addaleax
Copy link
Member

does the 72 character limit include some spaces at the beginning which I didn't add?

No, but maybe something got messed up while editing the commit message? Most of the lines in 8f34553 are longer than 72 chars (git show 8f34553 should show that)

Do the commit message rules apply to all of the commits or just the final squashed commit (which is what I imagine you actually pushed) and did you pull the message from the PR or the commits themselves?

Heh, looks like I should have pulled it from the PR description. ;) But yes, just the final squashed commit(s) that get merged. What you do inside the PR is completely up to you – rebase, squash, amend, or add commits in any way that you think makes sense. :)

@jasnell jasnell mentioned this pull request May 11, 2017
XadillaX added a commit to XadillaX/node that referenced this pull request Jun 9, 2017
The earlier version `napi_callback` returns `void` but now is
`napi_value`. The document of this section hasn't been modified.

Fixes: nodejs#12248
jasnell pushed a commit that referenced this pull request Jun 13, 2017
The earlier version `napi_callback` returns `void` but now is
`napi_value`. The document of this section hasn't been modified.

PR-URL: #13570
Fixes: #12248
Fixes: #13562
Reviewed-By: Gibson Fahnestock <gibfahn@gmail.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Jason Ginchereau <jasongin@microsoft.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
addaleax pushed a commit that referenced this pull request Jun 17, 2017
The earlier version `napi_callback` returns `void` but now is
`napi_value`. The document of this section hasn't been modified.

PR-URL: #13570
Fixes: #12248
Fixes: #13562
Reviewed-By: Gibson Fahnestock <gibfahn@gmail.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Jason Ginchereau <jasongin@microsoft.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
addaleax pushed a commit that referenced this pull request Jun 21, 2017
The earlier version `napi_callback` returns `void` but now is
`napi_value`. The document of this section hasn't been modified.

PR-URL: #13570
Fixes: #12248
Fixes: #13562
Reviewed-By: Gibson Fahnestock <gibfahn@gmail.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Jason Ginchereau <jasongin@microsoft.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
gabrielschulhof pushed a commit to gabrielschulhof/node that referenced this pull request Apr 10, 2018
Change `napi_callback` to return `napi_value` directly instead of
requiring `napi_set_return_value`.

When we invoke the callback, we will check the return value and
call `SetReturnValue` ourselves. If the callback returns `NULL`,
we don't set the return value in v8 which would have the same
effect as previously if the callback didn't call
`napi_set_return_value`. Seems to be a more natural way
to handle return values from callbacks. As a consequence,
remove `napi_set_return_value`.

Add a `napi_value` to `napi_property_descriptor` to support string
values which couldn't be passed in the `utf8name` parameter or
symbols as property names. Class names, however, cannot be symbols
so this `napi_value` must be a string type in that case.

Remove all of the `napi_callback_info` helpers except for
`napi_get_cb_info` and make all the parameters to
`napi_get_cb_info` optional except for argc.

Update all the test collateral according to these changes.
Also add `test/addons-napi/common.h` to house some common macros
for wrapping N-API calls and error handling.

PR-URL: nodejs#12248
Reviewed-By: Michael Dawson <michael_dawson@ca.ibm.com>
Reviewed-By: Anna Henningsen <anna@addaleax.net>
gabrielschulhof pushed a commit to gabrielschulhof/node that referenced this pull request Apr 10, 2018
The earlier version `napi_callback` returns `void` but now is
`napi_value`. The document of this section hasn't been modified.

PR-URL: nodejs#13570
Fixes: nodejs#12248
Fixes: nodejs#13562
Reviewed-By: Gibson Fahnestock <gibfahn@gmail.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Jason Ginchereau <jasongin@microsoft.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
MylesBorins pushed a commit that referenced this pull request Apr 16, 2018
Change `napi_callback` to return `napi_value` directly instead of
requiring `napi_set_return_value`.

When we invoke the callback, we will check the return value and
call `SetReturnValue` ourselves. If the callback returns `NULL`,
we don't set the return value in v8 which would have the same
effect as previously if the callback didn't call
`napi_set_return_value`. Seems to be a more natural way
to handle return values from callbacks. As a consequence,
remove `napi_set_return_value`.

Add a `napi_value` to `napi_property_descriptor` to support string
values which couldn't be passed in the `utf8name` parameter or
symbols as property names. Class names, however, cannot be symbols
so this `napi_value` must be a string type in that case.

Remove all of the `napi_callback_info` helpers except for
`napi_get_cb_info` and make all the parameters to
`napi_get_cb_info` optional except for argc.

Update all the test collateral according to these changes.
Also add `test/addons-napi/common.h` to house some common macros
for wrapping N-API calls and error handling.

Backport-PR-URL: #19447
PR-URL: #12248
Reviewed-By: Michael Dawson <michael_dawson@ca.ibm.com>
Reviewed-By: Anna Henningsen <anna@addaleax.net>
MylesBorins pushed a commit that referenced this pull request Apr 16, 2018
The earlier version `napi_callback` returns `void` but now is
`napi_value`. The document of this section hasn't been modified.

Backport-PR-URL: #19447
PR-URL: #13570
Fixes: #12248
Fixes: #13562
Reviewed-By: Gibson Fahnestock <gibfahn@gmail.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Jason Ginchereau <jasongin@microsoft.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
@MylesBorins MylesBorins mentioned this pull request Apr 16, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
c++ Issues and PRs that require attention from people who are familiar with C++. node-api Issues and PRs related to the Node-API.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

8 participants