fix(registry): evaluate defaultCacheDirectory lazily - #41942
Merged
Merged
Conversation
Devin Rousso (dcrousso)
requested review from
Pavel Feldman (pavelfeldman) and
Yury Semikhatsky (yury-s)
July 22, 2026 21:32
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.
| let _defaultCacheDirectory: string | undefined; | ||
|
|
||
| export const defaultRegistryDirectory = path.join(defaultCacheDirectory, 'ms-playwright'); | ||
| // Evaluated lazily so an explicit PLAYWRIGHT_BROWSERS_PATH lets Playwright run on |
There was a problem hiding this comment.
Please drop all these useless comments!
Pavel Feldman (pavelfeldman)
approved these changes
Jul 23, 2026
Pavel Feldman (pavelfeldman)
left a comment
Pavel Feldman (pavelfeldman)
left a comment
Member
There was a problem hiding this comment.
Let's strip the useless comments.
`defaultCacheDirectory` used to be computed inside the `PLAYWRIGHT_BROWSERS_PATH` branch of `registryDirectory`, so setting that variable skipped the platform check a later refactor hoisted it into an eagerly evaluated module constant, so it now throws `Unsupported platform: <platform>` at import, before `registryDirectory` consults `PLAYWRIGHT_BROWSERS_PATH` this regressed setups that point `PLAYWRIGHT_BROWSERS_PATH` at their own browsers on platforms without a default cache directory, such as Android/Termux evaluate `defaultCacheDirectory`, `defaultRegistryDirectory`, and `baseDaemonDir` lazily so an explicit `PLAYWRIGHT_BROWSERS_PATH` is honored before any default cache directory is computed `registryDirectory` still throws eagerly when no override is set, since there is nowhere to place browsers
Devin Rousso (dcrousso)
force-pushed
the
fix-41852
branch
from
July 23, 2026 17:37
a444544 to
4af292d
Compare
Contributor
Test results for "tests 1"5 flaky50308 passed, 1190 skipped Merge workflow run. |
Contributor
Test results for "MCP"1 failed 7794 passed, 1266 skipped Merge workflow run. |
Contributor
|
Hi, I'm the Playwright bot and I took a first look at the CI failures here. 🟢 The one failure is a pre-existing flake — the PR looks clear
DetailsPre-existing flake / infra
The diff is confined to browser cache-directory resolution — Triaged by the Playwright bot - agent run |
Devin Rousso (dcrousso)
merged commit Jul 23, 2026
3e14efd
into
microsoft:main
50 of 51 checks passed
This was referenced Jul 23, 2026
Merged
This was referenced Aug 7, 2026
|
Why this not updated in npm |
Mert Can Altin (mertcanaltin)
added a commit
to mertcanaltin/playwright-cli
that referenced
this pull request
Aug 23, 2026
`registry.defaultRegistryDirectory` became a function upstream in microsoft/playwright#41942 and arrived here with the roll to 1.63.0-alpha-2026-08-05. `cacheFile()` still used it as a string, so `path.join()` threw, `readCache()`/`writeCache()` swallowed the TypeError, and the update check was never cached: every single CLI invocation fetched the npm registry and re-ran the installed-skill check. Locally that is ~480ms per command instead of ~125ms. Tests always set PLAYWRIGHT_CLI_INSTALLATION_FOR_TEST, so the default branch was never exercised. Add a regression test that points HOME at a temp directory and asserts the cache file is written. Also hoist `cacheFile()` out of the try blocks so only I/O and parse failures are swallowed there, instead of masking a path-computation bug as "no cache".
Devin Rousso (dcrousso)
pushed a commit
to microsoft/playwright-cli
that referenced
this pull request
Aug 24, 2026
…le (#454) `registry.defaultRegistryDirectory` became a function upstream in microsoft/playwright#41942 and arrived here with the roll to 1.63.0-alpha-2026-08-05. `cacheFile()` still used it as a string, so `path.join()` threw, `readCache()`/`writeCache()` swallowed the TypeError, and the update check was never cached: every single CLI invocation fetched the npm registry and re-ran the installed-skill check. Locally that is ~480ms per command instead of ~125ms. Tests always set PLAYWRIGHT_CLI_INSTALLATION_FOR_TEST, so the default branch was never exercised. Add a regression test that points HOME at a temp directory and asserts the cache file is written. Also hoist `cacheFile()` out of the try blocks so only I/O and parse failures are swallowed there, instead of masking a path-computation bug as "no cache".
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
3 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.
defaultCacheDirectoryused to be computed inside thePLAYWRIGHT_BROWSERS_PATHbranch ofregistryDirectory, so setting that variable skipped the platform check#39423 hoisted it into an eagerly evaluated module constant, so it now throws
Unsupported platform: <platform>at import, beforeregistryDirectoryconsultsPLAYWRIGHT_BROWSERS_PATHthis regressed setups that point
PLAYWRIGHT_BROWSERS_PATHat their own browsers on platforms without a default cache directory, such as Android/Termuxevaluate
defaultCacheDirectory,defaultRegistryDirectory, andbaseDaemonDirlazily so an explicitPLAYWRIGHT_BROWSERS_PATHis honored before any default cache directory is computedregistryDirectorystill throws eagerly when no override is set, since there is nowhere to place browsersfixes #41852