n-api: add napi_get_node_version by addaleax · Pull Request #14696 · nodejs/node

addaleax · 2017-08-08T17:49:33Z

Add napi_get_node_version, to help with feature-detecting Node.js as an environment.

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

/cc @nodejs/n-api

cjihrig · 2017-08-08T17:52:14Z

doc/api/n-api.md

Period after Optional and after process.release.name.

cjihrig · 2017-08-08T17:59:42Z

src/node_api.cc

Could you add a test where release is null.

kfarnung · 2017-08-08T18:09:44Z

src/node_api.cc

Having a string out parameter is a little bit risky since there needs to be a clear chain of ownership. In this case it will always be a constant string, but as a developer it may not be clear what the scope of the string is and whether it needs to be freed by the caller.

but as a developer it may not be clear what the scope of the string is and whether it needs to be freed by the caller.

I can add a bit to the docs if you have a suggestion for wording, but since this refers to a const char pointer, there isn’t really any ambiguity.

Thanks @addaleax, you're right about the const char * being a good indicator, I hadn't really considered that the object would not be a valid argument to free with the const modifier. It does seem like all usages of this function would at least yield a global string value, so I think my original concern is invalid. I'll take a look at the documentation and suggest any specific changes there.

kfarnung · 2017-08-08T18:14:44Z

src/node_api.cc

Would it be better to use a struct here? I see two benefits:

No need for the developer to study the documentation to determine the size of the array required

Easier to access the parts of the version by member name

Yea, that makes sense. I’ll push again in a couple minutes.

addaleax · 2017-08-08T18:26:22Z

@kfarnung I’ve updated this anyway with the struct approach and clarified that the returned buffer is statically allocated, please take a look :)

kfarnung · 2017-08-08T18:39:02Z

src/node_api.cc

Since we're returning a pointer to this static struct, should the result be a const napi_node_version** to prevent modification by the callers?

I think the other option would be to use a single indirection to a non-const napi_node_version and then just fill in the values. I'm fine either way.

Since we're returning a pointer to this static struct, should the result be a const napi_node_version** to prevent modification by the callers?

Yes. :)

I think the other option would be to use a single indirection to a non-const napi_node_version and then just fill in the values. I'm fine either way.

I thought about that, but this approach has the advantage of being extensible on the N-API side in a backwards-compatible fashion, if we or somebody else (Node forkers?) ever need to do that.

Agreed on the current approach.

kfarnung

LGTM

bnoordhuis · 2017-08-08T21:02:00Z

FWIW, libuv has two separate functions for this, one that returns a version string and one that returns major * 65536 + minor * 256 + patch. Seems simpler than having an out pointer arg.

Aside: the env argument isn't (meaningfully) used. Maybe remove it?

jasongin · 2017-08-08T21:31:09Z

the env argument isn't (meaningfully) used. Maybe remove it?

All the napi_* functions have a env parameter, even though a few of them don't currently use it. We do this for a few reasons:

Consistency
N-API implementation on other VMs may require a context even if it isn't needed for V8.
If/when node supports multiple environments/isolates in a process we will be able to update the implementation without having breaking changes.

Maybe for this particular case, items 2 and 3 will never be an issue. But still it would be strange if this API did not follow the pattern of all the others that have env as the first parameter.

addaleax · 2017-08-10T20:34:27Z

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

This should be ready.

jasnell · 2017-08-10T20:38:51Z

doc/api/n-api.md

would it make sense for this struct to also include the dependency version details equivalent to process.versions?

I don’t like it when N-API exposes things that are already doable by just calling into JS, using, well N-API :) This particular function might be required to check whether some of that calling-into-JS works the way you want it to.

Well, eventually one of the goals should be for Node.js itself to use N-API internally, in which case this method would be the way to get at this information from JS :-) .. but that's down the road.

Well, eventually one of the goals should be for Node.js itself to use N-API internally

I think N-API inherently adds too much complexity to fully replace the other APIs we use. :)

in which case this method would be the way to get at this information from JS

Adding new methods is easy, and this was implemented specifically in a way that’s easy to extend on the N-API side. :)

addaleax · 2017-08-12T13:57:05Z

~~CI: https://ci.nodejs.org/job/node-test-commit/11729/~~
CI: https://ci.nodejs.org/job/node-test-commit/11731/

Add `napi_get_node_version`, to help with feature-detecting Node.js as an environment. PR-URL: nodejs#14696 Reviewed-By: Kyle Farnung <kfarnung@microsoft.com> Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Colin Ihrig <cjihrig@gmail.com> Reviewed-By: Tobias Nießen <tniessen@tnie.de>

addaleax · 2017-08-12T19:29:26Z

Landed in 835c383

Add `napi_get_node_version`, to help with feature-detecting Node.js as an environment. PR-URL: #14696 Reviewed-By: Kyle Farnung <kfarnung@microsoft.com> Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Colin Ihrig <cjihrig@gmail.com> Reviewed-By: Tobias Nießen <tniessen@tnie.de>

Add `napi_get_node_version`, to help with feature-detecting Node.js as an environment. PR-URL: nodejs#14696 Reviewed-By: Kyle Farnung <kfarnung@microsoft.com> Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Colin Ihrig <cjihrig@gmail.com> Reviewed-By: Tobias Nießen <tniessen@tnie.de>

Add `napi_get_node_version`, to help with feature-detecting Node.js as an environment. Backport-PR-URL: #19447 PR-URL: #14696 Reviewed-By: Kyle Farnung <kfarnung@microsoft.com> Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Colin Ihrig <cjihrig@gmail.com> Reviewed-By: Tobias Nießen <tniessen@tnie.de>

nodejs-github-bot added c++ Issues and PRs that require attention from people who are familiar with C++. lib / src Issues and PRs related to general changes in the lib or src directory. labels Aug 8, 2017

addaleax added 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. and removed lib / src Issues and PRs related to general changes in the lib or src directory. labels Aug 8, 2017

cjihrig approved these changes Aug 8, 2017

View reviewed changes

kfarnung reviewed Aug 8, 2017

View reviewed changes

addaleax force-pushed the napi-node-version branch from b41455a to b2e5ccf Compare August 8, 2017 18:25

kfarnung reviewed Aug 8, 2017

View reviewed changes

kfarnung approved these changes Aug 8, 2017

View reviewed changes

tniessen approved these changes Aug 9, 2017

View reviewed changes

jasnell reviewed Aug 10, 2017

View reviewed changes

jasnell approved these changes Aug 10, 2017

View reviewed changes

addaleax force-pushed the napi-node-version branch from 31e52f3 to b23c9f3 Compare August 12, 2017 13:56

addaleax force-pushed the napi-node-version branch from b23c9f3 to 47ff33e Compare August 12, 2017 15:43

addaleax force-pushed the napi-node-version branch from 47ff33e to ec61a0b Compare August 12, 2017 15:53

addaleax closed this Aug 12, 2017

addaleax deleted the napi-node-version branch August 12, 2017 19:29

addaleax mentioned this pull request Aug 13, 2017

v8.4.0 proposal #14811

Merged

MylesBorins added the dont-land-on-v6.x label Jan 11, 2018

MylesBorins mentioned this pull request Apr 16, 2018

v6.14.2 proposal #19996

Merged

Uh oh!

Conversation

addaleax commented Aug 8, 2017

Checklist

Affected core subsystem(s)

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

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

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!

addaleax commented Aug 8, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

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

Uh oh!

Choose a reason for hiding this comment

Uh oh!

kfarnung left a comment

Choose a reason for hiding this comment

Uh oh!

bnoordhuis commented Aug 8, 2017

Uh oh!

jasongin commented Aug 8, 2017

Uh oh!

addaleax commented Aug 10, 2017

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!

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

Uh oh!

Uh oh!

addaleax commented Aug 12, 2017

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

9 participants

kfarnung Aug 8, 2017 •

edited

Loading

kfarnung Aug 8, 2017 •

edited

Loading

addaleax commented Aug 12, 2017 •

edited

Loading