Skip to content
This repository has been archived by the owner on Oct 7, 2020. It is now read-only.

drop --napi-modules earlier than dropping the "experimental" status #9

Closed
mcollina opened this issue Aug 1, 2017 · 15 comments
Closed

Comments

@mcollina
Copy link
Member

mcollina commented Aug 1, 2017

In order to prove the validity, I propose we should just drop the --napi-modules flag and keep the experimental status.

I'm not sure we need @nodejs/ctc approval for this, but I'll add it to the agenda anyway.

@addaleax
Copy link
Member

addaleax commented Aug 1, 2017

+1, and I don’t think there are any rules saying we need CTC approval. A regular PR that gets approved without significant opposition should be fine.

@cjihrig
Copy link

cjihrig commented Aug 13, 2017

I'm +1 to this. The only issue is that it makes it a bit trickier to emit the experimental feature warning which is currently gated by the flag.

@evanlucas
Copy link

+1 from me

@evanlucas
Copy link

Could we not add the experimental warning in napi_module_register()?

@addaleax
Copy link
Member

not sure why this repository was chosen to open the issue, but we should probably ping @nodejs/n-api since it’s not one of the ones usually used for N-API discussions

@cjihrig
Copy link

cjihrig commented Aug 14, 2017

@evanlucas yes if napi_module_register() has access to env, needed for ProcessEmitWarning().

I agree that this repo is an odd choice for this issue. I think it should be moved to the core repo because a lot of people might have opinions about this.

@Fishrock123
Copy link

I'm -1, as far as anyone can tell landing an unflagged feature means we are stuck with it for a very significant amount of time. How would this prove validity?

@mhdawson
Copy link
Member

I'd agree we should move this to the core node repo.

@mcollina
Copy link
Member Author

@mhdawson can you please articulate why we need this? You did it way better than what I could do at the summit.

@aruneshchandra
Copy link

We agreed on dropping --napi-modules during VM Summit to reduce friction for people wanting to experiment with N-API in their existing apps without having to modify their existing deployment workflow.

@rvagg
Copy link
Member

rvagg commented Aug 17, 2017

as far as anyone can tell landing an unflagged feature means we are stuck with it for a very significant amount of time

"experimental" is our get-out-of-jail card here, we can do what we want with it at any time, caveat emptor. The flag is just an additional step above labelling it experimental.

My memory of the VM Summit was that those involved in developing N-API were pretty strong on their desire to unflag this in order to get broad enough deployment to make the next phase of testing practical, otherwise they are stuck in a backwater twiddling their fingers (paraphrasing here!). As @mcollina said though, it'd be good to have a clearer explanation of the justification that was formed at that meeting in response to the push-back. @Fishrock123 has concerns and he may be persuaded by the same arguments that I was.

I'm +1 on unflagging

@mcollina
Copy link
Member Author

Some more points:

a. If we are going to do this, we should try very hard not break the ABI compat. It's part of the testing, can we provide an ABI-stable node?
b. The full toolchain needs help, it is lacking on both on the documentation and testing sides: it will be the primary point were developers should go. No one of the existing NAN user will use the C API directly, and they will (and should) move to https://github.com/nodejs/node-addon-api.
c. If we ship N-API without a flag before LTS, in 1 full lts cycle we will reach a point where we should not recompile things anymore, as 2 LTS versions will have N-API, and Node 4 and 6 will have no-recompilation (Linux and Mac) as well because of nodejs/node-addon-api#103.

We can argue that we should lift the "experimental" warning. To do so this feature needs users, and people that will start porting modules. N-API has a chicken and egg problem, it needs validation, however no one will put in the effort to migrate unless we graduate it to something "a bit more stable" sooner later than later.

Removing the flag but keeping the warning is an intermediate step that we could do right now.

@mhdawson
Copy link
Member

The key issue is that until N-API is "real" it is hard for module owners (who we all know are busy) to justify the time doing a port. Until its "real" we'll probably be limited to a smallish set of module owners who we can convince to do a port for the benefit of the overall ecosystem

