Skip to content

Add a bunch of missing nodejs APIs#6617

Draft
danlapid wants to merge 1 commit intomainfrom
dlapid/moreNodeCompat2
Draft

Add a bunch of missing nodejs APIs#6617
danlapid wants to merge 1 commit intomainfrom
dlapid/moreNodeCompat2

Conversation

@danlapid
Copy link
Copy Markdown
Collaborator

Note: this commit was not yet reviewed by me, please do not review yet.

// 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 {
Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Note that it hasn't just been deprecated. It's been removed. https://nodejs.org/docs/latest/api/deprecations.html#dep0173-the-assertcalltracker-class

@github-actions
Copy link
Copy Markdown

The generated output of @cloudflare/workers-types has been changed by this PR. If this is intentional, run just generate-types to update the snapshot. Alternatively, you can download the full generated types:

Full Type Diff

Copy link
Copy Markdown
Contributor

@ask-bonk ask-bonk Bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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)

  1. dns.resolveTlsa throws synchronously instead of invoking the callback — Every other callback-based dns.resolve* function delivers errors through the callback (via callbackify wrapping a Promise-returning internal function). resolveTlsa throws synchronously, ignoring the callback entirely. This violates the Node.js dns.resolve* contract where errors go to the callback.

  2. tls.getCACertificates uses wrong error code — Node.js throws ERR_INVALID_ARG_VALUE for invalid type values (it's a value validation, not a type validation). Using ERR_INVALID_ARG_TYPE here would produce a misleading error message since the argument is a string, just not an accepted string value.

  3. sqlite.ts default export loses satisfies typeof sqlite type-checking — The original code applied satisfies typeof sqlite to the default export, ensuring it matched the Node.js type. The PR splits this into defaultExport satisfies typeof sqlite + export default { ...defaultExport, Session }. This means Session is added to the default export without type-checking against the sqlite type, but more importantly, if the sqlite type definition later adds Session, you'd get a type error on defaultExport (missing Session) instead of on the actual default export. Consider applying satisfies to the final export instead.

Comment thread src/node/dns.ts
_hostname: string,
_callback: (err: Error | null, records: unknown[]) => void
): void {
throw new ERR_METHOD_NOT_IMPLEMENTED('resolveTlsa');
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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:

Suggested change
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;

Comment thread src/node/tls.ts
Comment on lines +65 to +69
throw new ERR_INVALID_ARG_TYPE(
'type',
['"bundled"', '"extra"', '"system"'],
type
);
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Suggested change
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.)

Comment thread src/node/sqlite.ts
Comment on lines +124 to +134
const defaultExport = {
DatabaseSync,
StatementSync,
constants,
backup,
} satisfies typeof sqlite;

export default {
...defaultExport,
Session,
};
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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:

Suggested change
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.

@ask-bonk
Copy link
Copy Markdown
Contributor

ask-bonk Bot commented Apr 20, 2026

I'm Bonk, and I've done a quick review of your PR.

Review posted on PR #6617 with 3 issues found:

  1. dns.resolveTlsa throws synchronously instead of invoking the callback — All other dns.resolve* callback functions deliver errors asynchronously via the callback (using callbackify). This one throws synchronously, ignoring the callback parameter. Suggestion provided.

  2. tls.getCACertificates uses ERR_INVALID_ARG_TYPE instead of ERR_INVALID_ARG_VALUE — The argument is a string (correct type), just not an accepted value. Node.js uses ERR_INVALID_ARG_VALUE for this case. Suggestion provided.

  3. sqlite.ts default export loses satisfies type-checking — The refactor splits the export so Session bypasses the satisfies typeof sqlite check. Suggestion provided.

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.

github run

Note: this commit was not yet reviewed by me, please do not review yet.
@danlapid danlapid force-pushed the dlapid/moreNodeCompat2 branch from fafcbba to 63a31ef Compare April 20, 2026 16:17
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants