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: initialize a module via a special symbol by gabrielschulhof · Pull Request #20161 · nodejs/node

Conversation

@gabrielschulhof

@gabrielschulhof gabrielschulhof commented Apr 20, 2018

Copy link
Copy Markdown
Contributor

Much like regular modules, N-API modules can also benefit from having
a special symbol which they can expose.

Fixes: #19845

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

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++. lib / src Issues and PRs involving general changes in the lib/ or src/ directories. labels Apr 20, 2018
@gabrielschulhof gabrielschulhof mentioned this pull request Apr 20, 2018
src: kickstart addon by calling its constructor #20114
Closed
3 of 4 tasks
@gabrielschulhof gabrielschulhof added node-api Issues and PRs related to Node-API. semver-minor PRs that contain new features and should be released in the next minor version. labels Apr 20, 2018
@gabrielschulhof
gabrielschulhof requested a review from bnoordhuis April 20, 2018 00:41
vsemozhetbyt
vsemozhetbyt reviewed Apr 20, 2018
View reviewed changes
Comment thread doc/api/n-api.md Outdated

@vsemozhetbyt vsemozhetbyt Apr 20, 2018

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.

Nit: this line has 81 characters and may cause doc linting issue.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions
@gabrielschulhof
gabrielschulhof force-pushed the n-api-special-symbol branch from d0c4129 to b699724 Compare April 20, 2018 03:11
@gabrielschulhof

gabrielschulhof commented Apr 20, 2018

Copy link
Copy Markdown
Contributor Author

@vsemozhetbyt fixed. Thanks!
vsemozhetbyt 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.
bnoordhuis
bnoordhuis approved these changes Apr 20, 2018
View reviewed changes

@bnoordhuis bnoordhuis 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 modulo comments.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions
Comment thread doc/api/n-api.md Outdated

@bnoordhuis bnoordhuis Apr 20, 2018

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.

Can you break out the stylistic changes to a separate commit?

Sorry, something went wrong.

Uh oh!

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

@bnoordhuis bnoordhuis Apr 20, 2018

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.

This is arguably a separate change that belongs in its own commit.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions
Comment thread tools/remark-cli/package-lock.json Outdated

@bnoordhuis bnoordhuis Apr 20, 2018

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.

Should not be checked in.

Kinda annoying make lint still touches this file, I thought that had been fixed.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions
This was referenced Apr 21, 2018
src: cover extra load-via-special-symbol scenario #20186
Closed
doc: remove "For example" expression in N-API doc #20187
Closed
Gabriel Schulhof
n-api: initialize a module via a special symbol
051029c 
Much like regular modules, N-API modules can also benefit from having
a special symbol which they can expose.

Fixes: nodejs#19845
@gabrielschulhof
gabrielschulhof force-pushed the n-api-special-symbol branch from b699724 to 051029c Compare April 21, 2018 03:05
@gabrielschulhof

gabrielschulhof commented Apr 21, 2018

Copy link
Copy Markdown
Contributor Author

@bnoordhuis I split out the changes into two more PRs.
All reactions

Sorry, something went wrong.

Uh oh!

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

bnoordhuis commented Apr 21, 2018

Copy link
Copy Markdown
Member

That's fine. Separate commits in this PR would have been alright too. The main thing is to have one change per commit, makes bisecting/reverting and code archeology easier.
gabrielschulhof 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.
@gabrielschulhof

gabrielschulhof commented Apr 22, 2018

Copy link
Copy Markdown
Contributor Author

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

Sorry, something went wrong.

Uh oh!

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

gabrielschulhof commented Apr 23, 2018

Copy link
Copy Markdown
Contributor Author

Landed in 0f8caf2.
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@gabrielschulhof gabrielschulhof closed this Apr 23, 2018
gabrielschulhof pushed a commit that referenced this pull request Apr 23, 2018
Gabriel Schulhof
n-api: initialize a module via a special symbol
0f8caf2 
Much like regular modules, N-API modules can also benefit from having
a special symbol which they can expose.

