-
Notifications
You must be signed in to change notification settings - Fork 459
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
doc: Clarify positioning versus N-API #288
Conversation
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.
LGTM - Now it's more clear.
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.
So much clearer.
The only thing I'd say is that in the future, it would probably be better to not even reference N-API at all and just say "This is a C++ library for writing Node addons" and only later mentioning that it's built on top of N-API. As a C++ native addon developer, in my ideal world, I don't have to know about anything besides node-addon-api. However, considering this isn't integrated into the core docs and the module docs are incomplete, positioning it relative to N-API seems fine for now.
README.md
Outdated
provided by Node.js when using C++. It provides a C++ object model | ||
and exception handling semantics with low overhead. | ||
|
||
N-API is an ABI stale API for building native addons in C++. It is |
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 you just mean "N-API is a stable API" rather than "N-API is an ABI stale API"
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 you should also say N-API is a ABI stable C interface provided by Node.js for building native addons...
It is not necessary that such addons be written in C++ and saying in C++
muddies the waters a little bit IMO.
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.
Thanks for catching those 2, will update to address
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.
Thank you for this, it should help new users. I left a couple of suggestions.
README.md
Outdated
provided by Node.js when using C++. It provides a C++ object model | ||
and exception handling semantics with low overhead. | ||
|
||
N-API is an ABI stale API for building native addons in C++. It is |
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 you should also say N-API is a ABI stable C interface provided by Node.js for building native addons...
It is not necessary that such addons be written in C++ and saying in C++
muddies the waters a little bit IMO.
README.md
Outdated
**ECMA262 Language Specification**. | ||
recompilation. | ||
|
||
The node-addon-api module preserves the benefits of the N-API as it consists |
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.
add ticks around node-addon-api like node-addon-api
.
Recommendation:
The
node-addon-api
module, which is not a part of Node.js, preserves the benefits of the CN-API
by using inlineC++
code which only depends on the stable C ABI provided byN-API
.
README.md
Outdated
only of inline code that depends only on the stable API provided by N-API. | ||
As such, modules built against one version of Node.js using node-addon-api | ||
should run without having to be rebuilt with newer versions of Node.js. | ||
As new APIs are added to N-API, node-addon-api must be updated to provide |
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.
Start a new paragraph at As new APIs are added to...
PR-URL: #288 Reviewed-By: Nicola Del Gobbo <nicoladelgobbo@NickNaso.local>
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.
Looks good.
Landed as c2a620d |
PR-URL: nodejs/node-addon-api#288 Reviewed-By: Nicola Del Gobbo <nicoladelgobbo@NickNaso.local>
PR-URL: nodejs/node-addon-api#288 Reviewed-By: Nicola Del Gobbo <nicoladelgobbo@NickNaso.local>
PR-URL: nodejs/node-addon-api#288 Reviewed-By: Nicola Del Gobbo <nicoladelgobbo@NickNaso.local>
PR-URL: nodejs/node-addon-api#288 Reviewed-By: Nicola Del Gobbo <nicoladelgobbo@NickNaso.local>
No description provided.