Skip to content

Conversation

JeetuSuthar
Copy link
Contributor

This PR updates the signature of napi_delete_reference to accept a node_api_basic_env instead of a napi_env, as discussed in #59583. This change reflects that napi_delete_reference does not require the environment to be in a non-GC state and aligns the API with its intended usage.

  • Updates the function signature in both the implementation and header files.
  • Refactors all internal and test usages to pass a node_api_basic_env as the first argument.

Refs: #59583

Checklist
  • PR description includes references to the issue
  • Function signature and all usages updated
  • Tests updated to match new signature

/cc @nodejs/node-api

@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. node-api Issues and PRs related to the Node-API. labels Aug 30, 2025
@JeetuSuthar
Copy link
Contributor Author

@legendecas I removed the duplicated lines

@JeetuSuthar
Copy link
Contributor Author

@legendecas Hi, Could you please approve and run the CI..

Copy link

codecov bot commented Sep 1, 2025

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 89.93%. Comparing base (5af0355) to head (f58f962).
⚠️ Report is 106 commits behind head on main.

Additional details and impacted files
@@            Coverage Diff             @@
##             main   #59684      +/-   ##
==========================================
+ Coverage   89.87%   89.93%   +0.05%     
==========================================
  Files         667      667              
  Lines      196856   196776      -80     
  Branches    38653    38410     -243     
==========================================
+ Hits       176927   176973      +46     
+ Misses      12372    12213     -159     
- Partials     7557     7590      +33     
Files with missing lines Coverage Δ
src/js_native_api_v8.cc 76.56% <100.00%> (ø)

... and 54 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.

@JeetuSuthar
Copy link
Contributor Author

JeetuSuthar commented Sep 1, 2025

@legendecas @RafaelGSS @benjamingr Could you please approve and run the CI again? I’ve fixed all formatting and linter issues.

Copy link
Member

@legendecas legendecas left a comment

Choose a reason for hiding this comment

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

I believe the static_casts in the tests are unnecessary as node_api_basic_env is designed to be compatible with napi_env. It is a const napi_env.

Would you mind removing the changes with static_casts?

