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

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

@boingoing

This comment has been minimized.

Copy link
Contributor Author

commented Apr 6, 2017

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

@mscdex mscdex added the n-api label Apr 6, 2017

test/addons-napi/2_function_arguments/binding.c Outdated
@@ -1,58 +1,37 @@
#include <node_api.h>
#include "..\common.h"

This comment has been minimized.

Copy link
@addaleax
test/addons-napi/3_callbacks/binding.c Outdated
@@ -1,51 +1,43 @@
#include <node_api.h>
#include "..\common.h"

This comment has been minimized.

Copy link
@addaleax

addaleax Apr 6, 2017

Member

ditto, same everywhere

This comment has been minimized.

Copy link
@boingoing

boingoing Apr 6, 2017

Author Contributor

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

This comment has been minimized.

Copy link
@boingoing

boingoing Apr 6, 2017

Author Contributor

Addressed in a new commit - 03c0644

test/addons-napi/common.h Outdated
#define NAPI_RETVAL_NOTHING // Intentionally blank #define

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

This comment has been minimized.

Copy link
@mhdawson

mhdawson Apr 6, 2017

Member

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

This comment has been minimized.

Copy link
@boingoing

boingoing Apr 6, 2017

Author Contributor

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?

This comment has been minimized.

Copy link
@mhdawson

mhdawson Apr 6, 2017

Member

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.

This comment has been minimized.

Copy link
@mhdawson

mhdawson Apr 6, 2017

Member

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 ?

This comment has been minimized.

Copy link
@boingoing

boingoing Apr 6, 2017

Author Contributor

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.

This comment has been minimized.

Copy link
@mhdawson

mhdawson Apr 6, 2017

Member

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.

This comment has been minimized.

Copy link
@boingoing

boingoing Apr 6, 2017

Author Contributor

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) {

This comment has been minimized.

Copy link
@digitalinfinity

digitalinfinity Apr 6, 2017

Contributor

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

This comment has been minimized.

Copy link
@boingoing

boingoing Apr 6, 2017

Author Contributor

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?

This comment has been minimized.

Copy link
@jasongin

jasongin Apr 6, 2017

Contributor

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).

This comment has been minimized.

Copy link
@boingoing

boingoing Apr 7, 2017

Author Contributor

Added this helper via e2519d7

@digitalinfinity
Copy link
Contributor

left a comment

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) {

This comment has been minimized.

Copy link
@jasongin

jasongin Apr 6, 2017

Contributor

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.

This comment has been minimized.

Copy link
@boingoing

boingoing Apr 6, 2017

Author Contributor

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

This comment has been minimized.

Copy link
@jasongin

jasongin Apr 7, 2017

Contributor

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

This comment has been minimized.

Copy link
@boingoing

boingoing Apr 7, 2017

Author Contributor

Rebased.

@mhdawson

This comment has been minimized.

Copy link
Member

commented Apr 7, 2017

This will need a rebase as well.

napi: change napi_callback to return napi_value
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 boingoing:napi_callback_return_napi_value branch to 8f34553 Apr 7, 2017

@boingoing

This comment has been minimized.

Copy link
Contributor Author

commented Apr 7, 2017

Rebased on top of node/master

@boingoing boingoing referenced this pull request Apr 8, 2017
3 of 3 tasks complete
@addaleax
Copy link
Member

left a comment

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

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(); \

This comment has been minimized.

Copy link
@addaleax

addaleax Apr 9, 2017

Member

Can you also wrap result in parentheses here?

This comment has been minimized.

Copy link
@boingoing

boingoing Apr 9, 2017

Author Contributor

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.

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

napi_status V8NameFromPropertyDescriptor(napi_env env,

This comment has been minimized.

Copy link
@addaleax

addaleax Apr 9, 2017

Member

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?

This comment has been minimized.

Copy link
@boingoing

boingoing Apr 9, 2017

Author Contributor

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?

This comment has been minimized.

Copy link
@addaleax

addaleax Apr 10, 2017

Member

@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.

This comment has been minimized.

Copy link
@addaleax

addaleax Apr 10, 2017

Member

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

This comment has been minimized.

Copy link
@boingoing

boingoing Apr 10, 2017

Author Contributor

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. 👍

This comment has been minimized.

Copy link
@addaleax

addaleax Apr 10, 2017

Member

If you feel strongly about this, though

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

v8::PropertyAttribute attributes =
v8impl::V8PropertyAttributesFromDescriptor(p);
v8impl::V8PropertyAttributesFromDescriptor(p);

This comment has been minimized.

Copy link
@addaleax

addaleax Apr 9, 2017

Member

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

This comment has been minimized.

Copy link
@boingoing

boingoing Apr 9, 2017

Author Contributor

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

@@ -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);

This comment has been minimized.

Copy link
@addaleax

addaleax Apr 9, 2017

Member

(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));