The challenge is to balance getting enough feedback/use of the new API to be comfortable that it is ready to become non-experimental without stalling the overall effort by waiting too long.

The discussion at the vm summit was that removing the flag was a step towards making it easier to consume (and a bit more "real"). Leaving in the warning (which would be emitted only if the user uses an N-API module) means consumers are still warned that they are using an experimental feature, without having to modify their deployment infrastructure to add an additional flag which can sometimes be difficult.

The goal is still to get it to be non-experimental "soon" (ideally before 8 goes LTS), but removing the command line option is a concrete step we can do now.

@MylesBorins
Copy link

MylesBorins commented Aug 17, 2017 via email

@mhdawson
Copy link
Member

Was discussed in this weeks CTC/TSC meeting please see this comment: nodejs/node#14902 (comment). Closing as I think any further discussion can be handle PR #14902.

@Trott Trott removed the ctc-agenda label Aug 31, 2017
gabrielschulhof pushed a commit to gabrielschulhof/node that referenced this issue Sep 19, 2017
Remove the command line flag that was needed for N-API module loading.

Re: nodejs/vm#9
mhdawson pushed a commit to nodejs/node that referenced this issue Sep 19, 2017
Remove the command line flag that was needed for N-API module loading.
Re: nodejs/vm#9

PR-URL: #14902
Reviewed-By: Refael Ackermann <refack@gmail.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Matteo Collina <matteo.collina@gmail.com>
Reviewed-By: Michael Dawson <michael_dawson@ca.ibm.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Hitesh Kanwathirtha <digitalinfinity@gmail.com>
jasnell pushed a commit to nodejs/node that referenced this issue Sep 20, 2017
Remove the command line flag that was needed for N-API module loading.
Re: nodejs/vm#9

PR-URL: #14902
Reviewed-By: Refael Ackermann <refack@gmail.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Matteo Collina <matteo.collina@gmail.com>
Reviewed-By: Michael Dawson <michael_dawson@ca.ibm.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Hitesh Kanwathirtha <digitalinfinity@gmail.com>
Qard pushed a commit to Qard/ayo that referenced this issue Sep 21, 2017
Remove the command line flag that was needed for N-API module loading.
Re: nodejs/vm#9

PR-URL: nodejs/node#14902
Reviewed-By: Refael Ackermann <refack@gmail.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Matteo Collina <matteo.collina@gmail.com>
Reviewed-By: Michael Dawson <michael_dawson@ca.ibm.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Hitesh Kanwathirtha <digitalinfinity@gmail.com>
Qard pushed a commit to Qard/ayo that referenced this issue Sep 21, 2017
Remove the command line flag that was needed for N-API module loading.
Re: nodejs/vm#9

PR-URL: nodejs/node#14902
Reviewed-By: Refael Ackermann <refack@gmail.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Matteo Collina <matteo.collina@gmail.com>
Reviewed-By: Michael Dawson <michael_dawson@ca.ibm.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Hitesh Kanwathirtha <digitalinfinity@gmail.com>
gabrielschulhof pushed a commit to gabrielschulhof/node that referenced this issue Apr 16, 2018
Remove the command line flag that was needed for N-API module loading.
Re: nodejs/vm#9

PR-URL: nodejs#14902
Reviewed-By: Refael Ackermann <refack@gmail.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Matteo Collina <matteo.collina@gmail.com>
Reviewed-By: Michael Dawson <michael_dawson@ca.ibm.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Hitesh Kanwathirtha <digitalinfinity@gmail.com>
MylesBorins pushed a commit to nodejs/node that referenced this issue Apr 16, 2018
Remove the command line flag that was needed for N-API module loading.
Re: nodejs/vm#9

Backport-PR-URL: #19447
PR-URL: #14902
Reviewed-By: Refael Ackermann <refack@gmail.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Matteo Collina <matteo.collina@gmail.com>
Reviewed-By: Michael Dawson <michael_dawson@ca.ibm.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Hitesh Kanwathirtha <digitalinfinity@gmail.com>
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

No branches or pull requests

10 participants