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: mention N-API as recommended approach #28922

Closed
wants to merge 6 commits into from

Conversation

mhdawson
Copy link
Member

@mhdawson mhdawson commented Aug 1, 2019

We've had a few comments that from the doc it might not
be clear that N-API is the recommended approach for Addons.
As a start, mention N-API early in the non N-API section
as the recommended approach unless lower level access
is required.

Checklist
  • make -j4 test (UNIX), or vcbuild test (Windows) passes
  • documentation is changed or added
  • commit message follows commit guidelines

We've had a few comments that from the doc it might not
be clear that N-API is the recommended approach for Addons.
As a start, mention N-API early in the non N-API section
as the recommended approach unless lower level access
is required.
@nodejs-github-bot nodejs-github-bot added addons Issues and PRs related to native addons. doc Issues and PRs related to the documentations. labels Aug 1, 2019
Copy link
Contributor

@cjihrig cjihrig left a comment

Choose a reason for hiding this comment

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

One tiny nit, but LGTM.

doc/api/addons.md Outdated Show resolved Hide resolved
@mhdawson
Copy link
Member Author

mhdawson commented Aug 1, 2019

@cjihrig update to say 3 and include nan

doc/api/addons.md Outdated Show resolved Hide resolved
doc/api/addons.md Outdated Show resolved Hide resolved
doc/api/addons.md Outdated Show resolved Hide resolved
doc/api/addons.md Outdated Show resolved Hide resolved
Copy link
Member

@Trott Trott left a comment

Choose a reason for hiding this comment

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

Good as-is, although I have a preference for the suggestions I've left which mostly make things more succinct..

mhdawson and others added 4 commits August 14, 2019 16:56
Co-Authored-By: Rich Trott <rtrott@gmail.com>
Co-Authored-By: Rich Trott <rtrott@gmail.com>
Co-Authored-By: Rich Trott <rtrott@gmail.com>
Co-Authored-By: Rich Trott <rtrott@gmail.com>
@mhdawson mhdawson added the author ready PRs that have at least one approval, no pending requests for changes, and a CI started. label Aug 14, 2019
@Trott
Copy link
Member

Trott commented Aug 14, 2019

@mhdawson
Copy link
Member Author

Lite CI was good, landing.

@mhdawson
Copy link
Member Author

mhdawson commented Aug 16, 2019

Landed as 18405e6

@mhdawson mhdawson closed this Aug 16, 2019
mhdawson added a commit that referenced this pull request Aug 16, 2019
We've had a few comments that from the doc it might not
be clear that N-API is the recommended approach for Addons.
As a start, mention N-API early in the non N-API section
as the recommended approach unless lower level access
is required.

PR-URL: #28922
Reviewed-By: Richard Lau <riclau@uk.ibm.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Reviewed-By: Rich Trott <rtrott@gmail.com>
Reviewed-By: Trivikram Kamat <trivikr.dev@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
targos pushed a commit that referenced this pull request Aug 19, 2019
We've had a few comments that from the doc it might not
be clear that N-API is the recommended approach for Addons.
As a start, mention N-API early in the non N-API section
as the recommended approach unless lower level access
is required.

PR-URL: #28922
Reviewed-By: Richard Lau <riclau@uk.ibm.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Reviewed-By: Rich Trott <rtrott@gmail.com>
Reviewed-By: Trivikram Kamat <trivikr.dev@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
addons Issues and PRs related to native addons. author ready PRs that have at least one approval, no pending requests for changes, and a CI started. doc Issues and PRs related to the documentations.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

8 participants