Skip to content

doc: Clarify positioning versus N-API#288

Closed
mhdawson wants to merge 2 commits intonodejs:masterfrom
mhdawson:readme
Closed

doc: Clarify positioning versus N-API#288
mhdawson wants to merge 2 commits intonodejs:masterfrom
mhdawson:readme

Conversation

@mhdawson
Copy link
Copy Markdown
Member

No description provided.

Copy link
Copy Markdown
Member

@NickNaso NickNaso left a 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.

Copy link
Copy Markdown
Contributor

@rivertam rivertam left a 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.

Comment thread 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
Copy link
Copy Markdown
Contributor

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"

Copy link
Copy Markdown

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.

Copy link
Copy Markdown
Member Author

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

Copy link
Copy Markdown

@webern webern left a 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.

Comment thread 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
Copy link
Copy Markdown

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.

Comment thread README.md Outdated
**ECMA262 Language Specification**.
recompilation.

The node-addon-api module preserves the benefits of the N-API as it consists
Copy link
Copy Markdown

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 C N-API by using inline C++ code which only depends on the stable C ABI provided by N-API.

Comment thread README.md
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
Copy link
Copy Markdown

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...

@mhdawson
Copy link
Copy Markdown
Member Author

mhdawson commented Jun 21, 2018

@webern, @rivertam thanks for helping us improve the doc we have so far :) Incorporated your comments and will land.

mhdawson added a commit that referenced this pull request Jun 21, 2018
PR-URL: #288
Reviewed-By: Nicola Del Gobbo <nicoladelgobbo@NickNaso.local>
Copy link
Copy Markdown

@webern webern left a comment

Choose a reason for hiding this comment

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

Looks good.

@mhdawson
Copy link
Copy Markdown
Member Author

Landed as c2a620d

@mhdawson mhdawson closed this Jun 21, 2018
kevindavies8 added a commit to kevindavies8/node-addon-api-Develop that referenced this pull request Aug 24, 2022
PR-URL: nodejs/node-addon-api#288
Reviewed-By: Nicola Del Gobbo <nicoladelgobbo@NickNaso.local>
Marlyfleitas added a commit to Marlyfleitas/node-api-addon-Development that referenced this pull request Aug 26, 2022
PR-URL: nodejs/node-addon-api#288
Reviewed-By: Nicola Del Gobbo <nicoladelgobbo@NickNaso.local>
wroy7860 added a commit to wroy7860/addon-api-benchmark-node that referenced this pull request Sep 19, 2022
PR-URL: nodejs/node-addon-api#288
Reviewed-By: Nicola Del Gobbo <nicoladelgobbo@NickNaso.local>
johnfrench3 pushed a commit to johnfrench3/node-addon-api-git that referenced this pull request Aug 11, 2023
PR-URL: nodejs/node-addon-api#288
Reviewed-By: Nicola Del Gobbo <nicoladelgobbo@NickNaso.local>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants