Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

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

Closed
wants to merge 1 commit into from
Closed

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

wants to merge 1 commit into from

Conversation

gabrielschulhof
Copy link
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

@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
}
```

This macro includes `NAPI_MODULE`, and declares an `Init` function with a special
Copy link
Contributor

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

@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
For example, to define a class so that new instances can be created
(often used with [Object Wrap][]):
To define a class so that new instances can be created (often used with
[Object Wrap][]):
Copy link
Member

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
if (auto callback = GetInitializerCallback(&dlib)) {
callback(exports, module, context);
return;
}
Copy link
Member

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
@@ -2,7 +2,6 @@
"name": "remark-cli",
"version": "4.0.0",
"lockfileVersion": 1,
"preserveSymlinks": "1",
Copy link
Member

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

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

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

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

@gabrielschulhof
Copy link
Contributor Author

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

@vsemozhetbyt vsemozhetbyt vsemozhetbyt left review comments

@bnoordhuis bnoordhuis bnoordhuis approved these changes

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