n-api: remove n-api module loading flag by gabrielschulhof · Pull Request #14902 · nodejs/node · GitHub

Remove the command line flag that was needed for N-API module loading.

Re: nodejs/vm#9

Checklist

make -j4 test (UNIX), or vcbuild test (Windows) passes
tests and/or benchmarks are included
documentation is changed or added
commit message follows commit guidelines

Affected core subsystem(s)

n-api

FWIW

Isn't this semver-minor?

I have a couple questions. If they prove to be nothing, then LGTM.

The [] can be dropped too I think.

If you remove this line, where is the value set to -1?

Do any of our tests actually test for the warning?

Maybe leave this in as a no-op for now?

I think that would be a good idea. It will give us time to update docs etc and will avoid breaking people who are already using it.

+1 to leaving it for now.

FYI @nodejs/ctc, @Fishrock123 to make people who were involved in earlier discussions comment/object in this PR if they still have issues with the approach.

Nitpick: Do we want this warning to be printed on every n-api module load or just the first one? Complicates the code slightly but might be a better experience if n-api becomes more common?

typically we emit warnings only on the first use.

I would prefer this list to be smashed before this getting in a release.

LGTM

@mhdawson @digitalinfinity @addaleax @mcollina @cjihrig @refack I have now addressed the review comments.

I'm ok with the changes but marking as "Request changes" so that we avoid landing until we have addressed this comment from @mcollina

