doc: Add initial documentation for N-API by mhdawson · Pull Request #12549 · nodejs/node · GitHub

Add the initial documentation for the N-API

This PR is a result of work in the abi-stable-node repo:
https://github.com/nodejs/abi-stable-node/tree/doc,
with this PR being the cumulative work on the documentation
sections in that repo with the following contributors
in alphabetical order:

Author: Arunesh Chandra <arunesh.chandra@microsoft.com>
Author: Gabriel Schulhof <gabriel.schulhof@intel.com>
Author: Hitesh Kanwathirtha <hiteshk@microsoft.com>
Author: Jason Ginchereau <jasongin@microsoft.com>
Author: Michael Dawson <michael_dawson@ca.ibm.com>
Author: Sampson Gao <sampsong@ca.ibm.com>
Author: Taylor Woll <taylor.woll@microsoft.com>

Checklist

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

Affected core subsystem(s)

doc, n-api

Should this be titled something like "C/C++ Addons (N-API)", so that it is adjacent to the other "C/C++ Addons" topic?

makes sense to me, will update once we have more comments.

documented?

fixed

Didn't make it through the entire PR, but it looks pretty good. Can you make an effort to remove language like "your". 👍

I would drop newer here. Otherwise, it will probably claim to be new for years to come.

I don't know if we standardized anywhere, but you have two spaces after some sentences, and one period after others.

Fixed.

We should standardise, probably on 1.

Add a space after "document"

done

This sentence begins and ends with "instead." I think the second one can be dropped.

done

Same comments in these first two paragraphs apply.

done

Maybe start a new sentence at "and the"

done

This could probably be combined with the last sentence of the previous paragraph.

done

Does catch need to be in single quotes?

does flow better without, removed.

Is this guaranteed to be an Error object, or can it be whatever was thrown by JS?

That is a good point, it will be whatever was thrown.

error -> Error

fixed

the second -> in the section?

Fixed.

missing backticks above

Fixed

missing backticks above

Fixed

buggy space

Fixed

wrong backtick

fixed

data' -> data`

It seems this is a bug in GitHub: this comment is for line 2873, but it is misplaced here due to possibly long diff (+ diff is truncated here). Beware other comments for the distant lines may be misplaced too.

fixed

Till #12554 fixed, maybe it is reasonable to split very big docs into several parts for review time.

Did a pass

replacing spaces after period with one space
moving use of your

@cjihrig @vsemozhetbyt pushed commits to address all comments so far.

I'm happy to split up into separate PRs or review in this single PR. I'll leave it up to the reviewers to comment on which they think in best.

N-API

fixed

V8

done

V8

done

Added a rendered link in the OP, points to the branch, not a specific commit: https://github.com/mhdawson/io.js/blob/napi-doc/doc/api/n-api.md

Can we get rid of the Signature, Parameters, Return value, Description mini-headers? They take up a lot of space in the long document and are unnecessary. We could move it to be more like a JS api example, for consistency and readability.

Consider this comparison:

napi_close_handle_scope

Signature

NODE_EXTERN napi_status napi_close_handle_scope(napi_env e,
                                                napi_handle_scope scope);

Parameters

[in] env : The environment that the API is invoked under
[in] scope: napi_value representing the scope to be closed

Return value

napi_ok if the API succeeded.

Description

This API closes the scope passed in. Scopes must be closed in the
reverse order from which they were created.

vs

napi_close_handle_scope

NODE_EXTERN napi_status napi_close_handle_scope(napi_env e,
                                                napi_handle_scope scope);

[in] env : The environment that the API is invoked under
[in] scope: napi_value representing the scope to be closed

Returns napi_ok if the API succeeded.

This API closes the scope passed in. Scopes must be closed in the
reverse order from which they were created.

Also, can we add added-in placeholders to the API methods?

created.o is a typo

fixed

This paragraph seems to be talking about C++ wrappers around N-API, so I would strike faster here – they would be easier but they would inherently be limited to being as fast as N-API at best

