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.

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

doc: add tips for NODE_MODULE #45797

Merged
merged 1 commit into from
Dec 19, 2022

Conversation

theanarkh
Copy link
Contributor

If we define an addon with NODE_MODULE, we can not loaded it both in the
main and worker thread.

  • 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 addons Issues and PRs related to native addons. doc Issues and PRs related to the documentations. labels Dec 9, 2022
@theanarkh theanarkh changed the title doc: add tip for NODE_MODULE doc: add tips for NODE_MODULE Dec 9, 2022
doc/api/addons.md Outdated Show resolved Hide resolved
@theanarkh theanarkh force-pushed the add_tips_for_NODE_MODUILE branch 3 times, most recently from 5623814 to 66862e7 Compare December 10, 2022 04:37
@theanarkh theanarkh added the author ready PRs that have at least one approval, no pending requests for changes, and a CI started. label Dec 10, 2022
@theanarkh
Copy link
Contributor Author

@legendecas @addaleax Hi, can you help review this PR ? Thanks !

in worker.
2. If you load the addon in worker first, you can not load the addon in
main thread or other workers until the worker which have loaded the addon
exits.
Copy link
Member

Choose a reason for hiding this comment

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

I think this could be made a bit more succinct, e.g. “Addons defined with NODE_MODULE() cannot be loaded in multiple contexts or multiple threads at the same time.”?

I wouldn’t make the text here too verbose since the next section already touches on this quite a bit.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks, i have copied your text into docs.

@theanarkh theanarkh added the commit-queue Add this label to land a pull request using GitHub Actions. label Dec 19, 2022
@nodejs-github-bot nodejs-github-bot removed the commit-queue Add this label to land a pull request using GitHub Actions. label Dec 19, 2022
@nodejs-github-bot nodejs-github-bot merged commit b7f7651 into nodejs:main Dec 19, 2022
@nodejs-github-bot
Copy link
Collaborator

Landed in b7f7651

targos pushed a commit that referenced this pull request Jan 1, 2023
PR-URL: #45797
Reviewed-By: Gerhard Stöbich <deb2001-github@yahoo.de>
Reviewed-By: Chengzhong Wu <legendecas@gmail.com>
@RafaelGSS RafaelGSS mentioned this pull request Jan 2, 2023
RafaelGSS pushed a commit that referenced this pull request Jan 5, 2023
PR-URL: #45797
Reviewed-By: Gerhard Stöbich <deb2001-github@yahoo.de>
Reviewed-By: Chengzhong Wu <legendecas@gmail.com>
juanarbol pushed a commit that referenced this pull request Jan 26, 2023
PR-URL: #45797
Reviewed-By: Gerhard Stöbich <deb2001-github@yahoo.de>
Reviewed-By: Chengzhong Wu <legendecas@gmail.com>
@juanarbol juanarbol mentioned this pull request Jan 28, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
addons Issues and PRs related to native addons. author ready PRs that have at least one approval, no pending requests for changes, and a CI started. doc Issues and PRs related to the documentations.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants