From d98dd57bf2ef8d950f103c71e037292ee15d99d5 Mon Sep 17 00:00:00 2001 From: Jon Ursenbach Date: Wed, 15 Jul 2026 09:03:34 -0700 Subject: [PATCH 1/2] fix: only promisify raw natives so re-evaluation does not stack wrappers 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 --- generate/templates/templates/nodegit.js | 15 +++++- test/tests/reload.js | 28 +++++++++++ test/utils/reload_fixture.js | 63 +++++++++++++++++++++++++ 3 files changed, 105 insertions(+), 1 deletion(-) create mode 100644 test/tests/reload.js create mode 100644 test/utils/reload_fixture.js diff --git a/generate/templates/templates/nodegit.js b/generate/templates/templates/nodegit.js index c5fd769eb..ee90a63a9 100644 --- a/generate/templates/templates/nodegit.js +++ b/generate/templates/templates/nodegit.js @@ -9,7 +9,20 @@ try { var rawApi = require("node-gyp-build")(path.join(__dirname, "..")); -var promisify = fn => fn && util.promisify(fn); // jshint ignore:line +// The native binding is a process-wide singleton, so when this file is evaluated more than +// once in the same process (e.g. Jest evaluates it once per test file within a worker) the +// methods on the shared classes are no longer the raw callback-style natives: they are the +// promise-returning wrappers installed by an earlier evaluation (util.promisify below, or an +// extension such as lookupWrapper). Re-promisifying those makes every call emit a DEP0174 +// deprecation warning and leak a pending promise per extra layer, so only ever wrap the raw +// natives — anything else is already promise-based and is returned untouched. +var promisify = fn => { // jshint ignore:line + if (typeof fn !== "function" || + !Function.prototype.toString.call(fn).includes("[native code]")) { + return fn; + } + return util.promisify(fn); +}; // For disccussion on why `cloneDeep` is required, see: // https://github.com/facebook/jest/issues/3552 diff --git a/test/tests/reload.js b/test/tests/reload.js new file mode 100644 index 000000000..27a15c31a --- /dev/null +++ b/test/tests/reload.js @@ -0,0 +1,28 @@ +var path = require("path"); +var childProcess = require("child_process"); +var local = path.join.bind(path, __dirname); + +describe("Reload", function() { + // Jest (and anything else with a per-file module registry) evaluates lib/nodegit.js once + // per test file inside a long-lived process while the native binding stays a process-wide + // singleton. Re-evaluations must not re-promisify the shared native methods: each extra + // util.promisify layer emits a DEP0174 deprecation warning on every call and leaks a + // pending promise. Run in a child process so this suite's own module cache is untouched. + it("does not re-promisify shared natives when re-evaluated in one process", + function(done) { + this.timeout(30000); + + var fixture = local("../utils/reload_fixture.js"); + childProcess.execFile(process.execPath, [fixture], + function(error, stdout, stderr) { + if (error) { + done(new Error( + "reload fixture exited with code " + error.code + "\n" + stdout + stderr + )); + return; + } + + done(); + }); + }); +}); diff --git a/test/utils/reload_fixture.js b/test/utils/reload_fixture.js new file mode 100644 index 000000000..4362de1b1 --- /dev/null +++ b/test/utils/reload_fixture.js @@ -0,0 +1,63 @@ +// Evaluates lib/nodegit.js twice in one process, the way Jest does (fresh JS module +// registry per test file, shared native binding), then exercises methods that earlier +// versions would re-promisify. Exits non-zero if any DEP0174 warning is emitted. +var path = require("path"); + +var libDir = path.resolve(__dirname, "..", "..", "lib"); +var nodegitPath = path.join(libDir, "nodegit.js"); +var projectRepoPath = path.resolve(__dirname, "..", ".."); + +var warningCount = 0; +process.on("warning", function(warning) { + if (warning.code === "DEP0174") { + warningCount++; + } +}); + +// The native .node binding stays cached (it is a process-wide singleton no matter what); +// only the package's JS modules are re-evaluated. +function purgePackageJsFromCache() { + Object.keys(require.cache).forEach(function(key) { + var isNativeModule = key.slice(-5) === ".node"; + var isPackageJs = key.indexOf(libDir) === 0 || + key.indexOf("node-gyp-build") !== -1; + + if (!isNativeModule && isPackageJs) { + delete require.cache[key]; + } + }); +} + +var first = require(nodegitPath); + +purgePackageJsFromCache(); +var second = require(nodegitPath); + +if (second === first) { + console.error("fixture failed to re-evaluate nodegit"); + process.exit(2); +} + +// A promisify layer wrapped around an already promise-returning function emits DEP0174 at +// call time, which is what we detect. Commit.lookup goes through the lookupWrapper +// extension, the layer that historically got re-promisified. +second.Repository.open(projectRepoPath) + .then(function(repo) { + return repo.getHeadCommit().then(function(commit) { + return second.Commit.lookup(repo, commit.id()); + }); + }) + .catch(function(error) { + console.error("fixture could not exercise lookups: " + error); + process.exit(2); + }) + .then(function() { + setTimeout(function() { + if (warningCount > 0) { + console.error("DEP0174 warnings after re-evaluation: " + warningCount); + process.exit(1); + } + + process.exit(0); + }, 100); + }); From 0b79657afe24948001e186e5ffa6a7196200a247 Mon Sep 17 00:00:00 2001 From: Jon Ursenbach Date: Wed, 15 Jul 2026 13:46:18 -0700 Subject: [PATCH 2/2] chore: make promisify guard comments framework-agnostic Co-Authored-By: Claude Fable 5 --- generate/templates/templates/nodegit.js | 12 ++++++------ test/tests/reload.js | 10 +++++----- test/utils/reload_fixture.js | 6 +++--- 3 files changed, 14 insertions(+), 14 deletions(-) diff --git a/generate/templates/templates/nodegit.js b/generate/templates/templates/nodegit.js index ee90a63a9..b21e0f6a2 100644 --- a/generate/templates/templates/nodegit.js +++ b/generate/templates/templates/nodegit.js @@ -10,12 +10,12 @@ try { var rawApi = require("node-gyp-build")(path.join(__dirname, "..")); // The native binding is a process-wide singleton, so when this file is evaluated more than -// once in the same process (e.g. Jest evaluates it once per test file within a worker) the -// methods on the shared classes are no longer the raw callback-style natives: they are the -// promise-returning wrappers installed by an earlier evaluation (util.promisify below, or an -// extension such as lookupWrapper). Re-promisifying those makes every call emit a DEP0174 -// deprecation warning and leak a pending promise per extra layer, so only ever wrap the raw -// natives — anything else is already promise-based and is returned untouched. +// once in the same process the methods on the shared classes are no longer the raw +// callback-style natives: they are the promise-returning wrappers installed by an earlier +// evaluation (util.promisify below, or an extension such as lookupWrapper). Re-promisifying +// those makes every call emit a DEP0174 deprecation warning and leak a pending promise per +// extra layer, so only ever wrap the raw natives — anything else is already promise-based +// and is returned untouched. var promisify = fn => { // jshint ignore:line if (typeof fn !== "function" || !Function.prototype.toString.call(fn).includes("[native code]")) { diff --git a/test/tests/reload.js b/test/tests/reload.js index 27a15c31a..7f289d3b8 100644 --- a/test/tests/reload.js +++ b/test/tests/reload.js @@ -3,11 +3,11 @@ var childProcess = require("child_process"); var local = path.join.bind(path, __dirname); describe("Reload", function() { - // Jest (and anything else with a per-file module registry) evaluates lib/nodegit.js once - // per test file inside a long-lived process while the native binding stays a process-wide - // singleton. Re-evaluations must not re-promisify the shared native methods: each extra - // util.promisify layer emits a DEP0174 deprecation warning on every call and leaks a - // pending promise. Run in a child process so this suite's own module cache is untouched. + // lib/nodegit.js can be evaluated more than once in a single process while the native + // binding stays a process-wide singleton. Re-evaluations must not re-promisify the shared + // native methods: each extra util.promisify layer emits a DEP0174 deprecation warning on + // every call and leaks a pending promise. Run in a child process so this suite's own + // module cache is untouched. it("does not re-promisify shared natives when re-evaluated in one process", function(done) { this.timeout(30000); diff --git a/test/utils/reload_fixture.js b/test/utils/reload_fixture.js index 4362de1b1..4cca61bd7 100644 --- a/test/utils/reload_fixture.js +++ b/test/utils/reload_fixture.js @@ -1,6 +1,6 @@ -// Evaluates lib/nodegit.js twice in one process, the way Jest does (fresh JS module -// registry per test file, shared native binding), then exercises methods that earlier -// versions would re-promisify. Exits non-zero if any DEP0174 warning is emitted. +// Evaluates lib/nodegit.js twice in one process (fresh JS module registry, shared native +// binding), then exercises methods that earlier versions would re-promisify. Exits +// non-zero if any DEP0174 deprecation warning is emitted. var path = require("path"); var libDir = path.resolve(__dirname, "..", "..", "lib");