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.

Sign up for GitHub

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

Jump to bottom

doc: mention N-API as recommended approach #28922

Closed
wants to merge 6 commits into from
Closed

doc: mention N-API as recommended approach #28922

wants to merge 6 commits into from

Conversation

mhdawson
Copy link
Member

@mhdawson mhdawson commented Aug 1, 2019 •
edited
Loading

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

@mhdawson
doc: mention N-API as recommended approach
77c4e87 
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
cjihrig
cjihrig approved these changes Aug 1, 2019
View reviewed changes
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

Trott
Trott reviewed Aug 4, 2019
View reviewed changes
doc/api/addons.md Outdated Show resolved Hide resolved
Trott
Trott reviewed Aug 4, 2019
View reviewed changes
doc/api/addons.md Outdated Show resolved Hide resolved
Trott
Trott reviewed Aug 4, 2019
View reviewed changes
doc/api/addons.md Outdated Show resolved Hide resolved
Trott
Trott reviewed Aug 4, 2019
View reviewed changes
doc/api/addons.md Outdated Show resolved Hide resolved
Trott
Trott approved these changes Aug 4, 2019
View reviewed changes
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
@mhdawson @Trott
Update doc/api/addons.md
0cf210e 
Co-Authored-By: Rich Trott <[email protected]>
@mhdawson @Trott
Update doc/api/addons.md
22c7d38 
Co-Authored-By: Rich Trott <[email protected]>
@mhdawson @Trott
Update doc/api/addons.md
3e8ef6d 
Co-Authored-By: Rich Trott <[email protected]>
@mhdawson @Trott
Update doc/api/addons.md
429dd79 
Co-Authored-By: Rich Trott <[email protected]>
@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

Lite CI: https://ci.nodejs.org/job/node-test-pull-request-lite-pipeline/3831/

@mhdawson
Copy link
Member Author

Lite CI was good, landing.

@mhdawson
Copy link
Member Author

mhdawson commented Aug 16, 2019 •
edited
Loading

Landed as 18405e6

@mhdawson mhdawson closed this Aug 16, 2019
mhdawson added a commit that referenced this pull request Aug 16, 2019
@mhdawson
doc: mention N-API as recommended approach
18405e6 
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 <[email protected]>
Reviewed-By: Colin Ihrig <[email protected]>
Reviewed-By: Luigi Pinca <[email protected]>
Reviewed-By: Rich Trott <[email protected]>
Reviewed-By: Trivikram Kamat <[email protected]>
Reviewed-By: James M Snell <[email protected]>
targos pushed a commit that referenced this pull request Aug 19, 2019
@mhdawson @targos
doc: mention N-API as recommended approach
edbe38d 
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 <[email protected]>
Reviewed-By: Colin Ihrig <[email protected]>
Reviewed-By: Luigi Pinca <[email protected]>
Reviewed-By: Rich Trott <[email protected]>
Reviewed-By: Trivikram Kamat <[email protected]>
Reviewed-By: James M Snell <[email protected]>
@targos targos mentioned this pull request Aug 19, 2019
Merged
This was referenced Aug 20, 2019
Closed
Closed
This was referenced Aug 20, 2019
Closed
Closed
Closed
Closed
@mhdawson mhdawson deleted the doc-update2 branch September 14, 2020 21:18
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@trivikr trivikr trivikr approved these changes

@richardlau richardlau richardlau approved these changes

@cjihrig cjihrig cjihrig approved these changes

@lpinca lpinca lpinca approved these changes

@Trott Trott Trott approved these changes

@jasnell jasnell jasnell approved these changes

Assignees
No one assigned
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
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
8 participants
@mhdawson @Trott @jasnell @lpinca @cjihrig @richardlau @trivikr @nodejs-github-bot