GV

GeezerVison

The World Wide Web, restored to 800 pixels of glory.

Home|About|Gaming cabinet|Archives|How it works|Starter sites

On this site

  • Sign in
  • ActionsCI/CD and automation
  • CodespacesCloud development environments
  • All Features
  • Documentation
  • Training
  • Certifications
  • Build with Copilot
  • Customize Copilot
  • Integrations
  • Trending repos
  • GitHub Sponsors
  • Developer articles
  • Enterprises

This view

  • Original page
  • New address

GeezerVison is showing github.com — n-api: remove n-api module loading flag by gabrielschulhof · Pull Request #14902 · nodejs/node

Conversation

@gabrielschulhof

@gabrielschulhof gabrielschulhof commented Aug 17, 2017

Copy link
Copy Markdown
Contributor

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

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions
@nodejs-github-bot 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 Node-API. labels Aug 17, 2017
refack
refack approved these changes Aug 17, 2017
View reviewed changes

@refack refack left a comment

Copy link
Copy Markdown
Contributor

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.

FWIW

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions
@refack

refack commented Aug 17, 2017

Copy link
Copy Markdown
Contributor

Isn't this semver-minor?
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
cjihrig
cjihrig approved these changes Aug 17, 2017
View reviewed changes

@cjihrig cjihrig left a comment

Copy link
Copy Markdown
Contributor

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.

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

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions
Comment thread test/addons-napi/testcfg.py Outdated

@cjihrig cjihrig Aug 17, 2017

Copy link
Copy Markdown
Contributor

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.

The [] can be dropped too I think.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions
Comment thread src/node_api.cc Outdated

@cjihrig cjihrig Aug 17, 2017

Copy link
Copy Markdown
Contributor

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.

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

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions
Comment thread src/node.cc Outdated

@cjihrig cjihrig Aug 17, 2017

Copy link
Copy Markdown
Contributor

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.

Do any of our tests actually test for the warning?

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions
addaleax
addaleax approved these changes Aug 17, 2017
View reviewed changes
Comment thread src/node.cc Outdated

@addaleax addaleax Aug 17, 2017

Copy link
Copy Markdown
Member

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.

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

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions

@mhdawson mhdawson Aug 17, 2017

Copy link
Copy Markdown
Member

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.

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.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions

@jasnell jasnell Aug 18, 2017

Copy link
Copy Markdown
Member

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.

+1 to leaving it for now.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions
@mhdawson

mhdawson commented Aug 17, 2017

Copy link
Copy Markdown
Member

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.
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
digitalinfinity
digitalinfinity approved these changes Aug 18, 2017
View reviewed changes
Comment thread src/node.cc Outdated

@digitalinfinity digitalinfinity Aug 18, 2017

Copy link
Copy Markdown
Contributor

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.

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?

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions

@jasnell jasnell Aug 18, 2017

Copy link
Copy Markdown
Member

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.

typically we emit warnings only on the first use.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions
mcollina
mcollina approved these changes Aug 18, 2017
View reviewed changes

@mcollina mcollina left a comment

Copy link
Copy Markdown
Member

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.

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

LGTM

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions
@gabrielschulhof
gabrielschulhof force-pushed the remove-flag branch from 513fe34 to e6ad4b5 Compare August 18, 2017 10:49
@gabrielschulhof

gabrielschulhof commented Aug 18, 2017

Copy link
Copy Markdown
Contributor Author

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

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@gabrielschulhof
gabrielschulhof force-pushed the remove-flag branch from e6ad4b5 to 9b78363 Compare August 18, 2017 10:53
mhdawson
mhdawson suggested changes Aug 18, 2017
View reviewed changes

@mhdawson mhdawson left a comment •
edited
Loading

Uh oh!

There was an error while loading. Please reload this page.

Copy link
Copy Markdown
Member

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.

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 ?

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions
@mhdawson mhdawson mentioned this pull request Aug 30, 2017
Node.js Foundation Core Technical Committee (CTC) Meeting 2017-08-30 nodejs/CTC#171
Closed
@digitalinfinity

digitalinfinity commented Aug 30, 2017

Copy link
Copy Markdown
Contributor

@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)
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@mhdawson

mhdawson commented Aug 30, 2017

Copy link
Copy Markdown
Member

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.
aruneshchandra and matthargett reacted with hooray emoji
All reactions
  • 🎉 2 reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@mhdawson mhdawson mentioned this pull request Aug 30, 2017
drop --napi-modules earlier than dropping the "experimental" status nodejs/vm#9
Closed
jasnell
jasnell approved these changes Aug 30, 2017
View reviewed changes
@BridgeAR

BridgeAR commented Sep 13, 2017

Copy link
Copy Markdown
Member

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

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@mhdawson

mhdawson commented Sep 13, 2017

Copy link
Copy Markdown
Member

@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.
BridgeAR reacted with thumbs up emoji
All reactions
  • 👍 1 reaction

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@mhdawson

mhdawson commented Sep 14, 2017

Copy link
Copy Markdown
Member

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

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

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@mhdawson

mhdawson commented Sep 14, 2017 •
edited
Loading

Uh oh!

There was an error while loading. Please reload this page.

Copy link
Copy Markdown
Member

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.
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
mhdawson
mhdawson approved these changes Sep 18, 2017
View reviewed changes

@mhdawson mhdawson left a comment

Copy link
Copy Markdown
Member

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.

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

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions
@mhdawson

mhdawson commented Sep 18, 2017

Copy link
Copy Markdown
Member

