fix: install-browser now actually installs browsers#314
Open
leonletto wants to merge 1 commit intomicrosoft:mainfrom
Open
fix: install-browser now actually installs browsers#314leonletto wants to merge 1 commit intomicrosoft:mainfrom
leonletto wants to merge 1 commit intomicrosoft:mainfrom
Conversation
`install-browser` had no dedicated case in the bundled program.js switch statement, so it fell through to the default path that tries to connect to a running browser session. The daemon then returned the same "not installed" message, creating a recursive error loop. Fix: intercept `install-browser` in the entry point (playwright-cli.js) before delegating to the bundled Playwright code, and use playwright-core's registry to install the exact browser version matching the CLI's bundled Playwright.
Author
|
@microsoft-github-policy-service agree |
Author
@microsoft-github-policy-service agree |
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
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.
What's broken
playwright-cli install-browser webkittells you to runplaywright-cli install-browser webkit. That's the whole bug — the install command points at itself.The bundled
program.jshas no case forinstall-browserin its switch statement. It falls through to the default path, which tries to connect to a running browser session and forward the command to the daemon. The daemon checks for the browser, doesn't find the right version, and tells you to run the command you just ran.The fix
Intercept
install-browserinplaywright-cli.jsbefore it ever reaches the bundled Playwright code. Useplaywright-core's registry directly to find and install the exact browser version matching the CLI's bundled Playwright.Both syntaxes work:
Error messages are useful:
Why fix it here and not in the Playwright monorepo
The real source lives in the Playwright monorepo. The proper long-term fix is adding an
install-browsercase to the switch inprogram.jsthere. That's a reasonable follow-up.But this fix is self-contained, lives entirely in the one file this repo controls (
playwright-cli.js), and solves the problem now. No waiting on upstream. If you'd rather I submit the fix to the Playwright monorepo instead, happy to do that.Tests
Four new tests covering the happy path (positional arg,
--browserflag) and error cases (unknown browser, missing name). All pass alongside the existing test.Test plan
playwright-cli install-browser webkit— installs successfullyplaywright-cli install-browser --browser=webkit— installs successfullyplaywright-cli install-browser bogus— clear error, exit 1playwright-cli install-browser(no arg) — clear error, exit 1playwright-cli --help— still worksplaywright-cli open/ other commands — unaffectednpm test— all 5 tests pass