Skip to content

napi: Remove napi_is_construct_call and introduce napi_get_new_target #14698

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.

Sign up for GitHub

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

Jump to bottom
Closed
wants to merge 12 commits into from
Closed

napi: Remove napi_is_construct_call and introduce napi_get_new_target #14698

wants to merge 12 commits into from

Conversation

sampsongao
Copy link

@sampsongao sampsongao commented Aug 8, 2017 •
edited
Loading

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)

@nodejs-github-bot 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 sampsongao mentioned this pull request Aug 8, 2017
Closed
addaleax
addaleax reviewed Aug 8, 2017
View reviewed changes
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.

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.

src/node_api.cc Outdated
Copy link
Member

Choose a reason for hiding this comment

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

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

Copy link
Author

Choose a reason for hiding this comment

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

This is a typo, will fix it

jasongin
jasongin reviewed Aug 8, 2017
View reviewed changes
src/node_api.cc Outdated
Copy link
Member

Choose a reason for hiding this comment

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

Can the IsConstructCall() methods be deleted?

jasongin
jasongin reviewed Aug 8, 2017
View reviewed changes
src/node_api.cc Outdated
Copy link
Member

Choose a reason for hiding this comment

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

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

jasongin
jasongin reviewed Aug 8, 2017
View reviewed changes
src/node_api.h Outdated
Copy link
Member

Choose a reason for hiding this comment

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

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

jasongin
jasongin suggested changes Aug 8, 2017
View reviewed changes
Copy link
Member

@jasongin jasongin left a comment

Choose a reason for hiding this comment

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

Left some comments. Mainly the doc needs updating.

kfarnung
kfarnung reviewed Aug 8, 2017
View reviewed changes
test/addons-napi/6_object_wrap/myobject.cc Outdated
Copy link
Contributor

Choose a reason for hiding this comment

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

nit: Since this is C++ code use nullptr

jasongin
jasongin reviewed Aug 8, 2017
View reviewed changes
doc/api/n-api.md Outdated
Copy link
Member

Choose a reason for hiding this comment

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

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

Copy link
Contributor

Choose a reason for hiding this comment

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

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
cjihrig reviewed Aug 8, 2017
View reviewed changes
doc/api/n-api.md Outdated
Copy link
Contributor

Choose a reason for hiding this comment

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

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.

src/node_api.cc Outdated
Copy link
Contributor

Choose a reason for hiding this comment

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

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

Copy link
Author

Choose a reason for hiding this comment

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

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

jasongin
jasongin reviewed Aug 8, 2017
View reviewed changes
doc/api/n-api.md Outdated
Copy link
Member

Choose a reason for hiding this comment

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

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

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

mhdawson
mhdawson approved these changes Aug 15, 2017
View reviewed changes
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

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

@mhdawson
Copy link
Member

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 sampsongao mentioned this pull request Aug 21, 2017
Closed
@sampsongao
Copy link
Author

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

@BridgeAR
Copy link
Member

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

@BridgeAR
Copy link
Member

BridgeAR commented Sep 8, 2017

Ping @mhdawson

@mhdawson
Copy link
Member

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

@mhdawson
Copy link
Member

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

@mhdawson
Copy link
Member

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

@mhdawson
Copy link
Member

Linux failure looks unrelated, opened #15416

@mhdawson
Copy link
Member

mhdawson commented Sep 14, 2017 •
edited
Loading

ARM failure said this

ccache: error: x_calloc: Could not allocate 40 bytes

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

@mhdawson
Copy link
Member

Landed as 973c12f

@mhdawson mhdawson closed this Sep 14, 2017
mhdawson pushed a commit that referenced this pull request Sep 14, 2017
@mhdawson
n-api: napi_is_construct_call->napi_get_new_target
973c12f 
Remove napi_is_construct_call and introduce napi_get_new_target.

PR-URL: #14698
Reviewed-By: Jason Ginchereau <[email protected]>
Reviewed-By: Colin Ihrig <[email protected]>
Reviewed-By: Michael Dawson <[email protected]>
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Kyle Farnung <[email protected]>
addaleax pushed a commit to addaleax/ayo that referenced this pull request Sep 17, 2017
@addaleax
n-api: napi_is_construct_call->napi_get_new_target
a145c52 
Remove napi_is_construct_call and introduce napi_get_new_target.

PR-URL: nodejs/node#14698
Reviewed-By: Jason Ginchereau <[email protected]>
Reviewed-By: Colin Ihrig <[email protected]>
Reviewed-By: Michael Dawson <[email protected]>
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Kyle Farnung <[email protected]>
jasnell pushed a commit that referenced this pull request Sep 20, 2017
@jasnell
n-api: napi_is_construct_call->napi_get_new_target
a2d340f 
Remove napi_is_construct_call and introduce napi_get_new_target.

PR-URL: #14698
Reviewed-By: Jason Ginchereau <[email protected]>
Reviewed-By: Colin Ihrig <[email protected]>
Reviewed-By: Michael Dawson <[email protected]>
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Kyle Farnung <[email protected]>
@jasnell jasnell mentioned this pull request Sep 20, 2017
Merged
Qard pushed a commit to Qard/ayo that referenced this pull request Sep 21, 2017
n-api: napi_is_construct_call->napi_get_new_target
79cc4c9 
Remove napi_is_construct_call and introduce napi_get_new_target.

PR-URL: nodejs/node#14698
Reviewed-By: Jason Ginchereau <[email protected]>
Reviewed-By: Colin Ihrig <[email protected]>
Reviewed-By: Michael Dawson <[email protected]>
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Kyle Farnung <[email protected]>
gabrielschulhof pushed a commit to gabrielschulhof/node that referenced this pull request Apr 16, 2018
n-api: napi_is_construct_call->napi_get_new_target
574f09e 
Remove napi_is_construct_call and introduce napi_get_new_target.

PR-URL: nodejs#14698
Reviewed-By: Jason Ginchereau <[email protected]>
Reviewed-By: Colin Ihrig <[email protected]>
Reviewed-By: Michael Dawson <[email protected]>
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Kyle Farnung <[email protected]>
MylesBorins pushed a commit that referenced this pull request Apr 16, 2018
@MylesBorins
n-api: napi_is_construct_call->napi_get_new_target
fe87a59 
Remove napi_is_construct_call and introduce napi_get_new_target.

Backport-PR-URL: #19447
PR-URL: #14698
Reviewed-By: Jason Ginchereau <[email protected]>
Reviewed-By: Colin Ihrig <[email protected]>
Reviewed-By: Michael Dawson <[email protected]>
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Kyle Farnung <[email protected]>
@MylesBorins MylesBorins mentioned this pull request Apr 16, 2018
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@addaleax addaleax addaleax left review comments

@jasnell jasnell jasnell approved these changes

@cjihrig cjihrig cjihrig approved these changes

@mhdawson mhdawson mhdawson approved these changes

+2 more reviewers

@kfarnung kfarnung kfarnung approved these changes

@jasongin jasongin jasongin approved these changes

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

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

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

10 participants

@sampsongao @mhdawson @BridgeAR @jasnell @addaleax @kfarnung @cjihrig @jasongin @MylesBorins @nodejs-github-bot