Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Document guidance on adding APIs to N-API #301

Closed
mhdawson opened this issue Apr 26, 2018 · 4 comments
Closed

Document guidance on adding APIs to N-API #301

mhdawson opened this issue Apr 26, 2018 · 4 comments
Assignees
Milestone

Comments

@mhdawson
Copy link
Member

@digitalinfinity has a start, should add here.

@digitalinfinity
Copy link
Contributor

digitalinfinity commented Jun 12, 2018

The following are a set of proposed principles to add an API to the N-API surface. Text in italics are my comments/annotations. Feedback/thoughts welcome- I'm throwing this out there to start the discussion. Thanks to @kfarnung for his early feedback.

N-API design principles

  • A new API must adhere to N-API API shape and spirit (spirit is kind of what @mhdawson talks about here)
    • 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.
  • New API should be agnostic towards the underlying JavaScript VM
  • New API request PRs must have a corresponding documentation update
  • New API request 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)
  • 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
    • 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 implementation with an alternate VM (Node-ChakraCore can provide SLAs here e.g we’ll review PRs within a week, attempt best effort to implement within a week or sign off within that time)

@mhdawson
Copy link
Member Author

mhdawson commented Jun 13, 2018

A great start !

I'd propose

A new API addition must be signed off by at least one member of the N-API WG

be 2 members.

I think this one

The API must be used by a published real-world module.

might set the bar a bit too high. I think something like

Use of the API by a published real-world module will contribute significantly to the decision to take an API out of the experimental status

For

Experimental APIs must require an explicit flag to be set to opt-in

to date we've been discussing the use of a 'Define' as opposed to a flag. That means that module owners need to opt in, but that end users running node would not have to if they use a module that uses the API and opted in.

@digitalinfinity
Copy link
Contributor

Great suggestions @mhdawson - updated to incorporate your feedback

digitalinfinity added a commit to digitalinfinity/node that referenced this issue Jul 19, 2018
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
vsemozhetbyt pushed a commit to nodejs/node that referenced this issue Jul 25, 2018
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]>
targos pushed a commit to nodejs/node that referenced this issue Jul 26, 2018
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]>
@mhdawson
Copy link
Member Author

mhdawson commented Aug 2, 2018

Landed, closing

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

2 participants