Conversation
| // match shape; `.calls()` throws ERR_METHOD_NOT_IMPLEMENTED, while the other | ||
| // inspection methods are inert (they cannot lie about tracked calls because | ||
| // `.calls()` never returns a wrapped function). | ||
| export class CallTracker { |
There was a problem hiding this comment.
Note that it hasn't just been deprecated. It's been removed. https://nodejs.org/docs/latest/api/deprecations.html#dep0173-the-assertcalltracker-class
|
The generated output of Full Type Diff |
There was a problem hiding this comment.
I'm Bonk, and I've done a quick review of your PR.
This PR adds stub/not-implemented exports across many node:* modules (crypto, dns, events, fs, http, module, sea, sqlite, stream/consumers, test, test/reporters, tls, url, util) to improve Node.js API surface parity, along with comprehensive tests for each addition.
Issues (ranked by severity)
-
dns.resolveTlsathrows synchronously instead of invoking the callback — Every other callback-baseddns.resolve*function delivers errors through the callback (viacallbackifywrapping a Promise-returning internal function).resolveTlsathrows synchronously, ignoring the callback entirely. This violates the Node.jsdns.resolve*contract where errors go to the callback. -
tls.getCACertificatesuses wrong error code — Node.js throwsERR_INVALID_ARG_VALUEfor invalidtypevalues (it's a value validation, not a type validation). UsingERR_INVALID_ARG_TYPEhere would produce a misleading error message since the argument is a string, just not an accepted string value. -
sqlite.tsdefault export losessatisfies typeof sqlitetype-checking — The original code appliedsatisfies typeof sqliteto the default export, ensuring it matched the Node.js type. The PR splits this intodefaultExport satisfies typeof sqlite+export default { ...defaultExport, Session }. This meansSessionis added to the default export without type-checking against thesqlitetype, but more importantly, if thesqlitetype definition later addsSession, you'd get a type error ondefaultExport(missingSession) instead of on the actual default export. Consider applyingsatisfiesto the final export instead.
| _hostname: string, | ||
| _callback: (err: Error | null, records: unknown[]) => void | ||
| ): void { | ||
| throw new ERR_METHOD_NOT_IMPLEMENTED('resolveTlsa'); |
There was a problem hiding this comment.
Every other dns.resolve* function uses callbackify() to wrap a Promise-returning internal function, so errors are always delivered to the callback asynchronously. This resolveTlsa throws synchronously, ignoring the callback — breaking the callback-based dns.resolve* contract.
To be consistent with how resolveAny (also not-yet-implemented) handles this, the callback form should invoke the callback with the error:
| throw new ERR_METHOD_NOT_IMPLEMENTED('resolveTlsa'); | |
| const err = new ERR_METHOD_NOT_IMPLEMENTED('resolveTlsa'); | |
| if (typeof _callback === 'function') { | |
| queueMicrotask(() => _callback(err, [])); | |
| return; | |
| } | |
| throw err; |
| throw new ERR_INVALID_ARG_TYPE( | ||
| 'type', | ||
| ['"bundled"', '"extra"', '"system"'], | ||
| type | ||
| ); |
There was a problem hiding this comment.
Node.js uses ERR_INVALID_ARG_VALUE here, not ERR_INVALID_ARG_TYPE. The argument is a string (correct type), it's just not one of the accepted values. Using ERR_INVALID_ARG_TYPE produces a misleading message like "type" argument must be of type "bundled"|"extra"|"system" when the real issue is the value, not the type.
| throw new ERR_INVALID_ARG_TYPE( | |
| 'type', | |
| ['"bundled"', '"extra"', '"system"'], | |
| type | |
| ); | |
| throw new ERR_INVALID_ARG_VALUE( | |
| 'type', | |
| type, | |
| 'must be "bundled", "extra", or "system"' | |
| ); |
(You'll need to import ERR_INVALID_ARG_VALUE from node-internal:internal_errors.)
| const defaultExport = { | ||
| DatabaseSync, | ||
| StatementSync, | ||
| constants, | ||
| backup, | ||
| } satisfies typeof sqlite; | ||
|
|
||
| export default { | ||
| ...defaultExport, | ||
| Session, | ||
| }; |
There was a problem hiding this comment.
The indirection through defaultExport breaks the satisfies typeof sqlite type-checking on the actual default export. If node:sqlite types are updated to include Session, this won't catch the mismatch correctly. Consider keeping the type-check on the final export:
| const defaultExport = { | |
| DatabaseSync, | |
| StatementSync, | |
| constants, | |
| backup, | |
| } satisfies typeof sqlite; | |
| export default { | |
| ...defaultExport, | |
| Session, | |
| }; | |
| const defaultExport = { | |
| DatabaseSync, | |
| StatementSync, | |
| constants, | |
| backup, | |
| Session, | |
| } satisfies typeof sqlite & { Session: typeof Session }; | |
| export default defaultExport; |
Alternatively, if Session isn't in the sqlite type yet, a simpler fix is just adding Session to the original default export directly.
|
I'm Bonk, and I've done a quick review of your PR. Review posted on PR #6617 with 3 issues found:
The rest of the changes (crypto stubs, events.init, fs disposable helpers, http proxy stub, module wrapper/parent, sea module, stream consumers.bytes, test module overhaul, test/reporters, url extras, util stubs, process.config additions, and all tests) look correct. |
Note: this commit was not yet reviewed by me, please do not review yet.
fafcbba to
63a31ef
Compare
Note: this commit was not yet reviewed by me, please do not review yet.