feat(cli): add command to list installed browsers #35240
Conversation
|
@microsoft-github-policy-service agree |
This comment has been minimized.
This comment has been minimized.
Test results for "tests 1"1 failed 3 flaky38784 passed, 808 skipped Merge workflow run. |
dgozman
changed the title
| @@ -199,6 +199,29 @@ Examples: | |||
| - $ install chrome firefox | |||
| Install custom browsers, supports ${suggestedBrowsersToInstall()}.`); | |||
|
|
|||
| program | |||
| .command('list') | |||
There was a problem hiding this comment.
I see that issues talks about npx playwright install --list instead of npx playwright list. Generally, we'd prefer everything about browsers to be under install/uninstall.
There was a problem hiding this comment.
Sorry, that's my fault. I should have shared my thought process in the PR description.
Initially, I started implementing the --list option. Then, I discovered that it is possible to list installations for all instances of Playwright, and I realized the --all flag could be useful. I felt that adding --all and --list to the install command would be excessive, especially since --all only works in combination with --list.
That's why I decided to implement standalone command list
This approach also follows the same pattern as the remove command:
• npx playwright remove (--all)
• npx playwright list (--all)
If you still prefer --list and --all flags I can change it.
| // All new applications have a marker file right away. | ||
| (browserName !== 'firefox' && browserName !== 'chromium' && browserName !== 'webkit'); | ||
| if (!shouldHaveMarkerFile || (await existsAsync(browserDirectoryToMarkerFilePath(usedBrowserPath)))) | ||
|
|
There was a problem hiding this comment.
Instead of copying pretty much half of this method, perhaps we can extract the "step 1" from this function into a separate one and reuse it?
There was a problem hiding this comment.
I'm a bit lost. I extracted a part of the code that verifies the marker file into a standalone function _validateMarkerFile. I did this to reuse the same code in the remove and list functions.
What do you mean by saying 'step 1'?
| .action(async (options: { all?: boolean }) => { | ||
| await registry.list(!!options.all).then(browsersInfo => { | ||
| for (const info of browsersInfo) { | ||
| const whichInstanceLog = ` (${info.currentInstance ? 'CURRENT' : 'OTHER'})`; |
There was a problem hiding this comment.
I kinda find CURRENT/OTHER being very much in my face. Can we tone it down somehow? Perhaps not show "other" at all, because we print the target directory anyway, and only mark current as (current) when --all is not passed?
Add a new CLI command to list all browser installations managed by Playwright. Fixes #34183
This command shows browsers installed by the current Playwright instance
npx playwright listTo list browsers installed by all Playwright instances, use the
--allflag:npx playwright list --all