doc: add guide for updating N-API API surface #21877

digitalinfinity · 2018-07-19T02:03:25Z

This adds a new guide that outlines the principles and guidelines for contributing a new API to the N-API surface. These guidelines were formulated based on discussions in the API working group.

Refs: nodejs/abi-stable-node#301

Checklist

documentation is changed or added
commit message follows commit guidelines

This adds a new guide that outlines the principles and guidelines for contributing a new API to the N-API surface. These guidelines were formulated based on discussions in the API working group. Refs: nodejs/abi-stable-node#301

digitalinfinity · 2018-07-19T02:06:09Z

cc @nodejs/n-api

For more context, this document was written up to help provide node collaborators a framework to reason about contributions to the N-API surface. However, this is not a locked down document- rather, this PR is intended to solicit additional feedback from the broader community

Also, after chatting with @mhdawson, it seemed like doc/guides was the best place for such a document to live but if there's a better place to move this to, happy to update the PR.

jasnell · 2018-07-19T13:30:05Z

doc/guides/adding-new-napi-api.md

@@ -0,0 +1,31 @@
+# Contributing a new API to N-API
+
+N-API is Node's next generation ABI-stable API for native modules. While improving the API surface is encouraged and welcomed, the following are a set of principles and guidelines to keep in mind while adding a new N-API API.


Please line break at <=80 chars... makes it significantly easier to review :-)

jasnell · 2018-07-19T13:30:35Z

doc/guides/adding-new-napi-api.md

+
+N-API is Node's next generation ABI-stable API for native modules. While improving the API surface is encouraged and welcomed, the following are a set of principles and guidelines to keep in mind while adding a new N-API API.
+
+-	A new API **must** adhere to N-API API shape and spirit


nit: for consistency, please use * instead of - for bullet lists.

jasnell

Couple of nits, but LGTM!

vsemozhetbyt

Some nits)

vsemozhetbyt · 2018-07-19T16:02:59Z

doc/guides/adding-new-napi-api.md

@@ -0,0 +1,31 @@
+# Contributing a new API to N-API
+
+N-API is Node's next generation ABI-stable API for native modules. While improving the API surface is encouraged and welcomed, the following are a set of principles and guidelines to keep in mind while adding a new N-API API.


Node's -> Node.js's?

vsemozhetbyt · 2018-07-19T16:07:02Z

doc/guides/adding-new-napi-api.md

+-	A new API **must** adhere to N-API API shape and spirit
+    - **Must** be a C API
+    - **Must** not throw exceptions
+    - **Must** return napi_status


`napi_status`

vsemozhetbyt · 2018-07-19T16:07:24Z

doc/guides/adding-new-napi-api.md

+    - **Must** be a C API
+    - **Must** not throw exceptions
+    - **Must** return napi_status
+    - **Should** consume napi_env


vsemozhetbyt · 2018-07-19T16:09:12Z

doc/guides/adding-new-napi-api.md

+    - **Should** consume napi_env
+    - **Must** operate only on primitive data types, pointers to primitive datatypes or opaque handles
+    - **Must** be a necessary API and not a nice to have. Convenience APIs belong in node-addon-api.
+    - **Must** not change the signature of an existing N-API API  or break ABI compatibility with other versions of Node.


Some points have a period in the end, some have not. Should we unify them?

vsemozhetbyt · 2018-07-19T16:13:43Z

doc/guides/adding-new-napi-api.md

+    - **Must** be a necessary API and not a nice to have. Convenience APIs belong in node-addon-api.
+    - **Must** not change the signature of an existing N-API API  or break ABI compatibility with other versions of Node.
+-	New API **should** be agnostic towards the underlying JavaScript VM
+-	New API request PRs **must** have a corresponding documentation update


Is request PR tautological (here and below)?

vsemozhetbyt · 2018-07-19T16:23:12Z

doc/guides/adding-new-napi-api.md

+    - **Should** consume napi_env
+    - **Must** operate only on primitive data types, pointers to primitive datatypes or opaque handles
+    - **Must** be a necessary API and not a nice to have. Convenience APIs belong in node-addon-api.
+    - **Must** not change the signature of an existing N-API API  or break ABI compatibility with other versions of Node.


Node -> Node.js

vsemozhetbyt · 2018-07-19T16:25:25Z

doc/guides/adding-new-napi-api.md

+-	A new API **should** be discussed at the N-API working group meeting
+-	A new API addition **must** be signed off by at least two members of the N-API WG
+-	A new API addition **should** be simultaneously implemented in at least one other VM implementation of Node.
+-	A new API **must** be considered experimental for at least one minor version release of Node before it can be considered for promotion out of experimental


Node -> Node.js

vsemozhetbyt · 2018-07-19T16:26:53Z

doc/guides/adding-new-napi-api.md

