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

doc: Clarify positioning versus N-API #288

Closed
wants to merge 2 commits into from
Closed

Conversation

mhdawson
Copy link
Member

No description provided.

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

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

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

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

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

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

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.

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

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

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