fix(ext/node): fix 8 Node.js compat test failures#32755
Conversation
- events: handle EventTarget in listenerCount() via getEventListeners - worker_threads: accept NaN/null/undefined in cpuUsage() prevValue - process: add quic field to process.features - util: return null-prototype object from parseEnv() - net: fix autoSelectFamilyAttemptTimeout default from 250 to 500 Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Pass readableType/type options through Duplex.toWeb() to newReadableWritablePairFromDuplex() and into the underlying ReadableStream, enabling BYOB readers on byte streams. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
- streams: pass readableType/type options through Duplex.toWeb() to create byte streams for BYOB readers - fs: handle ENOTDIR in globSync getDirentSync instead of throwing Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
kajukitli
left a comment
There was a problem hiding this comment.
The PR contains a logical error in the type validation for prevValue. The NumberIsNaN() check is semantically incorrect when applied to an object - it will always return false for objects since NaN is a numeric concept. The check is redundant and misleading, as the subsequent validateObject call handles the actual type validation.
[HIGH] ext/node/polyfills/worker_threads.ts:665: Incorrect type check: NumberIsNaN(prevValue) always returns false for objects. This check is semantically wrong since prevValue is expected to be an object with user and system properties, not a number. The condition !NumberIsNaN(prevValue) is always true for any object, making it redundant and confusing.
Suggestion: Simplify the condition to just
prevValue != nulland remove theNumberIsNaNcheck, sincevalidateObjecton line 666 already handles the actual type validation.
NumberIsNaN() always returns false for objects, making the check misleading. Simplify to just `prevValue != null` since validateObject handles actual type validation. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
The NumberIsNaN check is intentional — Node.js treats NaN as "no previous value" (like null/undefined), so it must be filtered out before reaching validateObject. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
|
|
||
| let autoSelectFamilyDefault = true; | ||
| let autoSelectFamilyAttemptTimeoutDefault = 250; | ||
| let autoSelectFamilyAttemptTimeoutDefault = 500; |
There was a problem hiding this comment.
isn't 250 already the correct value here? (CMIIW)
Ref: https://github.com/nodejs/node/pull/52474/changes
There was a problem hiding this comment.
The current Node.js default is actually 500, not 250. See node_options.h: uint64_t network_family_autoselection_attempt_timeout = 500;. It was bumped from 250 to 500 after the PR you linked.
Summary
Fixes 8 Node.js compat tests that were failing after the node_test submodule update to Node.js 25.8.1 (#32705). Closes part of #32706.
events:listenerCount()now handlesEventTargetinstances by delegating togetEventListeners()worker_threads:cpuUsage()acceptsNaN/null/undefinedas "no previous value" (matching Node.js behavior)process: addquic: falsetoprocess.featuresutil:parseEnv()returns a null-prototype objectnet: fixautoSelectFamilyAttemptTimeoutdefault from 250 to 500 (matching Node.js)streams: passreadableType/typeoptions throughDuplex.toWeb()to create byte streams for BYOB readersfs: handleENOTDIRinglobSyncgetDirentSync()instead of throwingEnabled tests
test-child-process-spawn-timeout-kill-signal.jstest-child-process-fork-timeout-kill-signal.jstest-worker-cpu-usage.jstest-process-features.jstest-util-parse-env.jstest-net-autoselectfamily-attempt-timeout-default-value.jstest-stream-duplex.jstest-fs-glob.mjsTest plan
./x test-compat🤖 Generated with Claude Code