doc: add guide for updating N-API API surface #21877
There are no files selected for viewing
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -1,31 +1,50 @@ | ||
| # Contributing a new API to N-API | ||
|
|
||
| N-API is Node.js'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. | ||
| * **Must** be a C API. | ||
| * **Must** not throw exceptions. | ||
| * **Must** return `napi_status`. | ||
| * **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.js. | ||
| * New API **should** be agnostic towards the underlying JavaScript VM. | ||
|
There was a problem hiding this comment. Is this a "should" or a "must"? There was a problem hiding this comment. 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. There was a problem hiding this comment. 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 |
||
| * New API PRs **must** have a corresponding documentation update. | ||
| * New API PRs **must** be tagged as **n-api**. | ||
| * 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). | ||
|
There was a problem hiding this comment. A nit: wrapped lines in this level are not aligned with the first line properly (other levels seem OK). There was a problem hiding this comment. Fixed- thanks! |
||
| * 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.js. | ||
| * A new API **must** be considered experimental for at least one minor | ||
| version release of Node.js 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. | ||
| * 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**. | ||
| * 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.js implementation with an | ||
| alternate VM. | ||
Oops, something went wrong.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Extra space before "or"
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Fixed- thanks! And yes, whoops- the package-lock changes were accidental 😞