napi: Remove napi_is_construct_call and introduce napi_get_new_target #14698

sampsongao · 2017-08-08T20:29:18Z

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)

addaleax

Thank you! Can you also change the documentation?

Also, it would be great if you could try to fit the commit message into 50 characters (starting with n-api: ), otherwise somebody can do that while landing.

addaleax · 2017-08-08T20:30:47Z

src/node_api.cc

  CHECK_ENV(env);
  CHECK_ARG(env, cbinfo);
  CHECK_ARG(env, result);

  v8impl::CallbackWrapper* info =
-      reinterpret_cast<v8impl::CallbackWrapper*>(cbinfo);
+       reinterpret_cast<v8impl::CallbackWrapper*>(cbinfo);


nit: This is a bit pedantic but we do use 4-space indentation for statement continuations, this can stay as it was.

This is a typo, will fix it

jasongin · 2017-08-08T20:32:52Z

src/node_api.cc

@@ -439,11 +439,13 @@ class CallbackWrapper {
      : _this(this_arg), _args_length(args_length), _data(data) {}

  virtual bool IsConstructCall() = 0;


Can the IsConstructCall() methods be deleted?

jasongin · 2017-08-08T20:35:51Z

src/node_api.cc

  virtual void Args(napi_value* buffer, size_t bufferlength) = 0;
  virtual void SetReturnValue(napi_value value) = 0;

  napi_value This() { return _this; }

+


Remove the extra blank line. I think the linter will complain anyway.

jasongin · 2017-08-08T20:36:07Z

src/node_api.h

-NAPI_EXTERN napi_status napi_is_construct_call(napi_env env,
-                                               napi_callback_info cbinfo,
-                                               bool* result);
+NAPI_EXTERN napi_status napi_get_new_target(napi_env env,


Need to also update doc\api\n-api.md.

jasongin

Left some comments. Mainly the doc needs updating.

kfarnung · 2017-08-08T20:46:19Z

test/addons-napi/6_object_wrap/myobject.cc

-  NAPI_CALL(env, napi_is_construct_call(env, info, &is_constructor));
+  napi_value new_target;
+  NAPI_CALL(env, napi_get_new_target(env, info, &new_target));
+  bool is_constructor = ( new_target != NULL );


nit: Since this is C++ code use nullptr

jasongin · 2017-08-08T21:34:21Z

doc/api/n-api.md

 <!-- YAML
-added: v8.0.0
+added: v9.0.0


Use REPLACEME instead of a specific version. Whoever does a release will then update it to the correct version. (This will be in some later 8.x release.)

I don't know if it's worth it since n-api is still unofficial, but we could also make a function level changelog entry stating that it was renamed.

cjihrig · 2017-08-08T21:36:14Z

doc/api/n-api.md

 <!-- YAML
-added: v8.0.0
+added: v9.0.0


I don't know if it's worth it since n-api is still unofficial, but we could also make a function level changelog entry stating that it was renamed.

cjihrig · 2017-08-08T21:37:04Z

src/node_api.cc

  napi_value NewTarget() override {
-    return v8impl::JsValueFromV8LocalValue(_cbinfo.NewTarget());
+    if (_cbinfo.IsConstructCall()) {


Can you remove the curly braces on the if-else.

I think that it is better to be consistent with the rest of the file.

jasongin · 2017-08-08T21:39:37Z

doc/api/n-api.md

-This API checks if the the current callback was due to a
-consructor call.
+This API return the `new.target` of the construct call. If the current
+callback is not a constructor call, the result equals to `nulltpr`.


Replace "construct" with "constructor" here and above.

Fix grammar: "This API returns the new.target" ... "the result is nullptr."

mhdawson

LGTM

mhdawson · 2017-08-15T14:04:34Z

CI run: https://ci.nodejs.org/job/node-test-pull-request/9673/

…arget

mhdawson · 2017-08-18T14:12:30Z

AIX is known unrelated issue
ARM was problem in building and unrelated.

We are good to land, but I want to make sure we are ready with required changes in wrapper and ported modules before we land.

sampsongao · 2017-08-21T19:35:18Z

For node-sass: sampsongao/node-sass@9338267

BridgeAR · 2017-08-31T17:20:18Z

@mhdawson are you going to check for the required changes? If so, would you mind assigning yourself to the PR?

BridgeAR · 2017-09-08T03:04:59Z

Ping @mhdawson

mhdawson · 2017-09-14T13:23:43Z

Assigned to myself, will land along with other breaking changes.

mhdawson · 2017-09-14T15:23:52Z

New CI run since its been a while: https://ci.nodejs.org/job/node-test-pull-request/10080/

mhdawson · 2017-09-14T20:11:13Z

#14697 landed, going to check ci results and land if ok.

mhdawson · 2017-09-14T20:15:15Z

Linux failure looks unrelated, opened #15416

mhdawson · 2017-09-14T20:16:34Z

ARM failure said this

ccache: error: x_calloc: Could not allocate 40 bytes

so believe its unrelated as well, opened nodejs/build#884

mhdawson · 2017-09-14T20:29:10Z

Landed as 973c12f

Remove napi_is_construct_call and introduce napi_get_new_target. PR-URL: #14698 Reviewed-By: Jason Ginchereau <jasongin@microsoft.com> Reviewed-By: Colin Ihrig <cjihrig@gmail.com> Reviewed-By: Michael Dawson <michael_dawson@ca.ibm.com> Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Kyle Farnung <kfarnung@microsoft.com>

Remove napi_is_construct_call and introduce napi_get_new_target. PR-URL: nodejs/node#14698 Reviewed-By: Jason Ginchereau <jasongin@microsoft.com> Reviewed-By: Colin Ihrig <cjihrig@gmail.com> Reviewed-By: Michael Dawson <michael_dawson@ca.ibm.com> Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Kyle Farnung <kfarnung@microsoft.com>

Remove napi_is_construct_call and introduce napi_get_new_target. PR-URL: #14698 Reviewed-By: Jason Ginchereau <jasongin@microsoft.com> Reviewed-By: Colin Ihrig <cjihrig@gmail.com> Reviewed-By: Michael Dawson <michael_dawson@ca.ibm.com> Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Kyle Farnung <kfarnung@microsoft.com>

Remove napi_is_construct_call and introduce napi_get_new_target. PR-URL: nodejs/node#14698 Reviewed-By: Jason Ginchereau <jasongin@microsoft.com> Reviewed-By: Colin Ihrig <cjihrig@gmail.com> Reviewed-By: Michael Dawson <michael_dawson@ca.ibm.com> Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Kyle Farnung <kfarnung@microsoft.com>

Remove napi_is_construct_call and introduce napi_get_new_target. PR-URL: nodejs#14698 Reviewed-By: Jason Ginchereau <jasongin@microsoft.com> Reviewed-By: Colin Ihrig <cjihrig@gmail.com> Reviewed-By: Michael Dawson <michael_dawson@ca.ibm.com> Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Kyle Farnung <kfarnung@microsoft.com>

Remove napi_is_construct_call and introduce napi_get_new_target. Backport-PR-URL: #19447 PR-URL: #14698 Reviewed-By: Jason Ginchereau <jasongin@microsoft.com> Reviewed-By: Colin Ihrig <cjihrig@gmail.com> Reviewed-By: Michael Dawson <michael_dawson@ca.ibm.com> Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Kyle Farnung <kfarnung@microsoft.com>

nodejs-github-bot added c++ Issues and PRs that require attention from people who are familiar with C++. node-api Issues and PRs related to the Node-API. labels Aug 8, 2017

sampsongao mentioned this pull request Aug 8, 2017

Consider "new.target" for constructor callbacks nodejs/abi-stable-node#200

Closed

addaleax reviewed Aug 8, 2017

View reviewed changes

jasongin reviewed Aug 8, 2017

View reviewed changes

jasongin suggested changes Aug 8, 2017

View reviewed changes

kfarnung reviewed Aug 8, 2017

View reviewed changes

jasongin reviewed Aug 8, 2017

View reviewed changes

cjihrig reviewed Aug 8, 2017

View reviewed changes

jasongin reviewed Aug 8, 2017

View reviewed changes

jasongin approved these changes Aug 8, 2017

View reviewed changes

kfarnung approved these changes Aug 8, 2017

View reviewed changes

cjihrig approved these changes Aug 13, 2017

View reviewed changes

mhdawson approved these changes Aug 15, 2017

View reviewed changes

jasnell approved these changes Aug 16, 2017

View reviewed changes

Sampson Gao added 12 commits August 16, 2017 15:10

Remove napi_is_construct_call and introduce napi_get_new_target

f41fdd1

Fix indentation typo in node_api.cc

c99c54d

Remove extra line

b1a24a9

Remove CallbackWrapper.IsConstructCall

108fe66

Change NULL to nullptr in addons-napi test

9b83e22

Update n-api doc for napi_get_new_target

6fe53be

Check is construct call before calling v8::FunctionCallbackInfo::NewT…

f8397c6

…arget

Fix grammar in doc

2d830fc

Use REPLACEME instead of version number

ec67e17

Rephase using constructor call

b733c78

Fix typo in doc

f6ec28b

Fix linter errors

57e8976

sampsongao mentioned this pull request Aug 21, 2017

Replace napi_is_construct_call with napi_get_new_target nodejs/node-addon-api#113

Closed

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

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

napi: Remove napi_is_construct_call and introduce napi_get_new_target #14698

napi: Remove napi_is_construct_call and introduce napi_get_new_target #14698

sampsongao commented Aug 8, 2017 •

edited

addaleax left a comment

addaleax Aug 8, 2017

sampsongao Aug 8, 2017

jasongin Aug 8, 2017

jasongin Aug 8, 2017

jasongin Aug 8, 2017

jasongin left a comment

kfarnung Aug 8, 2017

jasongin Aug 8, 2017

cjihrig Aug 8, 2017

cjihrig Aug 8, 2017

cjihrig Aug 8, 2017

sampsongao Aug 8, 2017

jasongin Aug 8, 2017

mhdawson left a comment

mhdawson commented Aug 15, 2017

mhdawson commented Aug 18, 2017

sampsongao commented Aug 21, 2017

BridgeAR commented Aug 31, 2017

BridgeAR commented Sep 8, 2017

mhdawson commented Sep 14, 2017

mhdawson commented Sep 14, 2017

mhdawson commented Sep 14, 2017

mhdawson commented Sep 14, 2017

mhdawson commented Sep 14, 2017 •

edited

mhdawson commented Sep 14, 2017

		@@ -439,11 +439,13 @@ class CallbackWrapper {
		: _this(this_arg), _args_length(args_length), _data(data) {}

		virtual bool IsConstructCall() = 0;

napi: Remove napi_is_construct_call and introduce napi_get_new_target #14698

napi: Remove napi_is_construct_call and introduce napi_get_new_target #14698

Conversation

sampsongao commented Aug 8, 2017 • edited

Checklist

Affected core subsystem(s)

addaleax 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

jasongin 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

mhdawson left a comment

Choose a reason for hiding this comment

mhdawson commented Aug 15, 2017

mhdawson commented Aug 18, 2017

sampsongao commented Aug 21, 2017

BridgeAR commented Aug 31, 2017

BridgeAR commented Sep 8, 2017

mhdawson commented Sep 14, 2017

mhdawson commented Sep 14, 2017

mhdawson commented Sep 14, 2017

mhdawson commented Sep 14, 2017

mhdawson commented Sep 14, 2017 • edited

mhdawson commented Sep 14, 2017

sampsongao commented Aug 8, 2017 •

edited

mhdawson commented Sep 14, 2017 •

edited