doc: Add initial documentation for N-API #12549

mhdawson · 2017-04-20T18:25:08Z

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 <[email protected]>
Author: Gabriel Schulhof <[email protected]>
Author: Hitesh Kanwathirtha <[email protected]>
Author: Jason Ginchereau <[email protected]>
Author: Michael Dawson <[email protected]>
Author: Sampson Gao <[email protected]>
Author: Taylor Woll <[email protected]>

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

jasongin · 2017-04-20T18:29:07Z

doc/api/_toc.md

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.

vsemozhetbyt · 2017-04-20T19:07:30Z

doc/api/addons.md

documented?

cjihrig

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

cjihrig · 2017-04-20T19:20:36Z

doc/api/addons.md

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

cjihrig · 2017-04-20T19:21:07Z

doc/api/addons.md

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

We should standardise, probably on 1.

cjihrig · 2017-04-20T19:23:13Z

doc/api/addons.md

Add a space after "document"

cjihrig · 2017-04-20T19:24:01Z

doc/api/addons.md

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

cjihrig · 2017-04-20T19:24:59Z

doc/api/n-api.md

Same comments in these first two paragraphs apply.

cjihrig · 2017-04-20T19:52:52Z

doc/api/n-api.md

Maybe start a new sentence at "and the"

cjihrig · 2017-04-20T19:54:44Z

doc/api/n-api.md

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

cjihrig · 2017-04-20T19:55:56Z

doc/api/n-api.md

Does catch need to be in single quotes?

does flow better without, removed.

cjihrig · 2017-04-20T19:57:26Z

doc/api/n-api.md

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.

cjihrig · 2017-04-20T19:58:25Z

doc/api/n-api.md

error -> Error

vsemozhetbyt · 2017-04-20T20:37:43Z

doc/api/n-api.md

the second -> in the section?

vsemozhetbyt · 2017-04-20T21:01:44Z

doc/api/n-api.md

missing backticks above

vsemozhetbyt · 2017-04-20T21:04:11Z

doc/api/n-api.md

missing backticks above

vsemozhetbyt · 2017-04-20T21:06:01Z

doc/api/n-api.md

buggy space

vsemozhetbyt · 2017-04-20T21:10:15Z

doc/api/n-api.md

wrong backtick

vsemozhetbyt · 2017-04-20T21:14:59Z

doc/api/n-api.md

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.

vsemozhetbyt · 2017-04-20T22:15:12Z

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

mhdawson · 2017-04-21T14:42:04Z

Did a pass

replacing spaces after period with one space
moving use of your

mhdawson · 2017-04-21T14:58:36Z

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

mhdawson · 2017-04-21T15:00:15Z

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.

Fishrock123 · 2017-04-21T15:13:46Z

doc/api/n-api.md

Fishrock123 · 2017-04-21T15:14:38Z

doc/api/n-api.md

Fishrock123 · 2017-04-21T15:15:28Z

doc/api/addons.md

Fishrock123 · 2017-04-21T15:17:48Z

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

Fishrock123 · 2017-04-21T15:24:15Z

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.

Fishrock123 · 2017-04-21T15:25:22Z

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

Fishrock123 · 2017-04-21T15:26:12Z

doc/api/n-api.md

created.o is a typo

addaleax · 2017-04-21T23:06:41Z

doc/api/n-api.md

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

addaleax · 2017-04-21T23:09:03Z

doc/api/n-api.md

addaleax · 2017-04-21T23:12:17Z

doc/api/n-api.md

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.

addaleax · 2017-04-21T23:13:53Z

doc/api/n-api.md

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 ?

addaleax · 2017-04-21T23:14:28Z

doc/api/n-api.md

typo: Tthe and double space after it

addaleax · 2017-04-21T23:44:20Z

doc/api/n-api.md

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

removed TODO.

addaleax · 2017-04-21T23:59:35Z

doc/api/n-api.md

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

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

addaleax · 2017-04-22T00:00:01Z

doc/api/n-api.md

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

addaleax · 2017-04-22T00:00:16Z

doc/api/n-api.md

typo: lowercase pointer

addaleax · 2017-04-22T00:01:08Z

doc/api/n-api.md

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

jasnell

Couple of issues

jasnell · 2017-04-22T00:04:44Z

doc/api/addons.md

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.

jasnell · 2017-04-22T00:07:28Z

doc/api/n-api.md

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

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

Fishrock123 · 2017-04-24T15:56:22Z

doc/api/n-api.md

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.

mhdawson · 2017-04-24T22:33:06Z

@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.

vsemozhetbyt · 2017-04-25T00:50:08Z

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

Fishrock123

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.

Fishrock123 · 2017-04-25T15:14:37Z

doc/api/n-api.md

Should this be an enum def?

change to be enum definition

Fishrock123 · 2017-04-25T15:14:54Z

doc/api/n-api.md

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.

Fishrock123 · 2017-04-25T15:16:21Z

doc/api/n-api.md

- `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.

Fishrock123 · 2017-04-25T15:19:17Z

doc/api/n-api.md

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?)

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 <[email protected]> Author: Gabriel Schulhof <[email protected]> Author: Hitesh Kanwathirtha <[email protected]> Author: Jason Ginchereau <[email protected]> Author: Michael Dawson <[email protected]> Author: Sampson Gao <[email protected]> Author: Taylor Woll <[email protected]>

mhdawson · 2017-04-27T18:52:07Z

@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.

mhdawson · 2017-04-27T18:55:55Z

@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.

Fishrock123

@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.)

mhdawson · 2017-04-28T19:51:29Z

@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

addaleax · 2017-04-28T20:22:35Z

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

mhdawson · 2017-04-28T20:29:03Z

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

mhdawson · 2017-04-28T20:56:07Z

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 <[email protected]> Author: Gabriel Schulhof <[email protected]> Author: Hitesh Kanwathirtha <[email protected]> Author: Jason Ginchereau <[email protected]> Author: Michael Dawson <[email protected]> Author: Sampson Gao <[email protected]> Author: Taylor Woll <[email protected]> PR-URL: #12549 Reviewed-By: James M Snell <[email protected]> Reviewed-By: Jeremiah Senkpiel <[email protected]> Reviewed-By: Colin Ihrig <[email protected]>

mhdawson · 2017-04-28T21:09:57Z

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 <[email protected]> Author: Gabriel Schulhof <[email protected]> Author: Hitesh Kanwathirtha <[email protected]> Author: Jason Ginchereau <[email protected]> Author: Michael Dawson <[email protected]> Author: Sampson Gao <[email protected]> Author: Taylor Woll <[email protected]> PR-URL: #12549 Reviewed-By: James M Snell <[email protected]> Reviewed-By: Jeremiah Senkpiel <[email protected]> Reviewed-By: Colin Ihrig <[email protected]>

refack · 2017-05-08T18:50:54Z

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 <[email protected]> Author: Gabriel Schulhof <[email protected]> Author: Hitesh Kanwathirtha <[email protected]> Author: Jason Ginchereau <[email protected]> Author: Michael Dawson <[email protected]> Author: Sampson Gao <[email protected]> Author: Taylor Woll <[email protected]> PR-URL: nodejs#12549 Reviewed-By: James M Snell <[email protected]> Reviewed-By: Jeremiah Senkpiel <[email protected]> Reviewed-By: Colin Ihrig <[email protected]>

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 <[email protected]> Author: Gabriel Schulhof <[email protected]> Author: Hitesh Kanwathirtha <[email protected]> Author: Jason Ginchereau <[email protected]> Author: Michael Dawson <[email protected]> Author: Sampson Gao <[email protected]> Author: Taylor Woll <[email protected]> Backport-PR-URL: #19447 PR-URL: #12549 Reviewed-By: James M Snell <[email protected]> Reviewed-By: Jeremiah Senkpiel <[email protected]> Reviewed-By: Colin Ihrig <[email protected]>

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

Uh oh!

doc: Add initial documentation for N-API #12549

doc: Add initial documentation for N-API #12549

Uh oh!

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

Uh oh!

Choose a reason for hiding this comment

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