Skip to content

node-api: convert NewEnv to node_napi_env__::New #57834

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

Merged
merged 1 commit into from
Apr 19, 2025

Conversation

vmoroz
Copy link
Member

@vmoroz vmoroz commented Apr 11, 2025

Convert local v8impl::NewEnv function to the node_napi_env__::New method.
The new method helps creating new Node-API environment by any code that includes the node_api_internals.h header file.
There are no changes to the function bodies except for moving them to the top of the file.
The ThrowNodeApiVersionError had to be moved because the node_napi_env__::New uses it.

This change is required for the new C-based Node.js embedding API - PR #54660.
Since the PR #54660 is too big, it was decided in a Node-API meeting to split it up into smaller PRs.
This is the first PR in the series.

@vmoroz vmoroz added the node-api Issues and PRs related to the Node-API. label Apr 11, 2025
@nodejs-github-bot
Copy link
Collaborator

Review requested:

  • @nodejs/node-api

@nodejs-github-bot nodejs-github-bot added c++ Issues and PRs that require attention from people who are familiar with C++. needs-ci PRs that need a full CI run. labels Apr 11, 2025
@legendecas legendecas moved this from Need Triage to In Progress in Node-API Team Project Apr 11, 2025
Copy link

codecov bot commented Apr 11, 2025

Codecov Report

Attention: Patch coverage is 37.03704% with 17 lines in your changes missing coverage. Please review.

Project coverage is 90.15%. Comparing base (795dd8e) to head (66c68ed).
Report is 51 commits behind head on main.

Files with missing lines Patch % Lines
src/node_api.cc 37.03% 15 Missing and 2 partials ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main   #57834      +/-   ##
==========================================
- Coverage   90.24%   90.15%   -0.09%     
==========================================
  Files         630      628       -2     
  Lines      185470   185036     -434     
  Branches    36375    36234     -141     
==========================================
- Hits       167371   166823     -548     
- Misses      10993    11164     +171     
+ Partials     7106     7049      -57     
Files with missing lines Coverage Δ
src/node_api_internals.h 100.00% <ø> (ø)
src/node_api.cc 76.16% <37.03%> (+0.02%) ⬆️

... and 50 files with indirect coverage changes

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
  • 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

Copy link
Member

@mertcanaltin mertcanaltin left a comment

Choose a reason for hiding this comment

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

lgtm

@legendecas legendecas added the request-ci Add this label to start a Jenkins CI on a PR. label Apr 12, 2025
@github-actions github-actions bot removed the request-ci Add this label to start a Jenkins CI on a PR. label Apr 12, 2025
@nodejs-github-bot
Copy link
Collaborator

@nodejs-github-bot
Copy link
Collaborator

@vmoroz
Copy link
Member Author

vmoroz commented Apr 14, 2025

The build fails because VS2022 build for ARM64 hits out of memory issue:

C:\Program Files\Microsoft Visual Studio\2022\Community\VC\Tools\MSVC\14.43.34808\include\tuple(132,1): fatal  error C1060: compiler is out of heap space [C:\workspace\node-compile-windows\node\tools\v8_gypfiles\v8_initializers.vcxproj]
C:\Program Files\Microsoft Visual Studio\2022\Community\VC\Tools\MSVC\14.43.34808\include\type_traits(1527,92): fatal  error C1060: compiler is out of heap space [C:\workspace\node-compile-windows\node\tools\v8_gypfiles\v8_initializers.vcxproj]

I wonder if it is a known issue and someone is looking at it, or I should see where to add the /Zm compiler option to limit the memory use.

Copy link
Member

@mhdawson mhdawson left a comment

Choose a reason for hiding this comment

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

LGTM

@nodejs-github-bot
Copy link
Collaborator

@StefanStojanovic
Copy link
Contributor

The build fails because VS2022 build for ARM64 hits out of memory issue:

C:\Program Files\Microsoft Visual Studio\2022\Community\VC\Tools\MSVC\14.43.34808\include\tuple(132,1): fatal  error C1060: compiler is out of heap space [C:\workspace\node-compile-windows\node\tools\v8_gypfiles\v8_initializers.vcxproj]
C:\Program Files\Microsoft Visual Studio\2022\Community\VC\Tools\MSVC\14.43.34808\include\type_traits(1527,92): fatal  error C1060: compiler is out of heap space [C:\workspace\node-compile-windows\node\tools\v8_gypfiles\v8_initializers.vcxproj]

I wonder if it is a known issue and someone is looking at it, or I should see where to add the /Zm compiler option to limit the memory use.

Hello. Yes, this is a known problem, although it doesn't occur often. We are already setting /Zm2000. It could be increased e.g., /Zm3000, but that will probably not be necessary. The reason for this is that in Node.js v24 and later, we'll be moving from MSVC to ClangCL. This is a PR making it official which should land sometime this week. With ClangCL, we are not using PCH, as we have enabled ccache, so this will not be an issue.

I have restarted CI for this, so let's see if it succeeds this time.

@lpinca lpinca added the commit-queue Add this label to land a pull request using GitHub Actions. label Apr 19, 2025
@nodejs-github-bot nodejs-github-bot removed the commit-queue Add this label to land a pull request using GitHub Actions. label Apr 19, 2025
@nodejs-github-bot nodejs-github-bot merged commit c326200 into nodejs:main Apr 19, 2025
72 checks passed
@nodejs-github-bot
Copy link
Collaborator

Landed in c326200

@github-project-automation github-project-automation bot moved this from In Progress to Done in Node-API Team Project Apr 19, 2025
@vmoroz vmoroz deleted the pr/convert_newenv_to_a_method branch April 20, 2025 03:15
@vmoroz
Copy link
Member Author

vmoroz commented Apr 20, 2025

@StefanStojanovic and @lpinca , thank you for helping with the PR completion!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
c++ Issues and PRs that require attention from people who are familiar with C++. needs-ci PRs that need a full CI run. node-api Issues and PRs related to the Node-API.
Projects
Development

Successfully merging this pull request may close these issues.

8 participants