I would prefer [this list](https://github.com/nodejs/abi-stable-node/issues/271) to be smashed before this getting in a release.

I think its reasonable that we either address or make the case why we don't need to in advance.

We plan to discuss the one related to CS modules with Bradley in our next regular N-API meeting (Thursday). @aruneshchandra since I think @jasongin is away can you chase down the status on the changes related to async hooks ?

@mhdawson the n-api team chatted with @RReverser and @bmeck last Thursday about ES6 module supprt- the discussion is captured at nodejs/abi-stable-node#256 (comment). TLDR is that n-api will not attempt to provide first class support for any module loader, but we will be making a breaking change (@BoingBoing has already opened #15088)

Discussed in latest TSC/CTC meeting there did not seem to be objections to removing once we get through the list of breaking changes. Was left that if any TSC/CTC members object they should come and discuss in this PR.

@mhdawson it seems like there are no objections anymore besides yours. I think think could land if you are good with it?

@BridgeAR we are waiting until we have all of the remaining breaking changes in (we are close). We want those to land first as a group and then have this land as a second step.

@gabrielschulhof is in the weekly meeting were we are discussing, so is in the loop as to the next steps.

Once we are ready I'll go ahead and land this one.

Looks like we are close to landing the remaining breaking changes, CI run:

https://ci.nodejs.org/job/node-test-pull-request/10081/

Now just waiting on #15108
@sampsongao will add the tests tonight and hopefully we can land Friday Sep 15
And then I'll follow that with this one.

LGTM, just about to land last breaking change so this one can go in as well.

@gabrielschulhof the tests no longer pass because of the changes to the Init signature. I'm done for today. Can you take a look and I'll try to land tomorrow.

Remove the command line flag that was needed for N-API module loading. Re: nodejs/vm#9

https://ci.nodejs.org/job/node-test-pull-request/10141/

Weird linter failure, so I stopped that and started

https://ci.nodejs.org/job/node-test-pull-request/10142/

instead.

error: The last gc run reported the following. Please correct the root cause
and remove .git/gc.log.
Automatic cleanup will not be performed until the file is removed.

warning: There are too many unreachable loose objects; run 'git prune' to remove them.

I'll leave it running because make lint passes locally.

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>

* **crypto** * Support for multiple ECDH curves. [#15206](#15206) * **dgram** * Added `setMulticastInterface()` API. [#7855](#7855) * **n-api** * The command-line flag is no longer required to use N-API. [#14902](#14902) * **tls** * Docs-only deprecation of `parseCertString()`. [#14245](#14245) * **New Contributors** * Welcome Sebastiaan Deckers (@sebdeckers) as a new Collaborator! [#15354](#15354)

* **crypto** * Support for multiple ECDH curves. [#15206](#15206) * **dgram** * Added `setMulticastInterface()` API. [#7855](#7855) * Custom lookup functions are now supported. [#14560](#14560) * **n-api** * The command-line flag is no longer required to use N-API. [#14902](#14902) * **tls** * Docs-only deprecation of `parseCertString()`. [#14245](#14245) * **New Contributors** * Welcome Sebastiaan Deckers (@sebdeckers) as a new Collaborator! [#15354](#15354)

* **crypto** * Support for multiple ECDH curves. [#15206](nodejs/node#15206) * **dgram** * Added `setMulticastInterface()` API. [#7855](nodejs/node#7855) * Custom lookup functions are now supported. [#14560](nodejs/node#14560) * **n-api** * The command-line flag is no longer required to use N-API. [#14902](nodejs/node#14902) * **tls** * Docs-only deprecation of `parseCertString()`. [#14245](nodejs/node#14245) * **New Contributors** * Welcome Sebastiaan Deckers (@sebdeckers) as a new Collaborator! [#15354](nodejs/node#15354)

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>

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>

nodejs-github-bot added c++ Issues and PRs that require attention from people who are familiar with C++. node-api Issues and PRs related to the Node-API. labels Aug 17, 2017

refack approved these changes Aug 17, 2017

View reviewed changes

cjihrig approved these changes Aug 17, 2017

View reviewed changes

addaleax approved these changes Aug 17, 2017

View reviewed changes

digitalinfinity approved these changes Aug 18, 2017

View reviewed changes

mcollina approved these changes Aug 18, 2017

View reviewed changes

gabrielschulhof force-pushed the remove-flag branch from 513fe34 to e6ad4b5 Compare August 18, 2017 10:49

gabrielschulhof force-pushed the remove-flag branch from e6ad4b5 to 9b78363 Compare August 18, 2017 10:53

mhdawson suggested changes Aug 18, 2017

View reviewed changes

mhdawson mentioned this pull request Aug 30, 2017

Node.js Foundation Core Technical Committee (CTC) Meeting 2017-08-30 nodejs/CTC#171

Closed

mhdawson mentioned this pull request Aug 30, 2017

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

Closed

jasnell approved these changes Aug 30, 2017

View reviewed changes

mhdawson approved these changes Sep 18, 2017

View reviewed changes

n-api: remove n-api module loading flag

ca5d54f

Remove the command line flag that was needed for N-API module loading. Re: nodejs/vm#9

gabrielschulhof force-pushed the remove-flag branch from 9b78363 to ca5d54f Compare September 19, 2017 20:32

gabrielschulhof deleted the remove-flag branch September 25, 2017 09:20

MylesBorins added the dont-land-on-v6.x label Oct 17, 2017

richardlau mentioned this pull request Apr 16, 2018

documentation of --napi-modules #20082

Closed

MylesBorins mentioned this pull request Apr 16, 2018

v6.14.2 proposal #19996

Merged

bnoordhuis mentioned this pull request Apr 18, 2018

src: kickstart addon by calling its constructor #20114

Closed

4 tasks

richardlau mentioned this pull request Apr 25, 2018

n-api: update cli documentation #20301

Closed

3 tasks

n-api: remove n-api module loading flag by gabrielschulhof · Pull Request #14902 · nodejs/node · GitHub

Uh oh!

Uh oh!

n-api: remove n-api module loading flag - #14902

n-api: remove n-api module loading flag#14902gabrielschulhof wants to merge 1 commit intonodejs:masternodejs/node:masterfrom gabrielschulhof:remove-flaggabrielschulhof/node:remove-flagCopy head branch name to clipboard

Conversation

gabrielschulhof commented Aug 17, 2017

Checklist

Affected core subsystem(s)

Uh oh!

refack left a comment

Choose a reason for hiding this comment

Uh oh!

refack commented Aug 17, 2017

Uh oh!

cjihrig left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

mhdawson commented Aug 17, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

mcollina left a comment

Choose a reason for hiding this comment

Uh oh!

gabrielschulhof commented Aug 18, 2017

Uh oh!

mhdawson left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

digitalinfinity commented Aug 30, 2017

Uh oh!

mhdawson commented Aug 30, 2017

Uh oh!

BridgeAR commented Sep 13, 2017

Uh oh!

mhdawson commented Sep 13, 2017

Uh oh!

mhdawson commented Sep 14, 2017

Uh oh!

mhdawson commented Sep 14, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

mhdawson left a comment

Choose a reason for hiding this comment

Uh oh!

mhdawson commented Sep 18, 2017

Uh oh!

gabrielschulhof commented Sep 19, 2017

Uh oh!

gabrielschulhof commented Sep 19, 2017

Uh oh!

gabrielschulhof commented Sep 19, 2017

Uh oh!

gabrielschulhof commented Sep 19, 2017

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

11 participants

n-api: remove n-api module loading flag#14902
gabrielschulhof wants to merge 1 commit into
nodejs:masterfrom
gabrielschulhof:remove-flag

mhdawson left a comment •
edited
Loading

mhdawson commented Sep 14, 2017 •
edited
Loading