perf(ext/node): convert more polyfills to lazy-loaded JS#33882
Conversation
Convert 6 more node:* modules from eagerly-loaded ESM to lazy-loaded IIFE scripts with thin ESM wrappers: - node:wasi - node:trace_events - node:punycode - node:domain - node:perf_hooks - node:vm Also adds `trace_events` to the DENO_SUPPORTED_BUILTIN_NODE_MODULES list, which was previously missing.
fibibot
left a comment
There was a problem hiding this comment.
Continuation of the lazy-load lineage. Substance follows the established pattern from #33881 / #33876 — looks right for the identity contract this time around.
What I verified
The vm.js IIFE return shape (representative of all 6 conversions):
return {
default: { Module, Script, ..., compileFunction, measureMemory },
Module, Script, ..., compileFunction, measureMemory, // top-level for named imports
};vm_esm.js wrapper:
const mod = core.loadExtScript("ext:deno_node/vm.js");
export const { Module, Script, ... } = mod;
export default mod.default;01_require.js consumer:
const vm = core.loadExtScript("ext:deno_node/vm.js").default;So imported.default (ESM) and require("vm") (CJS) both resolve to mod.default — same object, identity preserved. Should pass process.getBuiltinModule("vm") === imported.default (the test that broke #33875). ✓
Trace events fix: trace_events was missing from DENO_SUPPORTED_BUILTIN_NODE_MODULES — adding it cleans up the spec test for unknown_builtin_node_module (5 lines removed from check.out). Worth a sanity check that tools/lint.js won't flag this as an unrelated change in the same PR, but it's a minor cleanup.
CI
56 success, 10 pending, 0 fail. All the substantive shards (build/lint/test_specs/test_node_compat) haven't reported yet. Will re-confirm once they land. The 6 modules being converted (wasi, trace_events, punycode, domain, perf_hooks, vm) are exactly what test-process-get-builtin.mjs tests — that test will be the load-bearing signal for whether identity contract is preserved.
Not APPROVE-ing yet per the CI-must-be-green rule; substance looks right but the test that broke #33875 is the one I want to see green.
The lsp_completions_node_specifier test has a hardcoded list of node builtins. Adding trace_events to DENO_SUPPORTED_BUILTIN_NODE_MODULES requires updating this test to include it.
fibibot
left a comment
There was a problem hiding this comment.
Substance is correct
The test-process-get-builtin.mjs family (which broke #33875 with the same conversion shape) passes on all 4 platforms × debug + release. All 14 test node_compat (1/3, 2/3) shards green — confirming the identity contract is preserved through this lazy-load conversion. ✓
One unrelated-Windows fail
test node_compat (3/3) debug windows-aarch64 and windows-x86_64 both fail with one test:
---- node_compat::parallel::test-child-process-send-returns-boolean.js ----
error: Uncaught TypeError: This handle type cannot be sent
const rv1 = s.send('one', handle, assert.ifError);
This test was enabled in #33869 (which I approved earlier today) and the failure surface is child.send(msg, handle) IPC-handle passing on Windows — completely unrelated to this PR's scope (lazy-loading wasi / trace_events / punycode / domain / perf_hooks / vm). Looking at the established pattern in tests/node_compat/config.jsonc, similar IPC-handle tests already have "windows": false (e.g., test-child-process-send-keep-open.js with reason "net.Socket IPC handle passing uses SCM_RIGHTS, not yet implemented on Windows"). The new boolean-return test should likely have the same gate.
This fail probably blocks merge of the boolean-return test's PR (#33869, already merged) more than it blocks this conversion PR. Worth a 1-line follow-up to add the Windows skip.
CI
131 success, 3 fail (1 = ci status rollup, 2 = the same Windows test), 2 skipping. All non-Windows shards green; Windows-specific test is the only blocker.
Not APPROVE-ing per the strict CI-must-be-green rule, but substantively this PR is APPROVE-quality — the conversion is correct, identity preserved, and the failing test is provably unrelated. If the maintainer adds "windows": false to the boolean-return test (either in this PR's diff or a follow-up that lands first), I can clear with APPROVE.
Continues the lazy-loading conversion started in #33876, #33881, and #33882. - Convert 3 `node:*` modules from eagerly-loaded ESM to lazy-loaded IIFE scripts with thin ESM wrappers: `node:querystring`, `node:async_hooks`, `node:diagnostics_channel` - Update import sites in `01_require.js`, `url.ts`, `net.ts`, `http2.ts`, `dgram.ts`, `_http_agent.js`, `_http_client.js`, `_http_server.js`, and `internal_binding/http_parser.ts` to use `core.loadExtScript()` instead of ESM imports - Also fixes stale `node:perf_hooks` ESM imports in `_http_client.js` and `_http_server.js` that were missed in #33882
Summary
Continues the lazy-loading conversion started in #33876 and #33881.
node:wasi,node:trace_events,node:punycode,node:domain,node:perf_hooks,node:vm01_require.js,http2.ts, andrepl.tsto usecore.loadExtScript()instead of ESM importstrace_eventstoDENO_SUPPORTED_BUILTIN_NODE_MODULES(was previously missing from the builtin list)Test plan
./x test-spec unknown_builtin_node_module— updated expectations, passes./x test-spec node_builtin— 11 tests pass./x test-spec domain— domain timer tests passcargo test unit_node::vm— passescargo test unit_node::domain— passescargo test unit_node::perf_hooks— passescargo test unit_node::punycode— passescargo test unit_node::http2— passes (import change)cargo test unit_node::repl— passes (import change)importand CJSrequire()