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.

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

Conversation

mhdawson
Copy link
Member

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

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

Fixes: nodejs/abi-stable-node#231
@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 May 24, 2017
doc/api/n-api.md Outdated

### napi_get_version
<!-- YAML
added: v8.0.0
Copy link
Member

Choose a reason for hiding this comment

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

added: REPLACEME

doc/api/n-api.md Outdated
-->
```C
NAPI_EXTERN napi_status napi_get_version(napi_env env,
uint32_t* result);
Copy link
Member

Choose a reason for hiding this comment

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

The argument alignment is slightly off here

@addaleax addaleax added the semver-minor PRs that contain new features and should be released in the next minor version. label May 24, 2017
@mhdawson
Copy link
Member Author

Thanks for the comments, push commit to address.


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

Choose a reason for hiding this comment

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

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

@jasongin
Copy link
Member

@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
Copy link
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.

@jasongin
Copy link
Member

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.

doc/api/n-api.md Outdated
`napi_get_version` to check which level of N-API is supported
at run time. Based on the result of this check the addon can
implement appropriate code paths when functions
are, or are not, fully implemented.
Copy link
Member

Choose a reason for hiding this comment

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

I would omit the mention of backporting stubs, since that wouldn't provide addon developers any real guarantees because many applications will not have the updates with the backported stubs. Instead, if addon developers desire backward compatibility with Node versions having older N-API versions, we should recommend dynamically loading APIs that might not be present, after checking the version.

I'd suggest changing this paragraph to something like this:

This API returns the highest N-API version supported by the
Node.js runtime.  N-API is planned to be additive such that newer
releases of Node.js may support additional API functions. To allow
an addon to use a newer API when it's available while providing a
fallback behavior when it's not, first call `napi_get_version()` to
determine if the API is available, then use `uv_dlsym()` to dynamically
load a pointer to the function.

@mhdawson
Copy link
Member Author

@jasongin pushed commit to address suggestion.

doc/api/n-api.md Outdated
newer releases of Node.js may support additional API functions.
In order to allow an addon to use a newer function when running with
versions of Node.js that support it, while providing
fallback behaviour when runnign with Node.js versions that don't
Copy link
Member

Choose a reason for hiding this comment

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

Typo: runnign

Also, do we prefer American or British spelling of "behaviour"?

@mhdawson
Copy link
Member Author

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

@mhdawson
Copy link
Member Author

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

Going to land.

@mhdawson
Copy link
Member Author

Landed as d9ee297

@mhdawson mhdawson closed this May 26, 2017
mhdawson added a commit that referenced this pull request May 26, 2017
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 pushed a commit that referenced this pull request May 28, 2017
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 mentioned this pull request May 28, 2017
@@ -18,6 +18,8 @@
#include "uv.h"
#include "node_api.h"

#define NAPI_VERSION 1
Copy link
Member

Choose a reason for hiding this comment

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

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

@mhdawson
Copy link
Member Author

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
Copy link
Member

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 napi_version branch June 28, 2017 19:23
gabrielschulhof pushed a commit to gabrielschulhof/node that referenced this pull request Apr 10, 2018
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 pushed a commit that referenced this pull request Apr 16, 2018
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 mentioned this pull request Apr 16, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
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. semver-minor PRs that contain new features and should be released in the next minor version.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

8 participants