@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.
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
Gabriel Schulhof
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
gabrielschulhof force-pushed the remove-flag branch from 9b78363 to ca5d54f Compare September 19, 2017 20:32
@gabrielschulhof

gabrielschulhof commented Sep 19, 2017

Copy link
Copy Markdown
Contributor Author

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

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@gabrielschulhof

gabrielschulhof commented Sep 19, 2017

Copy link
Copy Markdown
Contributor Author

Weird linter failure, so I stopped that and started

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

instead.
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@gabrielschulhof

gabrielschulhof commented Sep 19, 2017

Copy link
Copy Markdown
Contributor Author 
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.
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@gabrielschulhof

gabrielschulhof commented Sep 19, 2017

Copy link
Copy Markdown
Contributor Author

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

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
Qard pushed a commit to Qard/ayo that referenced this pull request Sep 21, 2017
Gabriel Schulhof Stephen Belanger
n-api: remove n-api module loading flag
6e03392 
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 <[email protected]>
Reviewed-By: Colin Ihrig <[email protected]>
Reviewed-By: Anna Henningsen <[email protected]>
Reviewed-By: Matteo Collina <[email protected]>
Reviewed-By: Michael Dawson <[email protected]>
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Hitesh Kanwathirtha <[email protected]>
Qard pushed a commit to Qard/ayo that referenced this pull request Sep 21, 2017
Gabriel Schulhof Stephen Belanger
n-api: remove n-api module loading flag
c859a10 
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 <[email protected]>
Reviewed-By: Colin Ihrig <[email protected]>
Reviewed-By: Anna Henningsen <[email protected]>
Reviewed-By: Matteo Collina <[email protected]>
Reviewed-By: Michael Dawson <[email protected]>
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Hitesh Kanwathirtha <[email protected]>
@gabrielschulhof
gabrielschulhof deleted the remove-flag branch September 25, 2017 09:20
jasnell added a commit that referenced this pull request Sep 25, 2017
@jasnell
2017-09-26, Node.js Version 8.6.0 (Current)
883cd78 
* **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)
jasnell added a commit that referenced this pull request Sep 26, 2017
@jasnell
2017-09-26, Node.js Version 8.6.0 (Current)
b48ff2a 
* **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)
jasnell added a commit that referenced this pull request Sep 26, 2017
@jasnell
2017-09-26, Node.js Version 8.6.0 (Current)
05e4c1d 
* **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)
MylesBorins pushed a commit that referenced this pull request Sep 29, 2017
@jasnell @MylesBorins
2017-09-26, Node.js Version 8.6.0 (Current)
8f07f00 
* **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)
addaleax pushed a commit to addaleax/ayo that referenced this pull request Sep 30, 2017
@jasnell @addaleax
2017-09-26, Node.js Version 8.6.0 (Current)
b7374d9 
* **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)
@MylesBorins MylesBorins added the dont-land-on-v6.x label Oct 17, 2017
gabrielschulhof pushed a commit to gabrielschulhof/node that referenced this pull request Apr 16, 2018
Gabriel Schulhof
n-api: remove n-api module loading flag
a61dd4b 
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 <[email protected]>
Reviewed-By: Colin Ihrig <[email protected]>
Reviewed-By: Anna Henningsen <[email protected]>
Reviewed-By: Matteo Collina <[email protected]>
Reviewed-By: Michael Dawson <[email protected]>
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Hitesh Kanwathirtha <[email protected]>
@richardlau richardlau mentioned this pull request Apr 16, 2018
documentation of --napi-modules #20082
Closed
MylesBorins pushed a commit that referenced this pull request Apr 16, 2018
Gabriel Schulhof @MylesBorins
n-api: remove n-api module loading flag
5e29823 
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 <[email protected]>
Reviewed-By: Colin Ihrig <[email protected]>
Reviewed-By: Anna Henningsen <[email protected]>
Reviewed-By: Matteo Collina <[email protected]>
Reviewed-By: Michael Dawson <[email protected]>
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Hitesh Kanwathirtha <[email protected]>
@MylesBorins MylesBorins mentioned this pull request Apr 16, 2018
v6.14.2 proposal #19996
Merged
@bnoordhuis bnoordhuis mentioned this pull request Apr 18, 2018
src: kickstart addon by calling its constructor #20114
Closed
3 of 4 tasks
@richardlau richardlau mentioned this pull request Apr 25, 2018
n-api: update cli documentation #20301
Closed
3 tasks done
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@mcollina mcollina mcollina approved these changes
@jasnell jasnell jasnell approved these changes
@addaleax addaleax addaleax approved these changes
@cjihrig cjihrig cjihrig approved these changes
+3 more reviewers
@refack refack refack approved these changes
@digitalinfinity digitalinfinity digitalinfinity approved these changes
@mhdawson mhdawson mhdawson approved these changes
Reviewers whose approvals may not affect merge requirements

Uh oh!

There was an error while loading. Please reload this page.

Assignees

No one assigned

Labels

c++ Issues and PRs that require attention from people who are familiar with C++. node-api Issues and PRs related to Node-API.

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

Uh oh!

There was an error while loading. Please reload this page.

11 participants

@gabrielschulhof @refack @mhdawson @digitalinfinity @BridgeAR @mcollina @jasnell @addaleax @cjihrig @MylesBorins @nodejs-github-bot
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.
You are visitor 01402503 — GeezerVison does not store the pages you look up.
Best viewed with Netscape Navigator 4.7 or Internet Explorer 6 at 800×600. No cookies required.