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 node.js addons language #12898

Closed
wants to merge 1 commit into
base: master
from

Conversation

@BethGriggs
Member

BethGriggs commented May 8, 2017

Fixes: #7129

Checklist
Affected core subsystem(s)

doc

@sam-github

Perhaps in the git commit message "language" should be "are C++"? Other than that, LGTM

@refack

This comment has been minimized.

Show comment
Hide comment
@refack

refack May 8, 2017

Member

@BethGriggs super nice of you 🍰
Maybe add a note about experimental napi for a future PR

Member

refack commented May 8, 2017

@BethGriggs super nice of you 🍰
Maybe add a note about experimental napi for a future PR

@refack

refack approved these changes May 8, 2017

@jasnell

jasnell approved these changes May 8, 2017

@refack refack referenced this pull request May 8, 2017

Closed

doc: Add initial documentation for N-API #12549

3 of 3 tasks complete
@Trott

Trott approved these changes May 8, 2017

@gibfahn

gibfahn approved these changes May 8, 2017

@danbev

danbev approved these changes May 9, 2017

@gibfahn

This comment has been minimized.

Show comment
Hide comment
@gibfahn

gibfahn May 9, 2017

Member

Maybe add a note about experimental napi

@refack what specifically would you want adding? An actual sentence would be nice (although it could probably be left for a follow-on PR anyway).

Member

gibfahn commented May 9, 2017

Maybe add a note about experimental napi

@refack what specifically would you want adding? An actual sentence would be nice (although it could probably be left for a follow-on PR anyway).

@cjihrig

cjihrig approved these changes May 9, 2017

@mhdawson

This comment has been minimized.

Show comment
Hide comment
@mhdawson

mhdawson May 9, 2017

Member

I think the change should likely be made in the title as well (first line in the file), and anything that links to that title.

Member

mhdawson commented May 9, 2017

I think the change should likely be made in the title as well (first line in the file), and anything that links to that title.

Show outdated Hide outdated doc/api/addons.md
@@ -1,6 +1,6 @@
# C/C++ Addons

This comment has been minimized.

@gibfahn

gibfahn May 9, 2017

Member

Good point @mhdawson, worth changing this as well.

I get these matches with a quick (rip)grep, @mhdawson do the n-api ones need changing?

➜  node git:(master) ❯ rg "C/C\+\+ Addons"                                                                                                                                                   ~/wrk/com/node
doc/api/_toc.md
10:* [C/C++ Addons](addons.html)
11:* [C/C++ Addons - N-API](n-api.html)

doc/api/addons.md
1:# C/C++ Addons
234:section titled [C/C++ Addons - N-API](n-api.html).

doc/api/n-api.md
15:outlined in the section titled  [C/C++ Addons](addons.html).
@gibfahn

gibfahn May 9, 2017

Member

Good point @mhdawson, worth changing this as well.

I get these matches with a quick (rip)grep, @mhdawson do the n-api ones need changing?

➜  node git:(master) ❯ rg "C/C\+\+ Addons"                                                                                                                                                   ~/wrk/com/node
doc/api/_toc.md
10:* [C/C++ Addons](addons.html)
11:* [C/C++ Addons - N-API](n-api.html)

doc/api/addons.md
1:# C/C++ Addons
234:section titled [C/C++ Addons - N-API](n-api.html).

doc/api/n-api.md
15:outlined in the section titled  [C/C++ Addons](addons.html).

This comment has been minimized.

@mhdawson

mhdawson May 10, 2017

Member

The N-API actually supports writing Addons in C so C/C++ is correct for them. The references to addons.html of course need to be updated everywhere.

@mhdawson

mhdawson May 10, 2017

Member

The N-API actually supports writing Addons in C so C/C++ is correct for them. The references to addons.html of course need to be updated everywhere.

@refack

This comment has been minimized.

Show comment
Hide comment
@refack

refack May 10, 2017

Member

@gibfahn @mhdawson what I ment before is that: is this change correct now that we have N-API?

Maybe we should just remove the reference to a language?
Node.js Addons are dynamically-linked shared objects, written in C++, that
->
Node.js Addons are dynamically-linked shared objects that

Also at line 8:

At the moment, the method for implementing Addons is rather complicated,
involving knowledge of several components and APIs

->

