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.

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

n-api: Sync with back-compat changes #12674

Closed
wants to merge 1 commit into from
Closed

n-api: Sync with back-compat changes #12674

wants to merge 1 commit into from

Conversation

jasongin
Copy link
Member

Background: To enable N-API support for node versions back to v4, the N-API code can also be built as an external addon. To make maintenance easier, a single codebase needs to support both built-in and external scenarios, along with Node versions >= 4 (and corresponding V8 versions).

This change includes several minor fixes to avoid using node internal APIs and support older V8 versions:

  • Expand node::arraysize()
  • In the CHECK_ENV() macro, return an error code instead of calling node::FatalError(). This is more consistent with how other invalid arguments to N-API functions are handled.
  • In v8impl::SetterCallbackWrapper::SetReturnValue(), do nothing instead of calling node::FatalError(). This is more consistent with JavaScript setter callbacks, where any returned value is silently ignored.
  • When queueing async work items, get the uv default loop instead of getting the loop from node::Environment::GetCurrent(). Currently that returns the same loop anyway. If/when node supports multiple environments, it should have a public API for getting the environment & event loop, and we can update this implementation then.
  • Use v8::Maybe::FromJust() instead of the newer alias ToChecked()

These changes were copied directly from nodejs/node-addon-api@c7d4df4 where they were reviewed as part of
nodejs/node-addon-api#25

Checklist
  • make -j4 test (UNIX), or vcbuild test (Windows) passes
  • commit message follows commit guidelines
Affected core subsystem(s)

n-api

@jasongin
n-api: Sync with back-compat changes
0341910 
Background: To enable N-API support for node versions back to v4, the
N-API code can also be built as an external addon. To make maintenance
easier, a single codebase needs to support both built-in and external
scenarios, along with Node versions >= 4 (and corresponding V8
versions).

This change includes several minor fixes to avoid using node
internal APIs and support older V8 versions:
  - Expand node::arraysize
  - In the CHECK_ENV() macro, return an error code instead of calling
    node::FatalError(). This is more consistent with how other invalid
    arguments to N-API functions are handled.
  - In v8impl::SetterCallbackWrapper::SetReturnValue(), do nothing
    instead of calling node::FatalError(). This is more consistent
    with JavaScript setter callbacks, where any returned value is
    silently ignored.
  - When queueing async work items, get the uv default loop instead of
    getting the loop from node::Environment::GetCurrent(). Currently
    that returns the same loop anyway. If/when node supports multiple
    environments, it should have a public API for getting the
    environment & event loop, and we can update this implementation
    then.
  - Use v8::Maybe::FromJust() instead of the newer alias ToChecked()

These changes were copied directly from
nodejs/node-addon-api@c7d4df4
where they were reviewed as part of
nodejs/node-addon-api#25
@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 Apr 26, 2017
@jasongin
Copy link
Member Author

@mhdawson @addaleax You reviewed these changes already over at nodejs/node-addon-api#25. As discussed there, this is just merging them back to the node master branch.

@addaleax
Copy link
Member

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

mhdawson
mhdawson approved these changes Apr 26, 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

@jasongin
Copy link
Member Author

The Windows CI job seems to have been... interrupted? Can someone restart it?

@mhdawson
Copy link
Member

I tried restarting one of the fanned jobs before without much luck so I'm just going to start a new ci run:https://ci.nodejs.org/job/node-test-pull-request/7711/

@mhdawson
Copy link
Member

CI clean, landing

@mhdawson
Copy link
Member

landed as 972bfe1

@mhdawson mhdawson closed this Apr 28, 2017
mhdawson pushed a commit that referenced this pull request Apr 28, 2017
@jasongin @mhdawson
n-api: Sync with back-compat changes
972bfe1 
Background: To enable N-API support for node versions back to v4, the
N-API code can also be built as an external addon. To make maintenance
easier, a single codebase needs to support both built-in and external
scenarios, along with Node versions >= 4 (and corresponding V8
versions).

