fix: only promisify raw natives so re-evaluation does not stack wrappers#25
Open
erunion wants to merge 2 commits into
Open
fix: only promisify raw natives so re-evaluation does not stack wrappers#25erunion wants to merge 2 commits into
erunion wants to merge 2 commits into
Conversation
The native binding is a process-wide singleton, but lib/nodegit.js assigns promisified wrappers onto its shared classes. Anything that evaluates the JS more than once per process (e.g. Jest, which gives every test file a fresh module registry inside a long-lived worker) re-promisified the methods each time: after the first evaluation the methods are promise-returning wrappers (util.promisify output, or extension wrappers such as lookupWrapper), and wrapping those again makes Node 22 emit a DEP0174 deprecation warning on every call and leaks a pending promise per extra layer. Guard the generated promisify helper so it only ever wraps the raw callback-style natives (which stringify as "[native code]") and returns anything already promise-based untouched. Add a regression test that evaluates lib/nodegit.js twice in one process and asserts no DEP0174 warnings are emitted. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
jboyens
approved these changes
Jul 15, 2026
flinehan
approved these changes
Jul 15, 2026
domharrington
approved these changes
Jul 16, 2026
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Problem
The native binding is a process-wide singleton, but the generated
lib/nodegit.jsassigns promisified wrappers onto the shared native classes (_Tree.lookup = promisify(_Tree.lookup), etc.). Anything that evaluates the package's JS more than once in the same process — notably Jest, which gives every test file a fresh module registry inside a long-lived shard worker — stacks another wrapper layer per evaluation. On Node 22, every call through autil.promisifylayer whose inner function already returns a Promise emits:In a downstream CI run this produced thousands of warnings per shard (in escalating bursts as workers accumulated layers), drowning out real test failures. Each extra layer also leaks a pending promise per call.
Why a simple "already wrapped" flag isn't enough
Tagging
promisify's output and skipping flagged functions does not fix this (verified against a local reproduction — all 156 warnings remained). After the template promisifies a method, the manual extensions replace it with their own plain promise-returning function (e.g.Commit.lookup = lookupWrapper(Commit)inlib/commit.js). The next evaluation promisifies that unflagged extension wrapper — which is why the--trace-deprecationstacks point atlib/utils/lookup_wrapper.js:27.Fix
Guard the generated
promisifyhelper so it only ever wraps the raw callback-style natives, which stringify as[native code]. Anything else (a promisify wrapper or an extension wrapper installed by a prior evaluation) is already promise-based and is returned untouched:Regression test
test/tests/reload.jsrunstest/utils/reload_fixture.jsin a child process (so the suite's own module cache is untouched). The fixture evaluateslib/nodegit.jstwice in one process — fresh JS module registry, shared native binding, exactly the Jest scenario — exercisesRepository.open/Commit.lookupincluding thelookupWrapperpath, and fails on any DEP0174 warning. It exits 1 against the old promisify and 0 with this fix.Verification
npm run lintpasses; regeneratedlib/nodegit.jsmatches the template output.test/id_rsakeys, andinit.defaultBranch=mainvs tests expectingmaster); none related to this change.🤖 Generated with Claude Code