+-	A new API addition **should** be simultaneously implemented in at least one other VM implementation of Node.
+-	A new API **must** be considered experimental for at least one minor version release of Node before it can be considered for promotion out of experimental
+    - Experimental APIs **must** be documented as such
+    - Experimental APIs **must** require an explicit compile-time flag (#define) to be set to opt-in


vsemozhetbyt · 2018-07-19T16:29:04Z

doc/guides/adding-new-napi-api.md

+    - Experimental APIs **must** require an explicit compile-time flag (#define) to be set to opt-in
+    - Experimental APIs **must** be considered for backport
+    - Experimental status exit criteria **must** involve at least the following:
+        - A new PR **must** be opened in nodejs/node to remove experimental status. This PR **must** be tagged as **n-api** and **semver-minor**.


`nodejs/node`

vsemozhetbyt · 2018-07-19T16:34:41Z

doc/guides/adding-new-napi-api.md

+        - Exiting an API from experimental **must** be signed off by the working group.
+        - If a backport is merited, an API **must** have a down level implementation.
+        - The API **should** be used by a published real-world module. Use of the API by a real-world published module will contribute favorably to the decision to take an API out of experimental status
+        - The API **must** be implemented in a node implementation with an alternate VM


node -> Node.js

vsemozhetbyt · 2018-07-19T16:37:34Z

Doc linting also have some Use spaces instead of hard-tabs errors.

vsemozhetbyt · 2018-07-19T16:38:37Z

cc @nodejs/documentation, @Trott

mhdawson

LGTM once existing comments are addressed.

mhdawson · 2018-07-19T16:39:54Z

Also obviously all the linter errors need to be fixed up.

mhdawson · 2018-07-19T17:10:45Z

@nodejs/tsc please review as there are some key points that affect PRs to node core.

For example:

A new API addition **must** be signed off by at least two members of the N-API WG

digitalinfinity · 2018-07-19T18:31:46Z

Thanks @jasnell @vsemozhetbyt @mhdawson for your feedback- incorporated it all into a new commit

cjihrig

The text generally LGTM. The package-lock changes should be backed out?

cjihrig · 2018-07-19T18:40:36Z

doc/guides/adding-new-napi-api.md

+      datatypes or opaque handles.
+    * **Must** be a necessary API and not a nice to have. Convenience APIs
+      belong in node-addon-api.
+    * **Must** not change the signature of an existing N-API API  or break


Extra space before "or"

Fixed- thanks! And yes, whoops- the package-lock changes were accidental 😞

cjihrig · 2018-07-19T18:41:23Z

doc/guides/adding-new-napi-api.md

+      belong in node-addon-api.
+    * **Must** not change the signature of an existing N-API API  or break
+      ABI compatibility with other versions of Node.js.
+* New API **should** be agnostic towards the underlying JavaScript VM.


Is this a "should" or a "must"?

You really need to perform the exercise of stretching the bed sheet onto two different mattresses to see if it really is a one-size-fits-all 🙂

I guess, basically, if this is a "must", then so is implementation on top of multiple engines below.

Yeah, also the thinking here was that there might be legitimate reasons why we might want to expose an API that was VM specific (that other VMs would just return an error/silently fail on), and so the thinking was to leave that up to the reviewers of the relevant PR

vsemozhetbyt · 2018-07-19T18:51:49Z

doc/guides/adding-new-napi-api.md

+* There **must** be at least one test case showing how to use the API.
+* There **should** be at least one test case per interesting use of the API.
+* There **should** be a sample provided that operates in a realistic way
+    (operating how a real addon would be written).


A nit: wrapped lines in this level are not aligned with the first line properly (other levels seem OK).

Fixed- thanks!

vsemozhetbyt · 2018-07-19T22:28:27Z

CI-lite: https://ci.nodejs.org/job/node-test-pull-request-lite/919/

vsemozhetbyt · 2018-07-25T20:45:11Z

Landed in e83126a
Thank you!

This adds a new guide that outlines the principles and guidelines for contributing a new API to the N-API surface. These guidelines were formulated based on discussions in the API working group. PR-URL: #21877 Refs: nodejs/abi-stable-node#301 Reviewed-By: James M Snell <[email protected]> Reviewed-By: Michael Dawson <[email protected]> Reviewed-By: Colin Ihrig <[email protected]> Reviewed-By: Vse Mozhet Byt <[email protected]>

Trott · 2018-08-07T23:32:12Z

Quite a bit late on this one, so I apologize, but:

It is not clear if this document is supposed to be policy. It describes itself as a list of "principles and guidelines to keep in mind". I don't recall if I started reviewing this or not, but had I, I would have stopped right there and let other folks review it because what do I have to say about n-api principles and guidelines? Happy to let n-api-savvy folks do that.

But then it gets to using must and should, which sure sounds like it's intended to be binding.

Given i's self-description as "guidelines" etc. along with its lack of discoverability (no link from COLLABORATOR_GUIDE or any other location in the repo, so the only way to find it is if you already know it's there), I'm not inclined to interpret this as policy but more guidance. (It probably doesn't help that I believe we have far too much policy for anyone to actually know and follow.)

