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

Bring N-API out of experimental #501

Closed
mhdawson opened this issue Mar 5, 2018 · 16 comments
Closed

Bring N-API out of experimental #501

mhdawson opened this issue Mar 5, 2018 · 16 comments

Comments

@mhdawson
Copy link
Member

mhdawson commented Mar 5, 2018

The criteria for bringing N-API out of experimental were discussed in the VM Summit last July and captured here: nodejs/abi-stable-node#284.

The items where:

  • Multiple implementations of n-api on at least 2 VMs
    • Was already in place as N-API is implemented on V8 and Chakra core
  • Define N-API / async_hooks dependency
  • At least two modules used in real world use cases on at least one VM
  • At least two modules that are NPM installable with pre-builds and all unit tests passing
    • We have worked through the process of how N-API will integrate with node-pre-gyp and
      have a pull request here to integrate the required changed #345

    • We did not have to make any changes to the APIs to support working with node-pre-gyp and
      don't believe any future changes will be required either. As such the N-API team does not believe
      waiting until the PR lands is necessary to move N-API out of experimental.

We have not had to make any breaking changes to the APIs since the last ones which were originally planned for Node 8.6.x back in September 2017.

Along the way we have had comments from a number of modules owners that they will be more interested in moving to N-API once it comes out of experimental.

At this point in time, the N-API team believes it has met the substance of the criteria originally set out and the recommendation of the N-API team is to move it out of experimental as we believe that we will not get a material amount of additional validation while it remains in experimental. This is because modules owners are reluctant to invest while it is in that state.

@cjihrig
Copy link
Contributor

cjihrig commented Mar 5, 2018

+1. Thanks for all of the hard work on N-API.

@jasnell
Copy link
Member

jasnell commented Mar 5, 2018

+1

@ofrobots
Copy link
Contributor

ofrobots commented Mar 5, 2018

I support this.

@mcollina
Copy link
Member

mcollina commented Mar 5, 2018

+1

2 similar comments
@rvagg
Copy link
Member

rvagg commented Mar 6, 2018

+1

@fhinkel
Copy link
Member

fhinkel commented Mar 6, 2018

+1

@fhinkel
Copy link
Member

fhinkel commented Mar 6, 2018

If there's no objections, moving N-API out of experimental doesn't need a TSC vote, right?

@Trott
Copy link
Member

Trott commented Mar 6, 2018

If there's no objections, moving N-API out of experimental doesn't need a TSC vote, right?

@fhinkel Looking at our documented process in GOVERNANCE.md, we can skip a vote if we:

  • @-mention @nodejs/tsc (<-- oh, look at that, I just did it)
  • wait 72 hours
  • get two or more LGTMs (I think the +1s count) and no objections

I think this should be removed from the meeting agenda, although by our rules, only @mhdawson can do that because they put it on the agenda in the first place. Michael or someone else can mention it during announcements if they want to raise awareness at the meeting.

But it sure looks like this is going to sit here for 72 hours (because I think the clock starts when @nodejs/tsc is @-mentioned), and then be affirmed without a vote.

@fhinkel
Copy link
Member

fhinkel commented Mar 6, 2018

@Trott thanks for explaining. Definitely didn't want to rush it, just making sure we don't skip a vote if needed.

@Trott
Copy link
Member

Trott commented Mar 6, 2018

@Trott thanks for explaining. Definitely didn't want to rush it, just making sure we don't skip a vote if needed.

71 hours and 35 minutes to go! 😄

@mhdawson
Copy link
Member Author

mhdawson commented Mar 6, 2018

I'm happy to remove from the agenda. I just wanted to make sure we had some visibility/comments and I think that is occurring.

@mcollina
Copy link
Member

mcollina commented Mar 8, 2018

As part of this process we should make sure that the real-world usage is captured in citgm. It's important we verify that we are not regressing things.
@mhdawson can you add a couple of those modules?

@mhdawson
Copy link
Member Author

mhdawson commented Mar 8, 2018

Testing for N-API itself is part of the regular regression runs.

For modules we should be testing both:

  1. node-addon-api, which do are testing nightly across plain https://ci.nodejs.org/view/x%20-%20Abi%20stable%20module%20API/job/node-test-node-addon-api/

  2. Modules. We have this job running:

