fix(ext/node): add emitExperimentalWarning/pendingDeprecate to internal/util, support modifyPrototype option in util.deprecate#33660
Conversation
nathanwhitbot
left a comment
There was a problem hiding this comment.
Two things:
Scope: First commit c001540 is the open #33650 work, not part of this PR's stated content. Same pattern @nathanwhit flagged on #33653 / #33658 — drop or rebase past it. Files ext/node/polyfills/fs.ts (+36/-4) and the watch-related entries in config.jsonc shouldn't be in this PR's diff.
Code:
emitExperimentalWarning (internal/util.mjs): direct port of lib/internal/util.js#L324-332. LGTM — string shape and ExperimentalWarning category match.
util.deprecate (util.ts): modifyPrototype option matches lib/util.js#L547-549 + lib/internal/util.js#L178,200-216. Nesting both setPrototypeOf and the length define under the gate is correct.
pendingDeprecate (internal/util.mjs): the gate process.execArgv?.includes("--pending-deprecation") is dead code in Deno — process.execArgv is hardcoded to [] in ext/node/polyfills/process.ts:121, so the check is always false and the warning never fires. Node also honors NODE_PENDING_DEPRECATION env var via getOptionValue, which this also misses. Either wire up the actual flag/env source (e.g. Deno.env.get("NODE_PENDING_DEPRECATION")) or note the gap in a comment so it doesn't silently regress later.
…al/util, support modifyPrototype option in util.deprecate
f3e42b7 to
c82630e
Compare
|
Force-pushed |
nathanwhitbot
left a comment
There was a problem hiding this comment.
Self-review of c82630e8a (post-cleanup HEAD):
emitExperimentalWarning — internal/util.mjs:
Matches lib/internal/util.js:324 exactly:
experimentalWarningsSafeSet keyed byfeature, early-return if seen.- Message format
${feature} is an experimental feature and might change at any timewith optionalmessagePrefixconcatenation. process.emitWarning(msg, "ExperimentalWarning", code, ctor)arity matches Node. ✓
pendingDeprecate — internal/util.mjs:
Length preservation via ObjectDefineProperty(deprecated, "length", { ...ObjectGetOwnPropertyDescriptor(fn, "length") }) matches Node lib/internal/util.js:167. ✓
Two minor divergences from Node's reference implementation (which uses getDeprecationWarningEmitter):
-
No-
codepath emits on every call instead of once. Node'sgetDeprecationWarningEmitterreturns a closure with an internalwarnedboolean (lib/internal/util.js:117-118); the first matching call setswarned = trueso subsequent calls are no-ops even whencodeis undefined. Our impl skips this and unconditionally callsprocess.emitWarningon every invocation whencode === undefined. The bundled test (test-util-deprecate.js) only exercises pendingDeprecate's length-preservation branch, so CI is green — but the divergence is observable if a caller ever usespendingDeprecate(fn, msg)(no code) and the function is called repeatedly. -
process.execArgv?.includes("--pending-deprecation")vs Node'sgetOptionValue('--pending-deprecation').execArgvonly reflects flags from the command line;getOptionValueconsults the resolved option which can also come fromNODE_OPTIONS. Probably fine in practice for Deno's runtime model, but worth a follow-up if Deno's NODE_OPTIONS handling lands.
Public util.deprecate(fn, msg, code, { modifyPrototype }) — util.ts:
- Signature matches the public API in
lib/util.js:546:function deprecate(fn, msg, code, { modifyPrototype } = {}). ✓ - Default
modifyPrototype = truepreserves existing callers;modifyPrototype: falseskips theObjectSetPrototypeOf,deprecated.prototype = fn.prototype, and length-defineProperty block — matching Node'slib/internal/util.js:200-216. - Note: with
modifyPrototype: false,lengthis not preserved (the defineProperty is inside the modifyPrototype branch). This matches Node's behavior — the test verifiesutil.deprecate(fn).length === fn.lengthonly on the default path. - Existing
let warned = falseper-call closure preserved on the public path, so the first test block (fn(); fn(); count: 1) passes.
Test enrollment: test-util-deprecate.js and test-util-emit-experimental-warning.js are alphabetically positioned correctly (after test-util-convert-signal-to-exit-code.mjs and before test-util-getcallsites.js). ✓
Holding to COMMENT until CI completes.
fibibot
left a comment
There was a problem hiding this comment.
LGTM. The three pieces line up with what parallel/test-util-deprecate.js and parallel/test-util-emit-experimental-warning.js actually exercise:
emitExperimentalWarning(feature, messagePrefix, code, ctor)— once-per-featureSetgate, optionalmessagePrefix, forwards toprocess.emitWarningwith"ExperimentalWarning". Direct port of Node'slib/internal/util.js. ✓pendingDeprecate(fn, msg, code)— wrapsfn, gates on--pending-deprecation+!noDeprecation, once-per-code via a separatependingCodesWarnedSet,ReflectApplyfor forwarding, and (this is the key part the test asserts)ObjectDefineProperty(deprecated, "length", { ...ObjectGetOwnPropertyDescriptor(fn, "length") })to preservefn.lengthon the wrapper. The test runsassert.strictEqual(internalUtil.pendingDeprecate(fn).length, fn.length)across 10 differentfnshapes — including arrow functions, rest params, and zero-arg — so the descriptor copy is necessary, not justdeprecated.length = fn.length(which can fail for arrow fns whose length is non-writable).util.deprecatemodifyPrototypeopt + length copy on the default path: same length-preservation logic added insideif (modifyPrototype), soutil.deprecate(fn).length === fn.lengthholds. ✓
Test enrollment alphabetically positioned correctly between test-util-deprecate-invalid-code.js and test-util-getcallsites-preparestacktrace.js. ✓
CI fully green: 133 SUCCESS, 0 FAILURE, 2 SKIPPED. All test node_compat and test unit_node shards pass.
Two small notes (non-blocking)
-
pendingDeprecateis effectively inert in real Deno usage. The gate isprocess.execArgv?.includes("--pending-deprecation"), butext/node/polyfills/process.ts:120declaresexecArgv: string[] = []as a constant empty array — Deno never populates it the way Node does. So even when a user runsdeno --pending-deprecation(if that flag exists), the warning won't fire. This is a pre-existing Deno↔Node gap, not introduced here, and the tests don't run with--pending-deprecationso the gate never trips during compat testing — but worth knowing. -
modifyPrototypeoption lives on the publicutil.deprecate. In Node, that option is oninternal/util.deprecateonly; the publicutil.deprecateis 3-arg. The compat tests don't passmodifyPrototype: falseagainst the public API, so the divergence is invisible to the test suite. If this is intentional (Deno often exposes more of the internal surface for compat), no action needed; if not, moving the 4-arg form tointernal/util.mjsand keeping the public one at 3 args matches Node more precisely.
Summary
Three small alignment fixes to bring
internal/utilandutil.deprecatecloser to Node.js, plus the two compat tests they unblock:internalUtil.emitExperimentalWarning(feature, messagePrefix, code, ctor)matchinglib/internal/util.js— emits anExperimentalWarningonce per feature.internalUtil.pendingDeprecate(fn, msg, code)— wrapsfnand forwards aDeprecationWarningonly when--pending-deprecationis set, while preservingfn.lengthon the wrapper.util.deprecate(fn, msg, code, { modifyPrototype })now accepts the options object 4th argument and skips the prototype-chain / length copy whenmodifyPrototype: false. The default path now also preservesfn.lengthon the deprecated wrapper.Enables:
parallel/test-util-deprecate.jsparallel/test-util-emit-experimental-warning.jsTest plan
cargo test --test node_compat -- test-util-deprecate test-util-emit-experimental-warning(all pass)cargo test --test node_compaton Linux — only pre-existing flaky failures (test-process-threadCpuUsage-worker-threads.js,test-dgram-send-cb-quelches-error.js,test-child-process-uid-gid.js,test-net-autoselectfamily.js,test-fs-promises-file-handle-readFile.js).