if (node_api_version == NAPI_VERSION_EXPERIMENTAL) {
CHECK_EQ(napi_create_reference(env, key, 1, &ref), napi_ok);
CHECK_EQ(napi_delete_reference(env, ref), napi_ok);
CHECK_EQ(napi_delete_reference(static_cast<node_api_basic_env>(env), ref),
Copy link
Member

Choose a reason for hiding this comment

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

I don't think this static_cast is necessary.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ok let me remove that

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@legendecas I removed the static cast

@JeetuSuthar
Copy link
Contributor Author

JeetuSuthar commented Sep 2, 2025

@legendecas I removed the static cast changes can u plz check

@JeetuSuthar
Copy link
Contributor Author

@legendecas any updates ...? Or any changes I have to do


MyObject::~MyObject() {
napi_delete_reference(env_, wrapper_);
napi_delete_reference(static_cast<node_api_basic_env>(env_), wrapper_);
Copy link
Member

Choose a reason for hiding this comment

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

Would you mind removing this change? As mentioned earlier, this is not necessary.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes done

MyObject::MyObject() : env_(nullptr), wrapper_(nullptr) {}

MyObject::~MyObject() { napi_delete_reference(env_, wrapper_); }
MyObject::~MyObject() {
Copy link
Member

Choose a reason for hiding this comment

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

Would you mind removing this unnecessary formatting?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@JeetuSuthar
Copy link
Contributor Author

@legendecas fixed unnecessary formatting and static class

@JeetuSuthar
Copy link
Contributor Author

Hi @legendecas, I’ve addressed all the feedback and all checks are passing .
Could you please take a final look and approve when you get a chance?
Thanks!

@JeetuSuthar
Copy link
Contributor Author

Any updates?

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

@nodejs-github-bot
Copy link
Collaborator

@legendecas legendecas moved this from Need Triage to In Progress in Node-API Team Project Sep 12, 2025
@legendecas legendecas added author ready PRs that have at least one approval, no pending requests for changes, and a CI started. commit-queue Add this label to land a pull request using GitHub Actions. commit-queue-squash Add this label to instruct the Commit Queue to squash all the PR commits into the first one. labels Sep 12, 2025
@nodejs-github-bot nodejs-github-bot removed the commit-queue Add this label to land a pull request using GitHub Actions. label Sep 12, 2025
@nodejs-github-bot nodejs-github-bot merged commit af4eac5 into nodejs:main Sep 12, 2025
68 checks passed
@nodejs-github-bot
Copy link
Collaborator

Landed in af4eac5

@github-project-automation github-project-automation bot moved this from In Progress to Done in Node-API Team Project Sep 12, 2025
targos pushed a commit that referenced this pull request Sep 18, 2025
PR-URL: #59684
Refs: #59583
Reviewed-By: Chengzhong Wu <legendecas@gmail.com>
Reviewed-By: Vladimir Morozov <vmorozov@microsoft.com>
targos pushed a commit that referenced this pull request Sep 24, 2025
PR-URL: #59684
Refs: #59583
Reviewed-By: Chengzhong Wu <legendecas@gmail.com>
Reviewed-By: Vladimir Morozov <vmorozov@microsoft.com>
tmeijn pushed a commit to tmeijn/dotfiles that referenced this pull request Sep 26, 2025
This MR contains the following updates:

| Package | Update | Change |
|---|---|---|
| [node](https://nodejs.org) ([source](https://github.com/nodejs/node)) | minor | `24.8.0` -> `24.9.0` |

MR created with the help of [el-capitano/tools/renovate-bot](https://gitlab.com/el-capitano/tools/renovate-bot).

**Proposed changes to behavior should be submitted there as MRs.**

---

### Release Notes

<details>
<summary>nodejs/node (node)</summary>

### [`v24.9.0`](https://github.com/nodejs/node/releases/tag/v24.9.0): 2025-09-25, Version 24.9.0 (Current), @&#8203;targos

[Compare Source](nodejs/node@v24.8.0...v24.9.0)

##### Notable Changes

- \[[`9b043a9096`](nodejs/node@9b043a9096)] - **(SEMVER-MINOR)** **http**: add shouldUpgradeCallback to let servers control HTTP upgrades (Tim Perry) [#&#8203;59824](nodejs/node#59824)
- \[[`a6456ab90a`](nodejs/node@a6456ab90a)] - **(SEMVER-MINOR)** **sqlite**: cleanup ERM support and export Session class (James M Snell) [#&#8203;58378](nodejs/node#58378)
- \[[`5563361d22`](nodejs/node@5563361d22)] - **(SEMVER-MINOR)** **sqlite**: add tagged template (0hm☘️) [#&#8203;58748](nodejs/node#58748)
- \[[`04013ee933`](nodejs/node@04013ee933)] - **(SEMVER-MINOR)** **worker**: add heap profile API (theanarkh) [#&#8203;59846](nodejs/node#59846)

##### Commits

- \[[`cbec4fd6de`](nodejs/node@cbec4fd6de)] - **benchmark**: calibrate config dgram multi-buffer (Bruno Rodrigues) [#&#8203;59696](nodejs/node#59696)
- \[[`9a4bbdc3c5`](nodejs/node@9a4bbdc3c5)] - **benchmark**: calibrate config cluster/echo.js (Nam Yooseong) [#&#8203;59836](nodejs/node#59836)
- \[[`0b284d86e8`](nodejs/node@0b284d86e8)] - **build**: add the missing macro definitions for OpenHarmony (hqzing) [#&#8203;59804](nodejs/node#59804)
- \[[`43e6e54d66`](nodejs/node@43e6e54d66)] - **build**: do not include custom ESLint rules testing in tarball (Antoine du Hamel) [#&#8203;59809](nodejs/node#59809)
- \[[`039ac19154`](nodejs/node@039ac19154)] - **crypto**: expose signatureAlgorithm on X509Certificate (Patrick Costa) [#&#8203;59235](nodejs/node#59235)
- \[[`647c332704`](nodejs/node@647c332704)] - **crypto**: use `return await` when returning Promises from async functions (Renegade334) [#&#8203;59841](nodejs/node#59841)
- \[[`8ed4587cf0`](nodejs/node@8ed4587cf0)] - **crypto**: use async functions for non-stub Promise-returning functions (Renegade334) [#&#8203;59841](nodejs/node#59841)
- \[[`bb051c56ef`](nodejs/node@bb051c56ef)] - **crypto**: avoid calls to `promise.catch()` (Renegade334) [#&#8203;59841](nodejs/node#59841)
- \[[`05e560dd25`](nodejs/node@05e560dd25)] - **deps**: update googletest to [`50b8600`](nodejs/node@50b8600) (Node.js GitHub Bot) [#&#8203;59955](nodejs/node#59955)
- \[[`fa40d3a785`](nodejs/node@fa40d3a785)] - **deps**: update archs files for openssl-3.5.3 (Node.js GitHub Bot) [#&#8203;59901](nodejs/node#59901)
- \[[`8c85570d18`](nodejs/node@8c85570d18)] - **deps**: upgrade openssl sources to openssl-3.5.3 (Node.js GitHub Bot) [#&#8203;59901](nodejs/node#59901)
- \[[`b71125664e`](nodejs/node@b71125664e)] - **deps**: update undici to 7.16.0 (Node.js GitHub Bot) [#&#8203;59830](nodejs/node#59830)
- \[[`dea5dd7077`](nodejs/node@dea5dd7077)] - **dgram**: restore buffer optimization in fixBufferList (Yoo) [#&#8203;59934](nodejs/node#59934)
- \[[`b0c1e67532`](nodejs/node@b0c1e67532)] - **diagnostics\_channel**: fix race condition with diagnostics\_channel and GC (Ugaitz Urien) [#&#8203;59910](nodejs/node#59910)
- \[[`0b37b594c3`](nodejs/node@0b37b594c3)] - **doc**: use "WebAssembly" instead of "Web Assembly" (Tobias Nießen) [#&#8203;59954](nodejs/node#59954)
- \[[`1e723f9c6b`](nodejs/node@1e723f9c6b)] - **doc**: fix typo in section on microtask order (Tobias Nießen) [#&#8203;59932](nodejs/node#59932)
- \[[`a28962a85c`](nodejs/node@a28962a85c)] - **doc**: update V8 fast API guidance (René) [#&#8203;58999](nodejs/node#58999)
- \[[`bd767c5d1b`](nodejs/node@bd767c5d1b)] - **doc**: add security escalation policy (Ulises Gascón) [#&#8203;59806](nodejs/node#59806)
- \[[`9df91e59e1`](nodejs/node@9df91e59e1)] - **doc**: type improvement of file `http.md` (yusheng chen) [#&#8203;58189](nodejs/node#58189)
- \[[`e4f571680b`](nodejs/node@e4f571680b)] - **doc**: deprecate closing `fs.Dir` on garbage collection (Livia Medeiros) [#&#8203;59839](nodejs/node#59839)
- \[[`e9cb986fa5`](nodejs/node@e9cb986fa5)] - **doc**: rephrase dynamic import() description (Nam Yooseong) [#&#8203;59224](nodejs/node#59224)
- \[[`026d4e33f7`](nodejs/node@026d4e33f7)] - **doc,crypto**: update subtle.generateKey and subtle.importKey (Filip Skokan) [#&#8203;59851](nodejs/node#59851)
- \[[`2b2591db52`](nodejs/node@2b2591db52)] - **esm**: make hasAsyncGraph non-enumerable (Joyee Cheung) [#&#8203;59905](nodejs/node#59905)
- \[[`993f05d323`](nodejs/node@993f05d323)] - **fs,win**: do not add a second trailing slash in readdir (Gerhard Stöbich) [#&#8203;59847](nodejs/node#59847)
- \[[`7aec53b607`](nodejs/node@7aec53b607)] - **(SEMVER-MINOR)** **http**: add shouldUpgradeCallback to let servers control HTTP upgrades (Tim Perry) [#&#8203;59824](nodejs/node#59824)
- \[[`83ae6102e7`](nodejs/node@83ae6102e7)] - **http**: optimize checkIsHttpToken for short strings (방진혁) [#&#8203;59832](nodejs/node#59832)
- \[[`6695067636`](nodejs/node@6695067636)] - **http,https**: handle IPv6 with proxies (Joyee Cheung) [#&#8203;59894](nodejs/node#59894)
- \[[`c5d910a0a9`](nodejs/node@c5d910a0a9)] - **http2**: fix allowHttp1+Upgrade, broken by shouldUpgradeCallback (Tim Perry) [#&#8203;59924](nodejs/node#59924)
- \[[`acada1fb82`](nodejs/node@acada1fb82)] - **inspector**: ensure adequate memory allocation for `Binary::toBase64` (René) [#&#8203;59870](nodejs/node#59870)
- \[[`396cc8ec65`](nodejs/node@396cc8ec65)] - **lib**: update inspect output format for subclasses (Miguel Marcondes Filho) [#&#8203;59687](nodejs/node#59687)
- \[[`fed1dac8de`](nodejs/node@fed1dac8de)] - **lib**: update isDeepStrictEqual to support options (Miguel Marcondes Filho) [#&#8203;59762](nodejs/node#59762)
- \[[`d785929fd7`](nodejs/node@d785929fd7)] - **lib**: add source map support for assert messages (Chengzhong Wu) [#&#8203;59751](nodejs/node#59751)
- \[[`ff13d1d61e`](nodejs/node@ff13d1d61e)] - **lib,src**: cache ModuleWrap.hasAsyncGraph (Chengzhong Wu) [#&#8203;59703](nodejs/node#59703)
- \[[`b200cd8470`](nodejs/node@b200cd8470)] - **lib,src**: refactor assert to load error source from memory (Chengzhong Wu) [#&#8203;59751](nodejs/node#59751)
- \[[`e94c57301b`](nodejs/node@e94c57301b)] - **meta**: add .npmrc with ignore-scripts=true (Joyee Cheung) [#&#8203;59914](nodejs/node#59914)
- \[[`728472a57b`](nodejs/node@728472a57b)] - **module**: only put directly require-d ESM into require.cache (Joyee Cheung) [#&#8203;59874](nodejs/node#59874)
- \[[`be48760b93`](nodejs/node@be48760b93)] - **node-api**: added SharedArrayBuffer api (Mert Can Altin) [#&#8203;59071](nodejs/node#59071)
- \[[`f006a14522`](nodejs/node@f006a14522)] - **node-api**: make napi\_delete\_reference use node\_api\_basic\_env (Jeetu Suthar) [#&#8203;59684](nodejs/node#59684)
- \[[`0f46c1c3b0`](nodejs/node@0f46c1c3b0)] - **repl**: fix cpu overhead pasting big strings to the REPL (Ruben Bridgewater) [#&#8203;59857](nodejs/node#59857)
- \[[`3eeb7b47ea`](nodejs/node@3eeb7b47ea)] - **sqlite**: fix crash session extension callbacks with workers (Bart Louwers) [#&#8203;59848](nodejs/node#59848)
- \[[`0fe53375ec`](nodejs/node@0fe53375ec)] - **(SEMVER-MINOR)** **sqlite**: cleanup ERM support and export Session class (James M Snell) [#&#8203;58378](nodejs/node#58378)
- \[[`9a3e58a007`](nodejs/node@9a3e58a007)] - **(SEMVER-MINOR)** **sqlite**: add tagged template (0hm☘️) [#&#8203;58748](nodejs/node#58748)
- \[[`f14ed5ab7b`](nodejs/node@f14ed5ab7b)] - **src**: simplify watchdog instantiations via `std::optional` (Anna Henningsen) [#&#8203;59960](nodejs/node#59960)
- \[[`e330f03f84`](nodejs/node@e330f03f84)] - **src**: update crypto objects to use DictionaryTemplate (James M Snell) [#&#8203;59942](nodejs/node#59942)
- \[[`69b5607cf4`](nodejs/node@69b5607cf4)] - **src**: simplify is\_callable by making it a concept (Tobias Nießen) [#&#8203;58169](nodejs/node#58169)
- \[[`86150f3401`](nodejs/node@86150f3401)] - **src**: rename private fields to follow naming convention (Moonki Choi) [#&#8203;59923](nodejs/node#59923)
- \[[`d17f299539`](nodejs/node@d17f299539)] - **src**: use DictionaryTemplate more in URLPattern (James M Snell) [#&#8203;59892](nodejs/node#59892)
- \[[`ac784912ac`](nodejs/node@ac784912ac)] - **src**: reduce the nearest parent package JSON cache size (Michael Smith) [#&#8203;59888](nodejs/node#59888)
- \[[`abecdcb536`](nodejs/node@abecdcb536)] - **src**: replace FIXED\_ONE\_BYTE\_STRING with Environment-cached strings (Moonki Choi) [#&#8203;59891](nodejs/node#59891)
- \[[`2bb152500b`](nodejs/node@2bb152500b)] - **src**: create strings in `FIXED_ONE_BYTE_STRING` as internalized (Anna Henningsen) [#&#8203;59826](nodejs/node#59826)
- \[[`03116a7cd8`](nodejs/node@03116a7cd8)] - **src**: remove `std::array` overload of `FIXED_ONE_BYTE_STRING` (Anna Henningsen) [#&#8203;59826](nodejs/node#59826)
- \[[`8a5325d6e3`](nodejs/node@8a5325d6e3)] - **src**: ensure `v8::Eternal` is empty before setting it (Anna Henningsen) [#&#8203;59825](nodejs/node#59825)
- \[[`f0c20ccd81`](nodejs/node@f0c20ccd81)] - **src**: remove unnecessary `Environment::GetCurrent()` calls (Moonki Choi) [#&#8203;59814](nodejs/node#59814)
- \[[`213188e491`](nodejs/node@213188e491)] - **stream**: use new AsyncResource instead of bind (Matteo Collina) [#&#8203;59867](nodejs/node#59867)
- \[[`ce8435b003`](nodejs/node@ce8435b003)] - **test**: testcase demonstrating issue 59541 (Eric Rannaud) [#&#8203;59801](nodejs/node#59801)
- \[[`8f32746142`](nodejs/node@8f32746142)] - **test**: guard write to proxy client if proxy connection is ended (Joyee Cheung) [#&#8203;59742](nodejs/node#59742)
- \[[`6790093fcb`](nodejs/node@6790093fcb)] - **tls**: load bundled and extra certificates off-thread (Joyee Cheung) [#&#8203;59856](nodejs/node#59856)
- \[[`f5d3f919d8`](nodejs/node@f5d3f919d8)] - **tls**: only do off-thread certificate loading on loading tls (Joyee Cheung) [#&#8203;59856](nodejs/node#59856)
- \[[`87bbaa23a0`](nodejs/node@87bbaa23a0)] - **tools**: fix `tools/make-v8.sh` for clang (Richard Lau) [#&#8203;59893](nodejs/node#59893)
- \[[`0d23fd525b`](nodejs/node@0d23fd525b)] - **tools**: skip test-internet workflow for draft MRs (Michaël Zasso) [#&#8203;59817](nodejs/node#59817)
- \[[`e17c73731a`](nodejs/node@e17c73731a)] - **tools**: copyedit `build-tarball.yml` (Antoine du Hamel) [#&#8203;59808](nodejs/node#59808)
- \[[`97c4e1bac9`](nodejs/node@97c4e1bac9)] - **typings**: remove unused imports (Nam Yooseong) [#&#8203;59880](nodejs/node#59880)
- \[[`8b29bbca76`](nodejs/node@8b29bbca76)] - **url**: replaced slice with at (Mikhail) [#&#8203;59181](nodejs/node#59181)
- \[[`6458867a6b`](nodejs/node@6458867a6b)] - **url**: add type checking to urlToHttpOptions() (simon-id) [#&#8203;59753](nodejs/node#59753)
- \[[`3c62b3886f`](nodejs/node@3c62b3886f)] - **util**: inspect objects with throwing Symbol.toStringTag (Ruben Bridgewater) [#&#8203;59860](nodejs/node#59860)
- \[[`6133a82875`](nodejs/node@6133a82875)] - **util**: fix debuglog.enabled not being present with callback logger (Ruben Bridgewater) [#&#8203;59858](nodejs/node#59858)
- \[[`9347ddddf4`](nodejs/node@9347ddddf4)] - **vm**: explain how to share promises between contexts w/ afterEvaluate (Eric Rannaud) [#&#8203;59801](nodejs/node#59801)
- \[[`44ce971619`](nodejs/node@44ce971619)] - **vm**: "afterEvaluate", evaluate() return a promise from the outer context (Eric Rannaud) [#&#8203;59801](nodejs/node#59801)
- \[[`6e586a1409`](nodejs/node@6e586a1409)] - **vm**: expose hasTopLevelAwait on SourceTextModule (Chengzhong Wu) [#&#8203;59865](nodejs/node#59865)
- \[[`49747a58a3`](nodejs/node@49747a58a3)] - **(SEMVER-MINOR)** **worker**: add heap profile API (theanarkh) [#&#8203;59846](nodejs/node#59846)
- \[[`b970c0bbc2`](nodejs/node@b970c0bbc2)] - **zlib**: reduce code duplication (jhofstee) [#&#8203;57810](nodejs/node#57810)
- \[[`9782ca2b1b`](nodejs/node@9782ca2b1b)] - **zlib**: implement fast path for crc32 (Gürgün Dayıoğlu) [#&#8203;59813](nodejs/node#59813)

</details>

---

### Configuration

📅 **Schedule**: Branch creation - At any time (no schedule defined), Automerge - At any time (no schedule defined).

🚦 **Automerge**: Disabled by config. Please merge this manually once you are satisfied.

♻ **Rebasing**: Whenever MR becomes conflicted, or you tick the rebase/retry checkbox.

🔕 **Ignore**: Close this MR and you won't be reminded about this update again.

---

 - [ ] <!-- rebase-check -->If you want to rebase/retry this MR, check this box

---

This MR has been generated by [Renovate Bot](https://github.com/renovatebot/renovate).
<!--renovate-debug:eyJjcmVhdGVkSW5WZXIiOiI0MS4xMzAuMCIsInVwZGF0ZWRJblZlciI6IjQxLjEzMC4wIiwidGFyZ2V0QnJhbmNoIjoibWFpbiIsImxhYmVscyI6WyJSZW5vdmF0ZSBCb3QiXX0=-->
aduh95 pushed a commit that referenced this pull request Oct 7, 2025
PR-URL: #59684
Refs: #59583
Reviewed-By: Chengzhong Wu <legendecas@gmail.com>
Reviewed-By: Vladimir Morozov <vmorozov@microsoft.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
author ready PRs that have at least one approval, no pending requests for changes, and a CI started. c++ Issues and PRs that require attention from people who are familiar with C++. commit-queue-squash Add this label to instruct the Commit Queue to squash all the PR commits into the first one. needs-ci PRs that need a full CI run. node-api Issues and PRs related to the Node-API.
Projects
Archived in project
Development

Successfully merging this pull request may close these issues.

5 participants