doc: better linkaged to node-addon-api #24371

mhdawson · 2018-11-14T22:41:03Z

One of the comments we got at the N-API workshop
at NodeConfEU was that we should have a better link to
node-addon-api and the docs in the main API docs for
N-API. The goal being to help people find node-addon-api
and potentially start with the node-addon-api docs
instead if they are using C++.

This expands and strengthens the link along with a
recommendation that starting with the node-addon-api
docs might make sense.

Checklist

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

nodejs-github-bot · 2018-11-14T22:41:04Z

@mhdawson build started: https://ci.nodejs.org/blue/organizations/jenkins/node-test-pull-request-lite-pipeline/detail/node-test-pull-request-lite-pipeline/1617/pipeline

Trott · 2018-11-15T02:26:09Z

Note for whoever lands this: first line of commit message seems to have a typo?

Trott · 2018-11-15T02:43:57Z

doc/api/n-api.md

-part of N-API, nor will they be maintained as part of Node.js. One such
-example is: [node-addon-api](https://github.com/nodejs/node-addon-api).
+and different compiler levels. **However, a C++ API can be easier to use
+in many cases.** In order to support using C++ the project maintains a


Do we really need to add bold text here? (No strong opinion other than we have a tendency to overuse bold and italics so it always seems worth asking.)

If you really want people to take notice of the sentence, make it more direct by getting rid of qualifiers like "However" and "in many cases" so it's just:

A C++ API can be easier to use.

Maybe even set it off as its own single-sentence paragraph if it's that important.

In order to -> To

Comma needed after C++

Trott · 2018-11-15T02:49:30Z

doc/api/n-api.md

+obj["foo"] = String::New(env, "bar");
+```
+
+what actually gets used in the addon is:


Splitting the sentence up with examples and having this fragment here makes it pretty hard to read/understand IMO.

Trott · 2018-11-15T02:49:42Z

doc/api/n-api.md

+This wrapper provides an inlineable C++ API. Binaries built
+with `node-addon-api` will depend on the symbols for the N-API C based
+functions exported by Node.js. `node-addon-api` can be seen as a more
+efficient way to write code that calls N-API. For example if the following


Comma needed after example

Trott · 2018-11-15T02:50:43Z

doc/api/n-api.md

+C++ wrapper module called
+[node-addon-api](https://github.com/nodejs/node-addon-api).
+This wrapper provides an inlineable C++ API. Binaries built
+with `node-addon-api` will depend on the symbols for the N-API C based


C based -> C-based

Is this sentence necessary? Can it be removed? It might be my lack of domain knowledge here, but it's unclear to me why this sentence is necessary given the surrounding material.

I think it's a key point so I'd like to leave it in.

Trott · 2018-11-15T03:01:19Z

doc/api/n-api.md

+
+While the concepts in the following sections are still useful when
+using `node-addon-api` instead of the C APIs, it may make sense
+to start with the API docs for `node-addon-api` instead.


This sentence is long, complicated, and contains too many qualifiers. How about splitting it up into at least two sentences? Also, provide a link to the API docs you're recommending people use?

Maybe all it really needs to be is?:

When using `node-addon-api` instead of the C APIs, start with the API docs for node-addon-api.

Trott · 2018-11-15T03:09:38Z

doc/api/n-api.md

+}
+```
+
+The end result is that the addon only uses the exported C APIs and as a result


Suggested change

The end result is that the addon only uses the exported C APIs and as a result

The end result is that the addon only uses the exported C APIs. As a result, it

Trott · 2018-11-15T03:10:58Z

doc/api/n-api.md

+[node-addon-api](https://github.com/nodejs/node-addon-api).
+This wrapper provides an inlineable C++ API. Binaries built
+with `node-addon-api` will depend on the symbols for the N-API C based
+functions exported by Node.js. `node-addon-api` can be seen as a more


Suggested change

functions exported by Node.js. `node-addon-api` can be seen as a more

functions exported by Node.js. `node-addon-api` is a more

Trott · 2018-11-15T03:12:27Z

doc/api/n-api.md

+with `node-addon-api` will depend on the symbols for the N-API C based
+functions exported by Node.js. `node-addon-api` can be seen as a more
+efficient way to write code that calls N-API. For example if the following
+`node-addon-api` code is written:


Maybe replace the whole lead-up here with this:

Take, for example, the following `node-addon-api` code:

Trott · 2018-11-15T03:12:45Z

doc/api/n-api.md

+obj["foo"] = String::New(env, "bar");
+```
+
+what actually gets used in the addon is:


Suggested change

what actually gets used in the addon is:

What actually gets used in the addon is:

One of the comments we got at the N-API workshop at NodeConfEU was that we should have a better link to node-addon-api and the docs in the main API docs for N-API. The goal being to help people find node-addon-api and potentially start with the node-addon-api docs instead if they are using C++. This expands and strengthens the link along with a recommendation that starting with the node-addon-api docs might make sense.

mhdawson · 2018-11-15T14:18:28Z

@Trott I believe I've addressed your comments.

thefourtheye · 2018-11-15T16:42:13Z

doc/api/n-api.md

-part of N-API, nor will they be maintained as part of Node.js. One such
-example is: [node-addon-api](https://github.com/nodejs/node-addon-api).
+and different compiler levels. A C++ API can be easier to use
+. To support using C++, the project maintains a


Nit: Seeing a line starting with a dot, seems not correct.

thefourtheye · 2018-11-15T16:43:05Z

doc/api/n-api.md

+functions exported by Node.js. `node-addon-api` is a more
+efficient way to write code that calls N-API. Take, for example, the 
+following `node-addon-api` code. The first section shows the
+`node-add-api` code and the section section shows what actually gets


node-addon-api, right?

Also, there are two "section"s.

doc/api/n-api.md

mhdawson · 2018-11-15T19:27:41Z

@thefourtheye thanks for the comments, I believe I've addressed them now.

doc/api/n-api.md

vsemozhetbyt

Doc form LGTM)

mhdawson · 2018-11-16T14:28:06Z

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

mhdawson · 2018-11-16T22:27:01Z

CI good landing.

mhdawson · 2018-11-16T22:33:02Z

Landed as a569686

One of the comments we got at the N-API workshop at NodeConfEU was that we should have a better link to node-addon-api and the docs in the main API docs for N-API. The goal being to help people find node-addon-api and potentially start with the node-addon-api docs instead if they are using C++. This expands and strengthens the link along with a recommendation that starting with the node-addon-api docs might make sense. PR-URL: #24371 Reviewed-By: Sakthipriyan Vairamani <thechargingvolcano@gmail.com> Reviewed-By: Vse Mozhet Byt <vsemozhetbyt@gmail.com> Reviewed-By: Franziska Hinkelmann <franziska.hinkelmann@gmail.com>

nodejs-github-bot added doc Issues and PRs related to the documentations. node-api Issues and PRs related to the Node-API. labels Nov 14, 2018

Trott reviewed Nov 15, 2018

View reviewed changes

mhdawson force-pushed the improve-node-addon-api-reference branch from f778998 to 7659fc1 Compare November 15, 2018 14:16

squash: address comments

040e4bb

thefourtheye approved these changes Nov 15, 2018

View reviewed changes

squash: address comments

2b1489b

vsemozhetbyt reviewed Nov 15, 2018

View reviewed changes

doc/api/n-api.md Outdated Show resolved Hide resolved

squash: address comments

c0de9f2

vsemozhetbyt approved these changes Nov 15, 2018

View reviewed changes

fhinkel approved these changes Nov 16, 2018

View reviewed changes

mhdawson closed this Nov 16, 2018

BridgeAR mentioned this pull request Dec 5, 2018

v11.4.0 proposal #24854

Merged

4 tasks

codebytere mentioned this pull request Jan 15, 2019

v10.15.1 proposal #25346

Merged

mhdawson deleted the improve-node-addon-api-reference branch September 30, 2019 13:13

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

doc: better linkaged to node-addon-api #24371

doc: better linkaged to node-addon-api #24371

mhdawson commented Nov 14, 2018

nodejs-github-bot commented Nov 14, 2018

Trott commented Nov 15, 2018

Trott Nov 15, 2018

Trott Nov 15, 2018

Trott Nov 15, 2018

Trott Nov 15, 2018

Trott Nov 15, 2018

Trott Nov 15, 2018

Trott Nov 15, 2018

Trott Nov 15, 2018

mhdawson Nov 15, 2018

Trott Nov 15, 2018

Trott Nov 15, 2018

Trott Nov 15, 2018

Trott Nov 15, 2018

Trott Nov 15, 2018

Trott Nov 15, 2018

mhdawson commented Nov 15, 2018

thefourtheye Nov 15, 2018

thefourtheye Nov 15, 2018

thefourtheye Nov 15, 2018

mhdawson commented Nov 15, 2018

vsemozhetbyt left a comment •

edited

Loading

mhdawson commented Nov 16, 2018 •

edited

Loading

mhdawson commented Nov 16, 2018

mhdawson commented Nov 16, 2018

	The end result is that the addon only uses the exported C APIs and as a result
	The end result is that the addon only uses the exported C APIs. As a result, it

	functions exported by Node.js. `node-addon-api` can be seen as a more
	functions exported by Node.js. `node-addon-api` is a more

	what actually gets used in the addon is:
	What actually gets used in the addon is:

doc: better linkaged to node-addon-api #24371

doc: better linkaged to node-addon-api #24371

Conversation

mhdawson commented Nov 14, 2018

Checklist

nodejs-github-bot commented Nov 14, 2018

Trott commented Nov 15, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mhdawson commented Nov 15, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mhdawson commented Nov 15, 2018

vsemozhetbyt left a comment • edited Loading

Choose a reason for hiding this comment

mhdawson commented Nov 16, 2018 • edited Loading

mhdawson commented Nov 16, 2018

mhdawson commented Nov 16, 2018

vsemozhetbyt left a comment •

edited

Loading

mhdawson commented Nov 16, 2018 •

edited

Loading