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.

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

n-api: add napi_get_version #13207

Closed
wants to merge 4 commits into
base: master
from

Conversation

Projects
None yet
8 participants
@mhdawson
Member

mhdawson commented May 24, 2017

Add napi_get_version function so that addons can
query the level of N-API supported.

Fixes: nodejs/abi-stable-node#231

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)

n-api

n-api: add napi_get_version
Add napi_get_version function so that addons can
query the level of N-API supported.

Fixes: nodejs/abi-stable-node#231
Show outdated Hide outdated doc/api/n-api.md
Show outdated Hide outdated doc/api/n-api.md
@mhdawson

This comment has been minimized.

Show comment
Hide comment
@mhdawson

mhdawson May 25, 2017

Member

Thanks for the comments, push commit to address.

Member

mhdawson commented May 25, 2017

Thanks for the comments, push commit to address.

// test version management funcitons
// expected version is currently 1
assert.strictEqual(test_general.testGetVersion(), 1);

This comment has been minimized.

@cjihrig

cjihrig May 25, 2017

Contributor

Is this test going to need to be updated every time the napi version is bumped?

@cjihrig

cjihrig May 25, 2017

Contributor

Is this test going to need to be updated every time the napi version is bumped?

@jasongin

This comment has been minimized.

Show comment
Hide comment
@jasongin

jasongin May 25, 2017

Contributor

@mhdawson I don't understand how this API can be very useful in practice, even after reading the doc added here.

Consider a native module that wants to use a function that was added in N-API v2. Even if that native module code calls napi_get_version() to check before invoking the new function, the native module will still fail to load on node versions with only N-API v1, due to unresolved external symbols. (I think? Maybe that is platform-dependent?) In that case, this scenario completely depends on back-porting "stubs" to older versions. But we know many applications do not always update to the latest patch version of their major release. And also, if we're backporting something, why wouldn't we backport the implementation rather than just stubs?

Contributor

jasongin commented May 25, 2017

@mhdawson I don't understand how this API can be very useful in practice, even after reading the doc added here.

Consider a native module that wants to use a function that was added in N-API v2. Even if that native module code calls napi_get_version() to check before invoking the new function, the native module will still fail to load on node versions with only N-API v1, due to unresolved external symbols. (I think? Maybe that is platform-dependent?) In that case, this scenario completely depends on back-porting "stubs" to older versions. But we know many applications do not always update to the latest patch version of their major release. And also, if we're backporting something, why wouldn't we backport the implementation rather than just stubs?

@addaleax

This comment has been minimized.

Show comment
Hide comment
@addaleax

addaleax May 25, 2017

Member

@jasongin Fwiw, addons can use dlsym & its siblings in order to load symbols dynamically if they know that those aren’t available unconditionally. But yes, otherwise addons would just fail to load.

Member

addaleax commented May 25, 2017

@jasongin Fwiw, addons can use dlsym & its siblings in order to load symbols dynamically if they know that those aren’t available unconditionally. But yes, otherwise addons would just fail to load.

@jasongin

This comment has been minimized.

Show comment
Hide comment
@jasongin

jasongin May 25, 2017

Contributor

addons can use dlsym & its siblings in order to load symbols dynamically if they know that those aren’t available unconditionally

Yes, we discussed this in the N-API sync meeting earlier today. That is a way module authors could use this napi_get_version() API to enable more complete backward-compatibility, without depending on back-ported stubs. It will require more work to be able to load and call such APIs, but that can potentially be hidden in the node-addon-api headers.

Contributor

jasongin commented May 25, 2017

addons can use dlsym & its siblings in order to load symbols dynamically if they know that those aren’t available unconditionally

Yes, we discussed this in the N-API sync meeting earlier today. That is a way module authors could use this napi_get_version() API to enable more complete backward-compatibility, without depending on back-ported stubs. It will require more work to be able to load and call such APIs, but that can potentially be hidden in the node-addon-api headers.

Show outdated Hide outdated doc/api/n-api.md
@mhdawson

This comment has been minimized.

Show comment
Hide comment
@mhdawson

mhdawson May 25, 2017

Member

@jasongin pushed commit to address suggestion.

Member

mhdawson commented May 25, 2017

@jasongin pushed commit to address suggestion.

Show outdated Hide outdated doc/api/n-api.md
@mhdawson

This comment has been minimized.

Show comment
Hide comment
@mhdawson

mhdawson May 26, 2017

Member

Pushed commit to address remaining comments.
CI run: https://ci.nodejs.org/job/node-test-pull-request/8323/

Member

mhdawson commented May 26, 2017

Pushed commit to address remaining comments.
CI run: https://ci.nodejs.org/job/node-test-pull-request/8323/

@mhdawson

This comment has been minimized.

Show comment
Hide comment
@mhdawson

mhdawson May 26, 2017

Member

OSX failure was unrelated to change being made. Opened this issue to track: #13248

Going to land.

Member

mhdawson commented May 26, 2017

OSX failure was unrelated to change being made. Opened this issue to track: #13248

Going to land.

@mhdawson

This comment has been minimized.

Show comment
Hide comment
@mhdawson

mhdawson May 26, 2017

Member

Landed as d9ee297

Member

mhdawson commented May 26, 2017

Landed as d9ee297

@mhdawson mhdawson closed this May 26, 2017

mhdawson added a commit that referenced this pull request May 26, 2017

n-api: add napi_get_version
Add napi_get_version function so that addons can
query the level of N-API supported.

PR-URL: #13207
Fixes: nodejs/abi-stable-node#231
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Jason Ginchereau <jasongin@microsoft.com>

jasnell added a commit that referenced this pull request May 28, 2017

n-api: add napi_get_version
Add napi_get_version function so that addons can
query the level of N-API supported.

PR-URL: #13207
Fixes: nodejs/abi-stable-node#231
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Jason Ginchereau <jasongin@microsoft.com>

@jasnell jasnell referenced this pull request May 28, 2017

Closed

8.0.0 Release Proposal #12220

@@ -18,6 +18,8 @@
#include "uv.h"
#include "node_api.h"
#define NAPI_VERSION 1

This comment has been minimized.

@kunalspathak

kunalspathak May 31, 2017

Contributor

@mhdawson - Was there any reason this is not inside src/node_api.h instead?

@kunalspathak

kunalspathak May 31, 2017

Contributor

@mhdawson - Was there any reason this is not inside src/node_api.h instead?

@mhdawson

This comment has been minimized.

Show comment
Hide comment
@mhdawson

mhdawson May 31, 2017

Member

I made it internal as it was only used by node_api.cc. Not sure if we want code doing compile time checks.

Member

mhdawson commented May 31, 2017

I made it internal as it was only used by node_api.cc. Not sure if we want code doing compile time checks.

@jasongin

This comment has been minimized.

Show comment
Hide comment
@jasongin

jasongin May 31, 2017

Contributor

We don't want to encourage people to do compile-time checks of the N-API version, because the runtime version could be different. I think it should stay internal-only.

Contributor

jasongin commented May 31, 2017

We don't want to encourage people to do compile-time checks of the N-API version, because the runtime version could be different. I think it should stay internal-only.

@mhdawson mhdawson deleted the mhdawson:napi_version branch Jun 28, 2017

gabrielschulhof added a commit to gabrielschulhof/node that referenced this pull request Mar 12, 2018

n-api: add napi_get_version
Add napi_get_version function so that addons can
query the level of N-API supported.

PR-URL: nodejs#13207
Fixes: nodejs/abi-stable-node#231
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Jason Ginchereau <jasongin@microsoft.com>

gabrielschulhof added a commit to gabrielschulhof/node that referenced this pull request Mar 15, 2018

n-api: add napi_get_version
Add napi_get_version function so that addons can
query the level of N-API supported.

PR-URL: nodejs#13207
Fixes: nodejs/abi-stable-node#231
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Jason Ginchereau <jasongin@microsoft.com>

gabrielschulhof added a commit to gabrielschulhof/node that referenced this pull request Mar 31, 2018

n-api: add napi_get_version
Add napi_get_version function so that addons can
query the level of N-API supported.

PR-URL: nodejs#13207
Fixes: nodejs/abi-stable-node#231
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Jason Ginchereau <jasongin@microsoft.com>

gabrielschulhof added a commit to gabrielschulhof/node that referenced this pull request Apr 6, 2018

n-api: add napi_get_version
Add napi_get_version function so that addons can
query the level of N-API supported.

PR-URL: nodejs#13207
Fixes: nodejs/abi-stable-node#231
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Jason Ginchereau <jasongin@microsoft.com>

gabrielschulhof added a commit to gabrielschulhof/node that referenced this pull request Apr 10, 2018

n-api: add napi_get_version
Add napi_get_version function so that addons can
query the level of N-API supported.

PR-URL: nodejs#13207
Fixes: nodejs/abi-stable-node#231
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Jason Ginchereau <jasongin@microsoft.com>

MylesBorins added a commit that referenced this pull request Apr 16, 2018

n-api: add napi_get_version
Add napi_get_version function so that addons can
query the level of N-API supported.

Backport-PR-URL: #19447
PR-URL: #13207
Fixes: nodejs/abi-stable-node#231
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Jason Ginchereau <jasongin@microsoft.com>

@MylesBorins MylesBorins referenced this pull request Apr 16, 2018

Merged

v6.14.2 proposal #19996

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment