-
Notifications
You must be signed in to change notification settings - Fork 30.1k
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_node_version #14696
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -3119,6 +3119,20 @@ napi_status napi_get_version(napi_env env, uint32_t* result) { | |
return napi_clear_last_error(env); | ||
} | ||
|
||
napi_status napi_get_node_version(napi_env env, | ||
const napi_node_version** result) { | ||
CHECK_ENV(env); | ||
CHECK_ARG(env, result); | ||
static const napi_node_version version = { | ||
NODE_MAJOR_VERSION, | ||
NODE_MINOR_VERSION, | ||
NODE_PATCH_VERSION, | ||
NODE_RELEASE | ||
}; | ||
*result = &version; | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Since we're returning a pointer to this static struct, should the I think the other option would be to use a single indirection to a non-const There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Yes. :)
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. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Agreed on the current approach. |
||
return napi_clear_last_error(env); | ||
} | ||
|
||
namespace uvimpl { | ||
|
||
static napi_status ConvertUVErrorCode(int code) { | ||
|
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.
would it make sense for this struct to also include the dependency version details equivalent to
process.versions
?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 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.
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.
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.
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 N-API inherently adds too much complexity to fully replace the other APIs we use. :)
Adding new methods is easy, and this was implemented specifically in a way that’s easy to extend on the N-API side. :)