fix: update better-sqlite3 patch to match PROPERTY_HOLDER macro#1
fix: update better-sqlite3 patch to match PROPERTY_HOLDER macro#1
Conversation
The upstream better-sqlite3 now uses the PROPERTY_HOLDER(info) macro instead of raw info.This() in NODE_GETTER accessors. Update the patch targets to match the new source so the HolderV2() fix applies correctly. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
heath-s
requested review from
astyfx and
Copilot
and removed request for
astyfx
There was a problem hiding this comment.
Pull request overview
Updates the better-sqlite3 Electron patch script so it can still apply the HolderV2() patch after upstream changed NODE_GETTER accessors to use PROPERTY_HOLDER(info) instead of info.This().
Changes:
- Update patch “from” match strings to target
Unwrap<...>(PROPERTY_HOLDER(info))for Statement/Database getters. - Update patch “to” replacement strings to
Unwrap<...>(info.HolderV2())to keep the patch working with the new upstream source.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| signature: "NODE_GETTER(Statement::JS_busy) {", | ||
| from: "Statement* stmt = Unwrap<Statement>(info.This());", | ||
| to: "Statement* stmt = Unwrap<Statement>(info.HolderV2());", | ||
| from: "Unwrap<Statement>(PROPERTY_HOLDER(info))", | ||
| to: "Unwrap<Statement>(info.HolderV2())", | ||
| }, |
There was a problem hiding this comment.
The new from/to patterns are now short substrings (e.g. Unwrap<Database>(info.HolderV2())). In patchScopedSourceBlock, the idempotency check returns early when the block contains to, even if it also still contains from, so a block that happens to include this substring elsewhere could incorrectly skip patching. Consider either (a) making from/to more specific/unique (full statement/line context), or (b) changing patchScopedSourceBlock to check/replace from first and only treat it as already patched when from is absent and to is present.
| filePath: "node_modules/better-sqlite3/src/objects/database.cpp", | ||
| signature: "NODE_GETTER(Database::JS_open) {", | ||
| from: "info.GetReturnValue().Set(Unwrap<Database>(info.This())->open);", | ||
| to: "info.GetReturnValue().Set(Unwrap<Database>(info.HolderV2())->open);", | ||
| from: "Unwrap<Database>(PROPERTY_HOLDER(info))", | ||
| to: "Unwrap<Database>(info.HolderV2())", | ||
| }, |
There was a problem hiding this comment.
This PR changes the patch targets to match PROPERTY_HOLDER(info), but the existing bun tests for patchScopedSourceBlock only cover the older info.This() patterns. Please add/update tests to exercise the new PROPERTY_HOLDER(info) → info.HolderV2() replacement so CI will catch upstream source changes that break this patch.
better-sqlite3 upstream changed getter C++ source from direct info.This() calls to a PROPERTY_HOLDER(info) macro. Update patch from/to strings to match the new source shape so the HolderV2() fix applies correctly on all current versions. Absorbs fix/better-sqlite3-patch-property-holder (PR #1). Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
|
Absorbed into PR #3 (fix: harden better-sqlite3 Electron ABI compatibility and install workflow). The PROPERTY_HOLDER patch strings and updated tests are now in the worktree-release-0.0.18 branch. |
Summary
info.This()대신PROPERTY_HOLDER(info)매크로를 사용하도록 변경됨Test plan
bun install후 패치 스크립트가 정상 실행되는지 확인🤖 Generated with Claude Code