napi: change napi_callback to return napi_value #12248
Conversation
|
@jasongin @digitalinfinity Please take a look at this change if you have the time. |
There was a problem hiding this comment.
Ah yup. Sorry, dev'd this on Windows. :) Thanks for catching.
There was a problem hiding this comment.
Addressed in a new commit - 03c0644
test/addons-napi/common.h
Outdated
There was a problem hiding this comment.
Should this check if the error is reporting an Exception. IF so we probably want to rethrow that instead of throwing a new Error.
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
updating to leave as pending is the equivalent and makes the most sense to me.
src/node_api.h
Outdated
There was a problem hiding this comment.
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 ?
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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
There was a problem hiding this comment.
Consider refactoring this into a helper method to get the property name from property descriptor
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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).
There was a problem hiding this comment.
Added this helper via e2519d7
src/node_api.cc
Outdated
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
Yeah, I noticed this change there, too. Fingers crossed it'll merge automatically.
|
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.
e2519d7 to
8f34553
Compare
|
Rebased on top of node/master |
There was a problem hiding this comment.
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(); \ |
There was a problem hiding this comment.
Can you also wrap result in parentheses here?
There was a problem hiding this comment.
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
| return local; | ||
| } | ||
|
|
||
| napi_status V8NameFromPropertyDescriptor(napi_env env, |
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
@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.
There was a problem hiding this comment.
Also, just noticed, if you go with static inline can you re-align the arguments here? ;)
There was a problem hiding this comment.
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. 👍
There was a problem hiding this comment.
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); |
There was a problem hiding this comment.
Is this intentional? We usually do 4-space indents for statement continuations
There was a problem hiding this comment.
Oh thanks for letting me know about that. I just thought this was an oversight.
src/node_api.cc
Outdated
| } else if (p->method != nullptr) { | ||
| v8::Local<v8::Object> cbdata = | ||
| v8impl::CreateFunctionCallbackData(env, p->method, p->data); | ||
| v8impl::CreateFunctionCallbackData(env, p->method, p->data); |
| size_t argc = 1; | ||
| napi_value args[1]; | ||
| napi_value _this; | ||
| NAPI_CALL(env, napi_get_cb_info(env, info, &argc, args, &_this, NULL)); |
There was a problem hiding this comment.
Can you prefer nullptr over NULL in C++ sources?
|
lint errors please fix up and make sure to run make test |
|
Fresh CI: https://ci.nodejs.org/job/node-test-commit/9006/ I assume this is ready to land once CI comes back okay? |
|
@addaleax yes should be ready to go. Would have landed earlier if the first CI run had passed. |
|
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 |
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>
|
@addaleax Thanks very much for your help with this PR! I used 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. |
No, but maybe something got messed up while editing the commit message? Most of the lines in 8f34553 are longer than 72 chars (
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. :) |
The earlier version `napi_callback` returns `void` but now is `napi_value`. The document of this section hasn't been modified. Fixes: nodejs#12248
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>
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>
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>
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>
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>
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>
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>
Change
napi_callbackto returnnapi_valuedirectly instead ofrequiring module code to call
napi_set_return_value.When we invoke the callback, we will check the return value and call
SetReturnValueourselves. If the callback returnsNULL, we don'tset the return value in v8 which would have the same effect as
previously if the callback didn't call
napi_set_return_value. Seemsto be a more natural way to handle return values from callbacks. As a
consequence, remove
napi_set_return_value.Add a
napi_valuetonapi_property_descriptorto supportstring values which couldn't be passed in the
utf8nameparameteror symbols as property names. Class names, however, cannot be symbols
so this
napi_valuemust be a string type in that case.Remove all of the
napi_callback_infohelpers except fornapi_get_cb_infoand make all the parameters tonapi_get_cb_infooptional. If
argvis provided,argcwill become non-optional.Update all the test collateral according to these changes. Also add
test/addons-napi/common.hto house some common macros for wrappingN-API calls and error handling.
Checklist
make -j4 test(UNIX), orvcbuild test(Windows) passesAffected core subsystem(s)
napi