This comment has been minimized.

Copy link
@addaleax

addaleax Apr 9, 2017

Member

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

This comment has been minimized.

Copy link
@boingoing

boingoing Apr 9, 2017

Author Contributor

Yes, makes sense.

@addaleax addaleax added this to the 8.0.0 milestone Apr 9, 2017

@boingoing

This comment has been minimized.

Copy link
Contributor Author

commented Apr 9, 2017

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

@mhdawson
Copy link
Member

left a comment

LGTM

@mhdawson

This comment has been minimized.

Copy link
Member

commented Apr 10, 2017

@mhdawson

This comment has been minimized.

Copy link
Member

commented Apr 10, 2017

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

This comment has been minimized.

Copy link
Contributor Author

commented Apr 10, 2017

@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

This comment has been minimized.

Copy link
Member

commented Apr 10, 2017

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

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

@mhdawson

This comment has been minimized.

Copy link
Member

commented Apr 10, 2017

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

@addaleax

This comment has been minimized.

Copy link
Member

commented Apr 10, 2017

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 added a commit that referenced this pull request Apr 10, 2017
n-api: change napi_callback to return napi_value
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

This comment has been minimized.

Copy link
Contributor Author

commented Apr 10, 2017

@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

This comment has been minimized.

Copy link
Member

commented Apr 10, 2017

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 referenced this pull request May 11, 2017
XadillaX added a commit to XadillaX/node that referenced this pull request Jun 9, 2017
doc: fix out of date napi_callback doc
The earlier version `napi_callback` returns `void` but now is
`napi_value`. The document of this section hasn't been modified.

Fixes: nodejs#12248
@XadillaX XadillaX referenced this pull request Jun 9, 2017
3 of 3 tasks complete
jasnell added a commit that referenced this pull request Jun 13, 2017
doc: fix out of date napi_callback doc
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 added a commit that referenced this pull request Jun 17, 2017
doc: fix out of date napi_callback doc
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 added a commit that referenced this pull request Jun 17, 2017
doc: fix out of date napi_callback doc
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 added a commit that referenced this pull request Jun 21, 2017
doc: fix out of date napi_callback doc
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 added a commit to gabrielschulhof/node that referenced this pull request Mar 11, 2018
n-api: change napi_callback to return napi_value
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 added a commit to gabrielschulhof/node that referenced this pull request Mar 12, 2018
doc: fix out of date napi_callback doc
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>
gabrielschulhof added a commit to gabrielschulhof/node that referenced this pull request Mar 15, 2018
doc: fix out of date napi_callback doc
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>
gabrielschulhof added a commit to gabrielschulhof/node that referenced this pull request Mar 31, 2018
n-api: change napi_callback to return napi_value
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 added a commit to gabrielschulhof/node that referenced this pull request Mar 31, 2018
doc: fix out of date napi_callback doc
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>
gabrielschulhof added a commit to gabrielschulhof/node that referenced this pull request Apr 6, 2018
doc: fix out of date napi_callback doc
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>
gabrielschulhof added a commit to gabrielschulhof/node that referenced this pull request Apr 10, 2018
n-api: change napi_callback to return napi_value
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 added a commit to gabrielschulhof/node that referenced this pull request Apr 10, 2018
doc: fix out of date napi_callback doc
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 added a commit that referenced this pull request Apr 16, 2018
n-api: change napi_callback to return napi_value
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 added a commit that referenced this pull request Apr 16, 2018
doc: fix out of date napi_callback doc
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 referenced this pull request Apr 16, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
8 participants
You can’t perform that action at this time.