GeezerVison

The World Wide Web, restored to 800 pixels of glory.

GeezerVison is showing github.com — napi: Remove napi_is_construct_call and introduce napi_get_new_target by sampsongao · Pull Request #14698 · nodejs/node

Conversation

sampsongao commented Aug 8, 2017 •

edited

Loading

Uh oh!

There was an error while loading. Please reload this page.

Copy link

Copy Markdown

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)

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions

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

addaleax left a comment

Copy link

Copy Markdown

Member

There was a problem hiding this 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.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions

Comment thread

src/node_api.cc Outdated

addaleax Aug 8, 2017

Copy link

Copy Markdown

Member

There was a problem hiding this comment.

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.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions

sampsongao Aug 8, 2017

Copy link

Copy Markdown

Author

There was a problem hiding this comment.

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

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions

jasongin reviewed

Aug 8, 2017

View reviewed changes

Comment thread

src/node_api.cc Outdated

jasongin Aug 8, 2017

Copy link

Copy Markdown

Member

There was a problem hiding this comment.

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?

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions

jasongin reviewed

Aug 8, 2017

View reviewed changes

Comment thread

src/node_api.cc Outdated

jasongin Aug 8, 2017

Copy link

Copy Markdown

Member

There was a problem hiding this comment.

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.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions

jasongin reviewed

Aug 8, 2017

View reviewed changes

Comment thread

src/node_api.h Outdated

jasongin Aug 8, 2017

Copy link

Copy Markdown

Member

There was a problem hiding this comment.

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.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions

jasongin suggested changes

Aug 8, 2017

View reviewed changes

jasongin left a comment

Copy link

Copy Markdown

Member

There was a problem hiding this 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.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions

kfarnung reviewed

Aug 8, 2017

View reviewed changes

Comment thread

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

kfarnung Aug 8, 2017

Copy link

Copy Markdown

Contributor

There was a problem hiding this comment.

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

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions

jasongin reviewed

Aug 8, 2017

View reviewed changes

Comment thread

doc/api/n-api.md Outdated

jasongin Aug 8, 2017

Copy link

Copy Markdown

Member

There was a problem hiding this comment.

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

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions

cjihrig Aug 8, 2017

Copy link

Copy Markdown

Contributor

There was a problem hiding this comment.

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.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions

cjihrig reviewed

Aug 8, 2017

View reviewed changes

Comment thread

doc/api/n-api.md Outdated

cjihrig Aug 8, 2017

Copy link

Copy Markdown

Contributor

There was a problem hiding this comment.

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.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions

Comment thread

src/node_api.cc Outdated

cjihrig Aug 8, 2017

Copy link

Copy Markdown

Contributor

There was a problem hiding this comment.

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.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions

sampsongao Aug 8, 2017

Copy link

Copy Markdown

Author

There was a problem hiding this comment.

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.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions

jasongin reviewed

Aug 8, 2017

View reviewed changes

Comment thread

doc/api/n-api.md Outdated

jasongin Aug 8, 2017

Copy link

Copy Markdown

Member

There was a problem hiding this comment.

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

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions

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

mhdawson left a comment

Copy link

Copy Markdown

Member

There was a problem hiding this comment.

Choose a reason for hiding this comment

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

LGTM

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions

mhdawson commented Aug 15, 2017

Copy link

Copy Markdown

Member

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

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

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

mhdawson commented Aug 18, 2017

Copy link

Copy Markdown

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.

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

sampsongao mentioned this pull request

Aug 21, 2017

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

Closed

sampsongao commented Aug 21, 2017

Copy link

Copy Markdown

Author

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

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

There was an error while loading. Please reload this page.

mhdawson commented Sep 14, 2017 •

edited

Loading

Uh oh!

There was an error while loading. Please reload this page.

Copy link

Copy Markdown

Member

ARM failure said this

ccache: error: x_calloc: Could not allocate 40 bytes

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

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

mhdawson commented Sep 14, 2017

Copy link

Copy Markdown

Member

Landed as 973c12f

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

mhdawson closed this

Sep 14, 2017

mhdawson pushed a commit that referenced this pull request

Sep 14, 2017


          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


          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


          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 mentioned this pull request

Sep 20, 2017

Node.js 8.6.0 Release Proposal #15509

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]>

MylesBorins added the dont-land-on-v6.x label

Oct 17, 2017

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


          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 mentioned this pull request

Apr 16, 2018

v6.14.2 proposal #19996

Merged

Reviewers

addaleax

addaleax left review comments

jasnell

jasnell approved these changes

cjihrig

cjihrig approved these changes

+3 more reviewers

kfarnung

kfarnung approved these changes

mhdawson

mhdawson approved these changes

jasongin

jasongin approved these changes

Reviewers whose approvals may not affect merge requirements

Uh oh!

There was an error while loading. Please reload this page.

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 Node-API.

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

Uh oh!

There was an error while loading. Please reload this page.

10 participants

Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.

You are visitor 01402503 — GeezerVison does not store the pages you look up.
Best viewed with Netscape Navigator 4.7 or Internet Explorer 6 at 800×600. No cookies required.

Conversation

sampsongao commented Aug 8, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Checklist

Affected core subsystem(s)

Uh oh!

addaleax left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jasongin left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

mhdawson left a comment

Choose a reason for hiding this comment

Uh oh!

mhdawson commented Aug 15, 2017

Uh oh!

mhdawson commented Aug 18, 2017

Uh oh!

sampsongao commented Aug 21, 2017

Uh oh!

BridgeAR commented Aug 31, 2017

Uh oh!

BridgeAR commented Sep 8, 2017

Uh oh!

mhdawson commented Sep 14, 2017

Uh oh!

mhdawson commented Sep 14, 2017

Uh oh!

mhdawson commented Sep 14, 2017

Uh oh!

mhdawson commented Sep 14, 2017

Uh oh!

mhdawson commented Sep 14, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

mhdawson commented Sep 14, 2017

Uh oh!

Reviewers

Uh oh!

Assignees

Labels

Projects

Milestone

Development

Uh oh!

10 participants

sampsongao commented Aug 8, 2017 •

edited

Loading

Uh oh!

There was an error while loading. Please reload this page.

mhdawson commented Sep 14, 2017 •

edited

Loading

Uh oh!

There was an error while loading. Please reload this page.