This change includes several minor fixes to avoid using node
internal APIs and support older V8 versions:
  - Expand node::arraysize
  - In the CHECK_ENV() macro, return an error code instead of calling
    node::FatalError(). This is more consistent with how other invalid
    arguments to N-API functions are handled.
  - In v8impl::SetterCallbackWrapper::SetReturnValue(), do nothing
    instead of calling node::FatalError(). This is more consistent
    with JavaScript setter callbacks, where any returned value is
    silently ignored.
  - When queueing async work items, get the uv default loop instead of
    getting the loop from node::Environment::GetCurrent(). Currently
    that returns the same loop anyway. If/when node supports multiple
    environments, it should have a public API for getting the
    environment & event loop, and we can update this implementation
    then.
  - Use v8::Maybe::FromJust() instead of the newer alias ToChecked()

PR-URL: #12674
Reviewed-By: Anna Henningsen <[email protected]>
Reviewed-By: Michael Dawson <[email protected]>
@jasongin jasongin deleted the napi-backcompat branch April 28, 2017 21:28
@jasnell jasnell mentioned this pull request May 11, 2017
Closed
gabrielschulhof pushed a commit to gabrielschulhof/node that referenced this pull request Apr 10, 2018
@jasongin
n-api: Sync with back-compat changes
406f4ce 
Background: To enable N-API support for node versions back to v4, the
N-API code can also be built as an external addon. To make maintenance
easier, a single codebase needs to support both built-in and external
scenarios, along with Node versions >= 4 (and corresponding V8
versions).

This change includes several minor fixes to avoid using node
internal APIs and support older V8 versions:
  - Expand node::arraysize
  - In the CHECK_ENV() macro, return an error code instead of calling
    node::FatalError(). This is more consistent with how other invalid
    arguments to N-API functions are handled.
  - In v8impl::SetterCallbackWrapper::SetReturnValue(), do nothing
    instead of calling node::FatalError(). This is more consistent
    with JavaScript setter callbacks, where any returned value is
    silently ignored.
  - When queueing async work items, get the uv default loop instead of
    getting the loop from node::Environment::GetCurrent(). Currently
    that returns the same loop anyway. If/when node supports multiple
    environments, it should have a public API for getting the
    environment & event loop, and we can update this implementation
    then.
  - Use v8::Maybe::FromJust() instead of the newer alias ToChecked()

PR-URL: nodejs#12674
Reviewed-By: Anna Henningsen <[email protected]>
Reviewed-By: Michael Dawson <[email protected]>
MylesBorins pushed a commit that referenced this pull request Apr 16, 2018
@jasongin @MylesBorins
n-api: Sync with back-compat changes
49d74c6 
Background: To enable N-API support for node versions back to v4, the
N-API code can also be built as an external addon. To make maintenance
easier, a single codebase needs to support both built-in and external
scenarios, along with Node versions >= 4 (and corresponding V8
versions).

This change includes several minor fixes to avoid using node
internal APIs and support older V8 versions:
  - Expand node::arraysize
  - In the CHECK_ENV() macro, return an error code instead of calling
    node::FatalError(). This is more consistent with how other invalid
    arguments to N-API functions are handled.
  - In v8impl::SetterCallbackWrapper::SetReturnValue(), do nothing
    instead of calling node::FatalError(). This is more consistent
    with JavaScript setter callbacks, where any returned value is
    silently ignored.
  - When queueing async work items, get the uv default loop instead of
    getting the loop from node::Environment::GetCurrent(). Currently
    that returns the same loop anyway. If/when node supports multiple
    environments, it should have a public API for getting the
    environment & event loop, and we can update this implementation
    then.
  - Use v8::Maybe::FromJust() instead of the newer alias ToChecked()

Backport-PR-URL: #19447
PR-URL: #12674
Reviewed-By: Anna Henningsen <[email protected]>
Reviewed-By: Michael Dawson <[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

@mhdawson mhdawson mhdawson approved these changes

@addaleax addaleax addaleax approved these changes

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.
4 participants
@jasongin @addaleax @mhdawson @nodejs-github-bot