Skip to content

Conversation

@ri7116
Copy link
Contributor

@ri7116 ri7116 commented Oct 8, 2025

Replace ToLocalChecked() with MaybeLocal::ToLocal() for BigInt::NewFromWords.
On failure, return napi_set_last_error(env, napi_generic_failure) to correctly propagate the error instead of risking a crash.
This aligns with existing error-handling macros and avoids unchecked conversions.

@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 Oct 8, 2025
Replace ToLocalChecked() with MaybeLocal::ToLocal() for
BigInt::NewFromWords. On failure, return
napi_set_last_error(env, napi_generic_failure) to correctly
propagate the error instead of risking a crash.

This aligns with existing error-handling macros and avoids
unchecked conversions.
@ri7116 ri7116 force-pushed the fix/napi-tolocal-bigint-words branch from 46a882a to 3d03c6e Compare October 8, 2025 12:05
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.

CHECK_MAYBE_EMPTY_WITH_PREAMBLE should already handles the empty result from v8::BigInt::NewFromWords.

Could you clarify how this change is different from CHECK_MAYBE_EMPTY_WITH_PREAMBLE?

@codecov
Copy link

codecov bot commented Oct 8, 2025

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 88.54%. Comparing base (0fb0406) to head (3d03c6e).
⚠️ Report is 3 commits behind head on main.

Additional details and impacted files
@@            Coverage Diff             @@
##             main   #60165      +/-   ##
==========================================
- Coverage   88.55%   88.54%   -0.01%     
==========================================
  Files         704      704              
  Lines      208089   208091       +2     
  Branches    40017    40015       -2     
==========================================
- Hits       184270   184262       -8     
- Misses      15818    15861      +43     
+ Partials     8001     7968      -33     
Files with missing lines Coverage Δ
src/js_native_api_v8.cc 76.52% <100.00%> (+0.02%) ⬆️

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

@ri7116 ri7116 closed this Oct 8, 2025
@github-project-automation github-project-automation bot moved this from Need Triage to Done in Node-API Team Project Oct 8, 2025
@ri7116
Copy link
Contributor Author

ri7116 commented Oct 8, 2025

CHECK_MAYBE_EMPTY_WITH_PREAMBLE should already handles the empty result from v8::BigInt::NewFromWords.

Could you clarify how this change is different from CHECK_MAYBE_EMPTY_WITH_PREAMBLE?

Thank you for clarifying, It seems I misunderstood. Just to confirm, is it correct to say that ToLocalChecked() is safe to use as long as the value has been validated beforehand?"

@legendecas
Copy link
Member

Just to confirm, is it correct to say that ToLocalChecked() is safe to use as long as the value has been validated beforehand?"

Yes, it is corret. For cases, we would recommend ToLocal combined with an if branch to reduce the checks. And in conditions like during bootstrap, we would also use ToLocalChecked() when we don't expect a value to be empty otherwise it should crash immediately.

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

Archived in project

Development

Successfully merging this pull request may close these issues.

3 participants