meant faster in terms of development not speed, but remove to avoid confusion

TODO?

fixed

Notes should be on separate paragraphs and not be all uppercase, followed by a capitalized sentence, i.e. *Note:* You should …

Done, and fixed other instances as well.

This is not the actual signature, though?

No, to keep thing shorter and because we have the full signature/details in the later sections moved it from these earlier references.

Maybe adding ... for the parameters would help ?

typo: Tthe and double space after it

fixed

If you’re talking about the data pointer possibly being moved: That does not happen. :)

removed TODO.

typo: returns, also s/C++/C/ again

fixed, and also went through and looked for other cases as well.

s/C++/C/ (going to stop commenting that now 😄)

typo: lowercase pointer

fixed

These should probably mention what happens when the value does not fit into the corresponding range?

Couple of issues

Should include the Experimental stability index flag here.

ok, we have that in the actual section on n-api but I can add here as well

Having it in both places is good.

Please avoid the use of you in the docs... here and throughout.

Did a pass removing all of the 'you's.

Here and throughout, can we make sure that the signature has the same names as the listed parameters?

e.g. e should be env to match.

fixed all of the env instances.

@addaleax @jasnell @Fishrock123 pushed commits to address all of the comments so far. Note that the short links render ok when you do make doc and view as html as opposed to what you see when you look at the rendered markdown link you posted.

This does not pass new doc linting:

  1987:1   error  Unexpected var, use let or const instead                                       no-var
  2011:1   error  Unexpected var, use let or const instead                                       no-var
  2035:1   error  Unexpected var, use let or const instead                                       no-var
  2036:1   error  Unexpected var, use let or const instead                                       no-var
  2056:1   error  Unexpected var, use let or const instead                                       no-var
  2455:1   error  Opening curly brace does not appear on the same line as controlling statement  brace-style
  2456:5   error  Expected indentation of 2 spaces but found 4                                   indent
  2546:1   error  Unexpected var, use let or const instead                                       no-var
  2630:1   error  Opening curly brace does not appear on the same line as controlling statement  brace-style
  2631:5   error  Expected indentation of 2 spaces but found 4                                   indent
  2634:1   error  Unexpected var, use let or const instead                                       no-var
  2634:11  error  Strings must use singlequote                                                   quotes
  2635:1   error  Unexpected var, use let or const instead                                       no-var

✖ 13 problems (13 errors, 0 warnings)

Lines like this also may need to be addressed for consistency:

// var obj = {}

Testing Linter CI: https://ci.nodejs.org/job/node-test-linter/8536/

Strange. Why Linting passes on CI?

Testing if full CI also passes with linting: https://ci.nodejs.org/job/node-test-pull-request/7659/ (passes, 1 Windows fail seems unrelated).

See #12635 and #12640

Looking good! Couple more comments.

Also, according to the github diff renderer formatting breaks halfway through... It is possible that is just because the diff it too large but we should check for markdown errors in case.

Should this be an enum def?

change to be enum definition

Can we apply the same hear rules to e.g. here?

Not sure what you mean here.

If you meant removing the headers (Definition and Members) I went ahead and did that. Seems better to me.

- `member`: Regular grammar rules.

Ditto for the parameters section of most APIs. I think this is the style we use elsewhere.

Not sure what you mean here either.

I think the point is:

error_code -> `error_code`

Ok guessing that it meant the parameter sections use use regular grammar including capital at the start and period at the end. Went through and fixed all that up.

Ok, fixed error_code -> error_code as well. It was pretty much ok except for the instance identified.

Yeah, that's what I meant. Code blocks and grammar. Sorry.

Can we use 1 space between [in/out] and name? Here and throughout. They don't render with more than 1 space it seems anyways?

(Unless make doc does, but I doubt it?)

Done

Add the initial documentation for the N-API This PR is a result of work in the abi-stable-node repo: https://github.com/nodejs/abi-stable-node/tree/doc, with this PR being the cumulative work on the documentation sections in that repo with the following contributors in alphabetical order: Author: Arunesh Chandra <arunesh.chandra@microsoft.com> Author: Gabriel Schulhof <gabriel.schulhof@intel.com> Author: Hitesh Kanwathirtha <hiteshk@microsoft.com> Author: Jason Ginchereau <jasongin@microsoft.com> Author: Michael Dawson <michael_dawson@ca.ibm.com> Author: Sampson Gao <sampsong@ca.ibm.com> Author: Taylor Woll <taylor.woll@microsoft.com>

@Fishrock123 github/large file may not be helping us here as I'm not 100% sure what's left with respect to https://github.com/nodejs/node/pull/12549/files#r112795649. I already added the stability index as James requested and I think that's the comment that that one shows me.

@Fishrock123 I don't have any great ideas on the headings right now, so I'd prefer to land this and then I can spend a bit more time and maybe come up with something in a later PR.

@mhdawson Oh whoops, didn't see it on the line below. LGTM to me on doc style. (I didn't review the APIs in detail.)

@addaleax I think I've addressed your issues so I plan to land soon. If not just let me know.

CI run: https://ci.nodejs.org/job/node-test-pull-request/7734/

outdated

@mhdawson Yeah, sorry, didn’t have the time to give it a full review. So please don’t let me hold up anything! :)

@addaleax thanks. Of course if you spot anything later on just let me know and I'll cover it in a later PR.

Ok CI good, landing.

Add the initial documentation for the N-API This PR is a result of work in the abi-stable-node repo: https://github.com/nodejs/abi-stable-node/tree/doc, with this PR being the cumulative work on the documentation sections in that repo with the following contributors in alphabetical order: Author: Arunesh Chandra <arunesh.chandra@microsoft.com> Author: Gabriel Schulhof <gabriel.schulhof@intel.com> Author: Hitesh Kanwathirtha <hiteshk@microsoft.com> Author: Jason Ginchereau <jasongin@microsoft.com> Author: Michael Dawson <michael_dawson@ca.ibm.com> Author: Sampson Gao <sampsong@ca.ibm.com> Author: Taylor Woll <taylor.woll@microsoft.com> PR-URL: #12549 Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Jeremiah Senkpiel <fishrock123@rocketmail.com> Reviewed-By: Colin Ihrig <cjihrig@gmail.com>

landed as deb9622

Add the initial documentation for the N-API This PR is a result of work in the abi-stable-node repo: https://github.com/nodejs/abi-stable-node/tree/doc, with this PR being the cumulative work on the documentation sections in that repo with the following contributors in alphabetical order: Author: Arunesh Chandra <arunesh.chandra@microsoft.com> Author: Gabriel Schulhof <gabriel.schulhof@intel.com> Author: Hitesh Kanwathirtha <hiteshk@microsoft.com> Author: Jason Ginchereau <jasongin@microsoft.com> Author: Michael Dawson <michael_dawson@ca.ibm.com> Author: Sampson Gao <sampsong@ca.ibm.com> Author: Taylor Woll <taylor.woll@microsoft.com> PR-URL: #12549 Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Jeremiah Senkpiel <fishrock123@rocketmail.com> Reviewed-By: Colin Ihrig <cjihrig@gmail.com>

NM

Add the initial documentation for the N-API This PR is a result of work in the abi-stable-node repo: https://github.com/nodejs/abi-stable-node/tree/doc, with this PR being the cumulative work on the documentation sections in that repo with the following contributors in alphabetical order: Author: Arunesh Chandra <arunesh.chandra@microsoft.com> Author: Gabriel Schulhof <gabriel.schulhof@intel.com> Author: Hitesh Kanwathirtha <hiteshk@microsoft.com> Author: Jason Ginchereau <jasongin@microsoft.com> Author: Michael Dawson <michael_dawson@ca.ibm.com> Author: Sampson Gao <sampsong@ca.ibm.com> Author: Taylor Woll <taylor.woll@microsoft.com> PR-URL: nodejs#12549 Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Jeremiah Senkpiel <fishrock123@rocketmail.com> Reviewed-By: Colin Ihrig <cjihrig@gmail.com>

