Skip to content

n-api: initialize a module via a special symbol#20161

Closed
gabrielschulhof wants to merge 1 commit intonodejs:masterfrom
gabrielschulhof:n-api-special-symbol
Closed

n-api: initialize a module via a special symbol#20161
gabrielschulhof wants to merge 1 commit intonodejs:masterfrom
gabrielschulhof:n-api-special-symbol

Conversation

@gabrielschulhof
Copy link
Contributor

@gabrielschulhof gabrielschulhof commented Apr 20, 2018

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

@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 related to general changes in the lib or src directory. labels Apr 20, 2018
@gabrielschulhof gabrielschulhof mentioned this pull request Apr 20, 2018
Closed
4 tasks
@gabrielschulhof gabrielschulhof added node-api Issues and PRs related to the Node-API. semver-minor PRs that contain new features and should be released in the next minor version. labels Apr 20, 2018
vsemozhetbyt
vsemozhetbyt reviewed Apr 20, 2018
View reviewed changes
doc/api/n-api.md Outdated
Copy link
Contributor

@vsemozhetbyt vsemozhetbyt Apr 20, 2018

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.

@gabrielschulhof
Copy link
Contributor Author

gabrielschulhof commented Apr 20, 2018

@vsemozhetbyt fixed. Thanks!

bnoordhuis
bnoordhuis approved these changes Apr 20, 2018
View reviewed changes
Copy link
Member

@bnoordhuis bnoordhuis left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM modulo comments.

doc/api/n-api.md Outdated
Copy link
Member

@bnoordhuis bnoordhuis Apr 20, 2018

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?

src/node.cc Outdated
Copy link
Member

@bnoordhuis bnoordhuis Apr 20, 2018

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.

tools/remark-cli/package-lock.json Outdated
Copy link
Member

@bnoordhuis bnoordhuis Apr 20, 2018

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.

This was referenced Apr 21, 2018
Closed
Closed
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
Copy link
Contributor Author

gabrielschulhof commented Apr 21, 2018

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

@bnoordhuis
Copy link
Member

bnoordhuis commented Apr 21, 2018

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
Copy link
Contributor Author

gabrielschulhof commented Apr 22, 2018

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

@gabrielschulhof
Copy link
Contributor Author

gabrielschulhof commented Apr 23, 2018

Landed in 0f8caf2.

gabrielschulhof pushed a commit that referenced this pull request Apr 23, 2018
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 <info@bnoordhuis.nl>
@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
@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 <info@bnoordhuis.nl>
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
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
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

Assignees

No one assigned

Labels

c++ Issues and PRs that require attention from people who are familiar with C++. lib / src Issues and PRs related to general changes in the lib or src directory. node-api Issues and PRs related to the 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

4 participants

@gabrielschulhof @bnoordhuis @vsemozhetbyt @nodejs-github-bot