https://ci.nodejs.org/job/node-test-napi-modules-citgm/

(just recently yellow, investigating), but it does not have all of the modules covered.

So its a good idea to try to add the modules listed above to the regular CiTGM. I've opened an issue in the abi-stable-node repo to pursue that. -> nodejs/node-addon-api#234

nodejs/node-addon-api#234

@mcollina
Copy link
Member

mcollina commented Mar 8, 2018

Side note, we should put "add at least an ecosystem module that use X feature when it exists experimental to citgm".

@mhdawson
Copy link
Member Author

mhdawson commented Mar 9, 2018

PR to remove experimental status nodejs/node#19262

mhdawson added a commit to mhdawson/io.js that referenced this issue Mar 14, 2018
Take n-api out of experimental as per:
nodejs/TSC#501
mhdawson added a commit to nodejs/node that referenced this issue Mar 14, 2018
Take n-api out of experimental as per:
nodejs/TSC#501

PR-URL: #19262
Reviewed-By: Matteo Collina <matteo.collina@gmail.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Michaël Zasso <targos@protonmail.com>
Reviewed-By: Gus Caplan <me@gus.host>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Franziska Hinkelmann <franziska.hinkelmann@gmail.com>
@mhdawson
Copy link
Member Author

PR landed !

gabrielschulhof pushed a commit to gabrielschulhof/node that referenced this issue Apr 16, 2018
Take n-api out of experimental as per:
nodejs/TSC#501

PR-URL: nodejs#19262
Reviewed-By: Matteo Collina <matteo.collina@gmail.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Michaël Zasso <targos@protonmail.com>
Reviewed-By: Gus Caplan <me@gus.host>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Franziska Hinkelmann <franziska.hinkelmann@gmail.com>
MylesBorins pushed a commit to nodejs/node that referenced this issue Apr 16, 2018
Take n-api out of experimental as per:
nodejs/TSC#501

Backport-PR-URL: #19447
PR-URL: #19262
Reviewed-By: Matteo Collina <matteo.collina@gmail.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Michaël Zasso <targos@protonmail.com>
Reviewed-By: Gus Caplan <me@gus.host>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Franziska Hinkelmann <franziska.hinkelmann@gmail.com>
mhdawson added a commit to nodejs/node that referenced this issue Jun 7, 2018
Take n-api out of experimental as per:
nodejs/TSC#501

Backport-PR-URL: #21083
PR-URL: #19262
Reviewed-By: Matteo Collina <matteo.collina@gmail.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Michaël Zasso <targos@protonmail.com>
Reviewed-By: Gus Caplan <me@gus.host>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Franziska Hinkelmann <franziska.hinkelmann@gmail.com>
MylesBorins pushed a commit to nodejs/node that referenced this issue Jun 14, 2018
Take n-api out of experimental as per:
nodejs/TSC#501

Backport-PR-URL: #21083
PR-URL: #19262
Reviewed-By: Matteo Collina <matteo.collina@gmail.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Michaël Zasso <targos@protonmail.com>
Reviewed-By: Gus Caplan <me@gus.host>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Franziska Hinkelmann <franziska.hinkelmann@gmail.com>
rvagg pushed a commit to nodejs/node that referenced this issue Aug 16, 2018
Take n-api out of experimental as per:
nodejs/TSC#501

Backport-PR-URL: #21083
PR-URL: #19262
Reviewed-By: Matteo Collina <matteo.collina@gmail.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Michaël Zasso <targos@protonmail.com>
Reviewed-By: Gus Caplan <me@gus.host>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Franziska Hinkelmann <franziska.hinkelmann@gmail.com>
abhishekumar-tyagi pushed a commit to abhishekumar-tyagi/node that referenced this issue May 5, 2024
Take n-api out of experimental as per:
nodejs/TSC#501

PR-URL: nodejs/node#19262
Reviewed-By: Matteo Collina <matteo.collina@gmail.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Michaël Zasso <targos@protonmail.com>
Reviewed-By: Gus Caplan <me@gus.host>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Franziska Hinkelmann <franziska.hinkelmann@gmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

8 participants