Add the initial documentation for the N-API This PR is a result of work in the abi-stable-node repo: https://github.com/nodejs/abi-stable-node/tree/doc, with this PR being the cumulative work on the documentation sections in that repo with the following contributors in alphabetical order: Author: Arunesh Chandra <arunesh.chandra@microsoft.com> Author: Gabriel Schulhof <gabriel.schulhof@intel.com> Author: Hitesh Kanwathirtha <hiteshk@microsoft.com> Author: Jason Ginchereau <jasongin@microsoft.com> Author: Michael Dawson <michael_dawson@ca.ibm.com> Author: Sampson Gao <sampsong@ca.ibm.com> Author: Taylor Woll <taylor.woll@microsoft.com> Backport-PR-URL: #19447 PR-URL: #12549 Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Jeremiah Senkpiel <fishrock123@rocketmail.com> Reviewed-By: Colin Ihrig <cjihrig@gmail.com>

nodejs-github-bot added the doc Issues and PRs related to the documentations. label Apr 20, 2017

jasongin reviewed Apr 20, 2017

View reviewed changes

mscdex added the node-api Issues and PRs related to the Node-API. label Apr 20, 2017

vsemozhetbyt reviewed Apr 20, 2017

View reviewed changes

cjihrig reviewed Apr 20, 2017

View reviewed changes

vsemozhetbyt reviewed Apr 20, 2017

View reviewed changes

vsemozhetbyt mentioned this pull request Apr 20, 2017

meta: beware review comments for long diffs #12554

Closed

mhdawson force-pushed the napi-doc branch from e8f9c72 to 4e4d307 Compare April 21, 2017 14:57

Fishrock123 reviewed Apr 21, 2017

View reviewed changes

addaleax previously requested changes Apr 22, 2017

View reviewed changes

jasnell requested changes Apr 22, 2017

View reviewed changes

Fishrock123 suggested changes Apr 24, 2017

View reviewed changes

vsemozhetbyt mentioned this pull request Apr 25, 2017

doc, tools: doc linting does not work in CI #12635

Closed

Fishrock123 suggested changes Apr 25, 2017

View reviewed changes

Fishrock123 approved these changes Apr 28, 2017

View reviewed changes

mhdawson closed this Apr 28, 2017

mhdawson reopened this Apr 28, 2017

mhdawson closed this Apr 28, 2017

evanlucas mentioned this pull request May 1, 2017

v7.10.0 proposal #12775

Merged

evanlucas added the dont-land-on-v7.x label May 3, 2017

addaleax added dont-land-on-v4.x labels May 4, 2017

jasnell mentioned this pull request May 11, 2017

8.0.0 Release Proposal #12220

Closed

mhdawson deleted the napi-doc branch June 28, 2017 19:24

MylesBorins mentioned this pull request Apr 16, 2018

v6.14.2 proposal #19996

Merged

doc: Add initial documentation for N-API by mhdawson · Pull Request #12549 · nodejs/node · GitHub

Navigation Menu

Uh oh!

Uh oh!

doc: Add initial documentation for N-API - #12549

doc: Add initial documentation for N-API#12549mhdawson wants to merge 22 commits intonodejs:masternodejs/node:masterfrom mhdawson:napi-docmhdawson/io.js:napi-docCopy head branch name to clipboard

Conversation

mhdawson commented Apr 20, 2017 • edited by Fishrock123 Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Checklist

Affected core subsystem(s)

Uh oh!

jasongin Apr 20, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

cjihrig left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

vsemozhetbyt Apr 20, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

doc: Add initial documentation for N-API#12549
mhdawson wants to merge 22 commits into
nodejs:masterfrom
mhdawson:napi-doc

mhdawson commented Apr 20, 2017 •
edited by Fishrock123
Loading

jasongin Apr 20, 2017 •
edited
Loading

vsemozhetbyt Apr 20, 2017 •
edited
Loading

vsemozhetbyt Apr 20, 2017 •
edited
Loading