fix(test): use synchronous module customization hooks#40891
Open
dgozman wants to merge 6 commits into
Open
Conversation
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
Node 26 deprecates `module.register()` in favour of the synchronous `module.registerHooks()` API. Prefer the synchronous hooks when available: they run in-process, need no preflight round-trip and no port transport. Fall back to the asynchronous loader on older Node, and expose `PLAYWRIGHT_FORCE_ASYNC_LOADER` as an opt-out. Fixes: microsoft#40868
The synchronous module customization hooks do not intercept the
`require.resolve(id, { paths })` form, so `require.resolve()` of a
TypeScript reporter (or any extensionless TypeScript specifier) failed.
Register dummy loaders for our extensions so the default CommonJS
resolver considers them; the `load` hook still does the transformation.
Fixes: microsoft#40868
The synchronous module customization hooks intercept require(), so the require() of package.json in folderIsModule() was recorded as a dependency of the test file being loaded, confusing --only-changed. Read and parse package.json directly instead.
The synchronous module customization hooks' resolve hook pre-populates the global deps collector, so collectCJSDependencies, which uses the collector itself as its visited set, would skip transitive deps under any child the hook had already added. To make matters worse, Node short- circuits the resolve hook for already-resolved (parent_dir, request) pairs via its relativeResolveCache, so transitive deps shared between sibling test files are silently missed. Walk the CJS module tree into a fresh set first, then merge into the global collector.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
When loadReporter(null, …) runs before loadConfig — as the test server does on runGlobalSetup — installTransformIfNeeded() was called before registerESMLoader() got a chance to set _needsPreflightAndPirates. That took the sync-only branch and skipped pirates for good, since transformInstalled became true. On Node where the async loader is picked (Node 20, or PLAYWRIGHT_FORCE_ASYNC_LOADER=1), pirates never intercepted require() afterwards, so a CJS globalSetup.js with mixed `import`/`module.exports` syntax got routed through Node 23+'s require(esm) auto-detection and threw "module is not defined". Split the install into a light step (extension shortcuts + source map support) and a pirates step. Run the light step unconditionally — when pirates installs later it overrides the same extensions. Have setNeedsPreflightAndPirates() retroactively install pirates if the light step already ran. Fixes: microsoft#40868
Merge esmLoaderHost.ts into transform.ts so the transform layer owns the full ESM loader lifecycle: register Node hooks, push state to the worker thread, pair local deps collection with the worker notification, and pull the cache back. transform.installTransformIfNeeded() now calls registerESMLoader() itself, so any requireOrImport entry settles the sync-vs-async decision before deciding whether to install pirates — removing the ordering bug at the source. setSingleTSConfig and setTransformConfig are async and push changes to the loader thread inline. registerESMLoader seeds the worker with the current snapshot when it falls back to the async loader. loadConfig no longer needs configureESMLoader/configureESMLoaderTransformConfig — the setters do it themselves. loaderChannel is the single source of truth for "we have a worker": installTransformIfNeeded checks it to pick the CJS-hooks path, requireOrImport checks it for the preflight.
Contributor
Test results for "MCP"7181 passed, 1113 skipped Merge workflow run. |
Contributor
Test results for "tests 1"3 flaky42061 passed, 850 skipped Merge workflow run. |
pavelfeldman
approved these changes
May 21, 2026
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
module.registerHooks()over the deprecatedmodule.register()(deprecated as of Node 26). The synchronous loader runs in-process: no.esm.preflightround-trip and no port transport, and it interceptsrequire()so the pirates-based CommonJS hook stands down.registerHooksnor reliablerequire(esm));PLAYWRIGHT_FORCE_ASYNC_LOADER=1opts back in to async on newer Node.package.jsonas a test dep, CJS deps collection, and lazy pirates install when the async path is taken after another module triggered transform setup first.esmLoaderHostintotransform.tsso the transform layer owns the full ESM-loader lifecycle (register hooks, push state to the worker thread, pull cache back).setSingleTSConfig/setTransformConfigare now async and propagate to the loader thread reactively, soconfigureESMLoader*calls are no longer needed inloadConfig.loaderChannelis the single source of truth for "we are on the async path".Test matrix
Targeted suites (
loader.spec.ts+global-setup.spec.ts, 70 tests) on every supported Node, exercising both loader paths where applicable:registerHooks)The original VSCode-extension codegen failure (
globalSetup.jswith mixedimport/module.exportssyntax) reproduced under Node 20 /PLAYWRIGHT_FORCE_ASYNC_LOADER=1againstplaywright-vscodelocally and now passes.Fixes #40868