Fixes: #19845
PR-URL: #20161
Reviewed-By: Ben Noordhuis <[email protected]>
@gabrielschulhof
gabrielschulhof deleted the n-api-special-symbol branch April 23, 2018 21:25
MylesBorins pushed a commit that referenced this pull request May 4, 2018
Gabriel Schulhof @MylesBorins
n-api: initialize a module via a special symbol
cd83df3 
Much like regular modules, N-API modules can also benefit from having
a special symbol which they can expose.

Fixes: #19845
PR-URL: #20161
Reviewed-By: Ben Noordhuis <[email protected]>
MylesBorins added a commit that referenced this pull request May 8, 2018
@MylesBorins
2018-05-08, Version 10.1.0 (Current)
216acb1 
Notable Changes:

* console:
  - make console.table() use colored inspect (TSUYUSATO Kitsune)
    #20510
* fs:
  - move fs/promises to fs.promises (cjihrig)
    #20504
* http:
  - added aborted property to request (Robert Nagy)
    #20094
* n-api:
  - initialize a module via a special symbol (Gabriel Schulhof)
    #20161
* src:
  - add public API to expose the main V8 Platform (Allen Yonghuang Wang)
    #20447

PR-URL: Coming Soon
@MylesBorins MylesBorins mentioned this pull request May 8, 2018
v10.1.0 proposal #20606
Merged
MylesBorins added a commit that referenced this pull request May 8, 2018
@MylesBorins
2018-05-08, Version 10.1.0 (Current)
f8ed2a6 
Notable Changes:

* console:
  - make console.table() use colored inspect (TSUYUSATO Kitsune)
    #20510
* fs:
  - move fs/promises to fs.promises (cjihrig)
    #20504
* http:
  - added aborted property to request (Robert Nagy)
    #20094
* n-api:
  - initialize a module via a special symbol (Gabriel Schulhof)
    #20161
* src:
  - add public API to expose the main V8 Platform (Allen Yonghuang Wang)
    #20447

PR-URL: #20606
MylesBorins added a commit that referenced this pull request May 9, 2018
@MylesBorins
2018-05-08, Version 10.1.0 (Current)
687867d 
Notable Changes:

* console:
  - make console.table() use colored inspect (TSUYUSATO Kitsune)
    #20510
* fs:
  - move fs/promises to fs.promises (cjihrig)
    #20504
* http:
  - added aborted property to request (Robert Nagy)
    #20094
* n-api:
  - initialize a module via a special symbol (Gabriel Schulhof)
    #20161
* src:
  - add public API to expose the main V8 Platform (Allen Yonghuang Wang)
    #20447

PR-URL: #20606
MylesBorins added a commit that referenced this pull request May 9, 2018
@MylesBorins
2018-05-08, Version 10.1.0 (Current)
2c0fc3b 
Notable Changes:

* console:
  - make console.table() use colored inspect (TSUYUSATO Kitsune)
    #20510
* fs:
  - move fs/promises to fs.promises (cjihrig)
    #20504
* http:
  - added aborted property to request (Robert Nagy)
    #20094
* n-api:
  - initialize a module via a special symbol (Gabriel Schulhof)
    #20161
* src:
  - add public API to expose the main V8 Platform (Allen Yonghuang Wang)
    #20447

PR-URL: #20606
@vmoroz vmoroz mentioned this pull request Jan 23, 2023
node-api: deprecate napi_module_register based registration #46319
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@bnoordhuis bnoordhuis bnoordhuis approved these changes
+1 more reviewer
@vsemozhetbyt vsemozhetbyt vsemozhetbyt left review comments
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++. lib / src Issues and PRs involving general changes in the lib/ or src/ directories. node-api Issues and PRs related to Node-API. semver-minor PRs that contain new features and should be released in the next minor version.

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

No N-API equivalent for new well-known-symbol loading feature

Uh oh!

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

4 participants

@gabrielschulhof @bnoordhuis @vsemozhetbyt @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.