If it is intended to be a policy, I think that needs to be clarified and it probably should get considerably more TSC scrutiny.

mhdawson · 2018-08-08T22:25:56Z

It was intended to be policy, it does need more discover-ability by inclusion as a reference in the onboarding guide at least for a start. I had included a @nodejs/tsc ping specifically because I wanted to get people to chime in -> see #21877 (comment)

mhdawson · 2018-08-08T22:26:49Z

Suggestions on other places to link to for discover-ability also greatly appreciated.

Trott · 2018-08-09T05:35:20Z

Suggestions on other places to link to for discover-ability also greatly appreciated.

I'd go with COLLABORATOR_GUIDE rather than onboarding. Onboarding is something most Collaborators should ideally only have to look at once. Anything they'll need repeatedly should be in COLLABORATOR_GUIDE. (Yes, there's still stuff in onboarding that needs to be moved to COLLABORATOR_GUIDE. But for most things a Collaborator is likely to need again, onboarding links to COLLABORATOR_GUIDE or the material is duplicated there. I'm not a fan of the duplication either, though.)

mhdawson · 2018-08-09T15:25:22Z

@Trott I'll create a PR adding a link in the COLLABORATOR_GUIDE.

mhdawson · 2018-08-29T20:24:47Z

PR to add to collaborator guide #22593

doc: add guide for updating N-API API surface

f7aec90

This adds a new guide that outlines the principles and guidelines for contributing a new API to the N-API surface. These guidelines were formulated based on discussions in the API working group. Refs: nodejs/abi-stable-node#301

jasnell reviewed Jul 19, 2018

View reviewed changes

jasnell approved these changes Jul 19, 2018

View reviewed changes

vsemozhetbyt added doc Issues and PRs related to the documentations. node-api Issues and PRs related to the Node-API. labels Jul 19, 2018

vsemozhetbyt reviewed Jul 19, 2018

View reviewed changes

mhdawson approved these changes Jul 19, 2018

View reviewed changes

mhdawson added the tsc-review label Jul 19, 2018

Incorporate review feedback

1f32f7c

cjihrig approved these changes Jul 19, 2018

View reviewed changes

vsemozhetbyt reviewed Jul 19, 2018

View reviewed changes

Undo changes to package-lock, address more feedback

ee906cf

vsemozhetbyt approved these changes Jul 19, 2018

View reviewed changes

vsemozhetbyt closed this Jul 25, 2018

targos mentioned this pull request Jul 31, 2018

v10.8.0 proposal #22040

Merged

This was referenced Aug 2, 2018

2018-08-01 Version 10.8.0 (Current) @targos beevelop/docker-android-nodejs#226

Closed

2018-08-01 Version 10.8.0 (Current) @targos beevelop/docker-android-nodejs#227

Closed

Trott mentioned this pull request Aug 7, 2018

doc: add req for sec wg review in some cases #21896

Closed

3 tasks

		@@ -0,0 +1,31 @@
		# Contributing a new API to N-API

		N-API is Node's next generation ABI-stable API for native modules. While improving the API surface is encouraged and welcomed, the following are a set of principles and guidelines to keep in mind while adding a new N-API API.


		N-API is Node's next generation ABI-stable API for native modules. While improving the API surface is encouraged and welcomed, the following are a set of principles and guidelines to keep in mind while adding a new N-API API.

		- A new API must adhere to N-API API shape and spirit

doc: add guide for updating N-API API surface #21877

doc: add guide for updating N-API API surface #21877

Conversation

digitalinfinity commented Jul 19, 2018

Checklist

digitalinfinity commented Jul 19, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jasnell left a comment

Choose a reason for hiding this comment

vsemozhetbyt left a 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

vsemozhetbyt commented Jul 19, 2018

vsemozhetbyt commented Jul 19, 2018

mhdawson left a comment

Choose a reason for hiding this comment

mhdawson commented Jul 19, 2018

mhdawson commented Jul 19, 2018

digitalinfinity commented Jul 19, 2018

cjihrig left a 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

vsemozhetbyt commented Jul 19, 2018

vsemozhetbyt commented Jul 25, 2018

Trott commented Aug 7, 2018

mhdawson commented Aug 8, 2018 • edited Loading

mhdawson commented Aug 8, 2018

Trott commented Aug 9, 2018

mhdawson commented Aug 9, 2018

mhdawson commented Aug 29, 2018

mhdawson commented Aug 8, 2018 •

edited

Loading