At the moment, the method for implementing Addons without a helper library
such as [`nan`](#native-abstractions-for-nodejs) or using the [N-API](#n-api),
is rather complicated, involving knowledge of several components and APIs
Member

refack commented May 10, 2017

@gibfahn @mhdawson what I ment before is that: is this change correct now that we have N-API?

Maybe we should just remove the reference to a language?
Node.js Addons are dynamically-linked shared objects, written in C++, that
->
Node.js Addons are dynamically-linked shared objects that

Also at line 8:

At the moment, the method for implementing Addons is rather complicated,
involving knowledge of several components and APIs

->

At the moment, the method for implementing Addons without a helper library
such as [`nan`](#native-abstractions-for-nodejs) or using the [N-API](#n-api),
is rather complicated, involving knowledge of several components and APIs
@gibfahn

This comment has been minimized.

Show comment
Hide comment
@gibfahn

gibfahn May 10, 2017

Member

what I ment before is that: is this change correct now that we have N-API?

I suspect that this doc should be updated to cover n-api, but that's probably something that can/should be done separately. This change makes sense as-is.

Also if we talk about n-api we need to mention that it's experimental, so I don't think we should just add it in next to nan.

Member

gibfahn commented May 10, 2017

what I ment before is that: is this change correct now that we have N-API?

I suspect that this doc should be updated to cover n-api, but that's probably something that can/should be done separately. This change makes sense as-is.

Also if we talk about n-api we need to mention that it's experimental, so I don't think we should just add it in next to nan.

@refack

This comment has been minimized.

Show comment
Hide comment
@refack

refack May 10, 2017

Member

that's probably something that can/should be done separately

probably, but FWIW both nan and N-API are covered lower in the doc.

Member

refack commented May 10, 2017

that's probably something that can/should be done separately

probably, but FWIW both nan and N-API are covered lower in the doc.

@mhdawson

This comment has been minimized.

Show comment
Hide comment
@mhdawson

mhdawson May 10, 2017

Member

@refack I had the same discussion with @sam-github and he convinced me this change was ok as existing addons are C++ only and the original section only dealt with them. The nan and later N-API sections were inserted, but the overall flow text not updated well enough to integrate them nicely. What seemed to make sense was to let this change go in now, as it is valid to the section to which it applies, and then plan a more complete update once N-API is no longer experimental.

Member

mhdawson commented May 10, 2017

@refack I had the same discussion with @sam-github and he convinced me this change was ok as existing addons are C++ only and the original section only dealt with them. The nan and later N-API sections were inserted, but the overall flow text not updated well enough to integrate them nicely. What seemed to make sense was to let this change go in now, as it is valid to the section to which it applies, and then plan a more complete update once N-API is no longer experimental.

@refack

This comment has been minimized.

Show comment
Hide comment
@refack

refack May 10, 2017

Member

What seemed to make sense was to let this change go in now, as it is valid to the section to which it applies, and then plan a more complete update once N-API is no longer experimental.

Sound good.

Member

refack commented May 10, 2017

What seemed to make sense was to let this change go in now, as it is valid to the section to which it applies, and then plan a more complete update once N-API is no longer experimental.

Sound good.

@gibfahn

This comment has been minimized.

Show comment
Hide comment
@gibfahn
Member

gibfahn commented May 11, 2017

@mhdawson LGTY?

@gibfahn

This comment has been minimized.

Show comment
Hide comment
@mhdawson

LGTM

@sam-github

This comment has been minimized.

Show comment
Hide comment
@sam-github

sam-github May 11, 2017

Member

Landed in abfd4bf

Member

sam-github commented May 11, 2017

Landed in abfd4bf

@sam-github sam-github closed this May 11, 2017

sam-github added a commit that referenced this pull request May 11, 2017

doc: clarify node.js addons are c++
PR-URL: #12898
Fixes: #7129
Reviewed-By: Sam Roberts <vieuxtech@gmail.com>
Reviewed-By: Sakthipriyan Vairamani <thechargingvolcano@gmail.com>
Reviewed-By: Refael Ackermann <refack@gmail.com>
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Rich Trott <rtrott@gmail.com>
Reviewed-By: Gibson Fahnestock <gibfahn@gmail.com>
Reviewed-By: Timothy Gu <timothygu99@gmail.com>
Reviewed-By: Daniel Bevenius <daniel.bevenius@gmail.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Michael Dawson <michael_dawson@ca.ibm.com>

anchnk added a commit to anchnk/node that referenced this pull request May 19, 2017

doc: clarify node.js addons are c++
PR-URL: nodejs#12898
Fixes: nodejs#7129
Reviewed-By: Sam Roberts <vieuxtech@gmail.com>
Reviewed-By: Sakthipriyan Vairamani <thechargingvolcano@gmail.com>
Reviewed-By: Refael Ackermann <refack@gmail.com>
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Rich Trott <rtrott@gmail.com>
Reviewed-By: Gibson Fahnestock <gibfahn@gmail.com>
Reviewed-By: Timothy Gu <timothygu99@gmail.com>
Reviewed-By: Daniel Bevenius <daniel.bevenius@gmail.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Michael Dawson <michael_dawson@ca.ibm.com>

@jasnell jasnell referenced this pull request May 28, 2017

Closed

8.0.0 Release Proposal #12220

@gibfahn gibfahn referenced this pull request Jun 15, 2017

Closed

Auditing for 6.11.1 #230

2 of 3 tasks complete
@MylesBorins

This comment has been minimized.

Show comment
Hide comment
@MylesBorins

MylesBorins Jun 22, 2017

Member

This does not land cleanly in LTS. Please feel free to manually backport. Please also feel free to replace do-not-land if it is being backported

Member

MylesBorins commented Jun 22, 2017

This does not land cleanly in LTS. Please feel free to manually backport. Please also feel free to replace do-not-land if it is being backported

@BethGriggs BethGriggs deleted the BethGriggs:doc-api-addons branch Feb 21, 2018

gabrielschulhof added a commit to gabrielschulhof/node that referenced this pull request Mar 12, 2018

doc: clarify node.js addons are c++
PR-URL: nodejs#12898
Fixes: nodejs#7129
Reviewed-By: Sam Roberts <vieuxtech@gmail.com>
Reviewed-By: Sakthipriyan Vairamani <thechargingvolcano@gmail.com>
Reviewed-By: Refael Ackermann <refack@gmail.com>
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Rich Trott <rtrott@gmail.com>
Reviewed-By: Gibson Fahnestock <gibfahn@gmail.com>
Reviewed-By: Timothy Gu <timothygu99@gmail.com>
Reviewed-By: Daniel Bevenius <daniel.bevenius@gmail.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Michael Dawson <michael_dawson@ca.ibm.com>

gabrielschulhof added a commit to gabrielschulhof/node that referenced this pull request Mar 31, 2018

doc: clarify node.js addons are c++
PR-URL: nodejs#12898
Fixes: nodejs#7129
Reviewed-By: Sam Roberts <vieuxtech@gmail.com>
Reviewed-By: Sakthipriyan Vairamani <thechargingvolcano@gmail.com>
Reviewed-By: Refael Ackermann <refack@gmail.com>
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Rich Trott <rtrott@gmail.com>
Reviewed-By: Gibson Fahnestock <gibfahn@gmail.com>
Reviewed-By: Timothy Gu <timothygu99@gmail.com>
Reviewed-By: Daniel Bevenius <daniel.bevenius@gmail.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Michael Dawson <michael_dawson@ca.ibm.com>

gabrielschulhof added a commit to gabrielschulhof/node that referenced this pull request Apr 10, 2018

doc: clarify node.js addons are c++
PR-URL: nodejs#12898
Fixes: nodejs#7129
Reviewed-By: Sam Roberts <vieuxtech@gmail.com>
Reviewed-By: Sakthipriyan Vairamani <thechargingvolcano@gmail.com>
Reviewed-By: Refael Ackermann <refack@gmail.com>
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Rich Trott <rtrott@gmail.com>
Reviewed-By: Gibson Fahnestock <gibfahn@gmail.com>
Reviewed-By: Timothy Gu <timothygu99@gmail.com>
Reviewed-By: Daniel Bevenius <daniel.bevenius@gmail.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Michael Dawson <michael_dawson@ca.ibm.com>

MylesBorins added a commit that referenced this pull request Apr 16, 2018

doc: clarify node.js addons are c++
Backport-PR-URL: #19447
PR-URL: #12898
Fixes: #7129
Reviewed-By: Sam Roberts <vieuxtech@gmail.com>
Reviewed-By: Sakthipriyan Vairamani <thechargingvolcano@gmail.com>
Reviewed-By: Refael Ackermann <refack@gmail.com>
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Rich Trott <rtrott@gmail.com>
Reviewed-By: Gibson Fahnestock <gibfahn@gmail.com>
Reviewed-By: Timothy Gu <timothygu99@gmail.com>
Reviewed-By: Daniel Bevenius <daniel.bevenius@gmail.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Michael Dawson <michael_dawson@ca.ibm.com>

@MylesBorins MylesBorins referenced this pull request Apr 16, 2018

Merged

v6.14.2 proposal #19996

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment