Skip to content

feat: Support V8 v14#32

Open
timfish wants to merge 7 commits intomainfrom
timfish/fix/v8-v14-issue
Open

feat: Support V8 v14#32
timfish wants to merge 7 commits intomainfrom
timfish/fix/v8-v14-issue

Conversation

@timfish
Copy link
Copy Markdown
Collaborator

@timfish timfish commented Apr 1, 2026

This PR adds support for V8 v14 by moving the json parsing from the native module into JavaScript. The previous solution was either causing build errors or runtime crashes depending on build config.

This PR also:

  • Updates node-abi to support newer Node.js versions
  • Ensures that if the binary is built from source due to no prebuilt match, it's copied to the expected location
  • Adds Node.js 25 to the test matrix which tests all of the above

Thanks to @nikitakot for supplying a thoroughly tested fix with a diff!

@github-actions
Copy link
Copy Markdown

github-actions bot commented Apr 1, 2026

Semver Impact of This PR

🟡 Minor (new features)

📋 Changelog Preview

This is how your changes will appear in the changelog.
Entries from this PR are highlighted with a left border (blockquote style).


New Features ✨

  • Support V8 v14 by timfish in #32

Bug Fixes 🐛

  • (security) Replace execSync with execFileSync to prevent command injection by fix-it-felix-sentry in #30

Internal Changes 🔧

Release

  • Fix changelog-preview permissions by BYK in #29
  • Bump Craft version to fix issues by BYK in #27
  • Switch from action-prepare-release to Craft by BYK in #25

Other

  • Enable Craft auto changelog by timfish in #33
  • Pin GitHub Actions to full-length commit SHAs by joshuarli in #31
  • Use pull_request_target for changelog preview by BYK in #28
  • macos-13 deprecation by timfish in #26

🤖 This preview updates automatically when you update the PR.

@timfish timfish changed the title fix: V8 v14 issue feat: Support V8 v14 Apr 2, 2026
@timfish timfish marked this pull request as ready for review April 2, 2026 19:26
@timfish timfish requested a review from Copilot April 2, 2026 19:27
Copy link
Copy Markdown

Copilot AI left a comment

Choose a reason for hiding this comment

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

Pull request overview

This PR aims to restore compatibility with V8 v14 (e.g., Electron v38) by avoiding v8::JSON::Parse usage in the native addon and shifting JSON parsing of thread state into the JavaScript wrapper, alongside updates to build/install behavior and CI coverage.

Changes:

  • Move asyncState/pollState JSON parsing from module.cc into src/index.ts.
  • Update node-abi to a newer version to cover newer Node.js ABIs.
  • Ensure source-built binaries are copied into the expected lib/ target on install; expand CI Node.js matrix to include Node 25.

Reviewed changes

Copilot reviewed 5 out of 6 changed files in this pull request and generated 1 comment.

Show a summary per file
File Description
src/index.ts Parses native-returned JSON strings into JS objects for asyncState/pollState.
module.cc Stops calling v8::JSON::Parse and returns JSON strings for state fields.
scripts/check-build.mjs Makes source compilation path also copy the built .node into the expected target location.
package.json Bumps node-abi dependency range.
yarn.lock Locks updated node-abi resolution.
.github/workflows/ci.yml Adds Node.js 25 to the test matrix.

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Copy link
Copy Markdown

@cursor cursor bot left a comment

Choose a reason for hiding this comment

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

Cursor Bugbot has reviewed your changes and found 1 potential issue.

Fix All in Cursor

Bugbot Autofix is OFF. To automatically fix reported issues with cloud agents, enable autofix in the Cursor dashboard.

const thread: Thread<A, P> = {
frames: value.frames,
...(value.asyncState && { asyncState: JSON.parse(value.asyncState) }),
...(value.pollState && { pollState: JSON.parse(value.pollState) }),
Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Unhandled JSON.parse can crash entire stack capture

Medium Severity

The JSON.parse calls for asyncState and pollState have no error handling. The previous C++ code using v8::JSON::Parse gracefully skipped a property if parsing failed, preserving the rest of the thread data. Now, if JSON.parse throws for any thread's state, the entire captureStackTrace() call fails and all thread data (including stack frames for every thread) is lost. Since this function is used in watchdog/monitoring contexts, a total failure means monitoring silently stops working. Wrapping these in try-catch to match the old resilient behavior would be prudent.

Fix in Cursor Fix in Web

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Incompatible with Electron v38

2 participants