Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

[BUG] browsers are eventually removed when installed with Playwright CLI #5902

Closed
aslushnikov opened this issue Mar 22, 2021 · 0 comments · Fixed by #5904
Closed

[BUG] browsers are eventually removed when installed with Playwright CLI #5902

aslushnikov opened this issue Mar 22, 2021 · 0 comments · Fixed by #5904
Assignees
@aslushnikov

Comments

@aslushnikov
Copy link
Collaborator

Repro:

In the first package:

  1. Install npm install playwright-chromium@v1.9.0
  2. Install firefox additionally with npx playwright install firefox

In the second package:

  1. Install npm install playwright@v1.8.0

Notice that Firefox is getting removed.

This bug is critical and manifests itself when working with downloadable channels.
@aslushnikov aslushnikov self-assigned this Mar 22, 2021
aslushnikov added a commit to aslushnikov/playwright that referenced this issue Mar 22, 2021
@aslushnikov
fix(installer): retain browsers installed via Playwrigth CLI
6ef1c1e 
Browser registry is responsible for 3 things:
1. Remove downloaded browsers if there are no packages that refer to them
2. Install default browsers needed for the current package
3. Install browsers on-demand when used through Playwright CLI

Currently, registry relies on a single "download" field in `browsers.json`
to carry both (1) and (2). However, browsers in (3) are marked as
`download: false` so that they aren't installed automatically in (2), so
auto-remove procedure in (3) removes them on subsequent installation.

One possible approach to fix this would be modifying package's `browsers.json` to
change `download: false` to `true` when browsers are installed with
Playwright CLI. This approach was explored here:
microsoft@bc04a51

We decided against this since we have a history of issues related to
package modifications after NPM installation. This breaks all
sorts of yarn/npm caching mechanisms.

Instead, this patch is a two-step refactor:
- remove the "download" field in `browsers.json`. Now, all registries
(including old ones from previously-released versions) will retain any
browsers that are mentioned in the `browsers.json`.
- add a new flag "installByDefault", that is **only used** for default
installation.

With this change, the registry tasks are done like this:
- (1) auto-removal: if browser has a back reference, it is retained,
otherwise it is removed from registry
- (2) default installation: use only `installByDefault` to carry default installations
- (3) CLI installation: simply installs browsers. Since we retain
everythings that's referenced in (1), browsers aren't removed.

Fixes microsoft#5902
@aslushnikov aslushnikov mentioned this issue Mar 22, 2021
Merged
aslushnikov added a commit that referenced this issue Mar 22, 2021
@aslushnikov
fix(installer): retain browsers installed via Playwrigth CLI (#5904)
2064d27 
Browser registry is responsible for 3 things:
1. Remove downloaded browsers if there are no packages that refer to them
2. Install default browsers needed for the current package
3. Install browsers on-demand when used through Playwright CLI

Currently, registry relies on a single "download" field in `browsers.json`
to carry both (1) and (2). However, browsers in (3) are marked as
`download: false` so that they aren't installed automatically in (2), so
auto-remove procedure in (1) removes them on subsequent installation.

One possible approach to fix this would be modifying package's `browsers.json` to
change `download: false` to `true` when browsers are installed with
Playwright CLI. This approach was explored here:
bc04a51

We decided against this since we have a history of issues related to
package modifications after NPM installation. This breaks all
sorts of yarn/npm caching mechanisms.

Instead, this patch is a two-step refactor:
- remove the "download" field in `browsers.json`. Now, all registries
(including old ones from previously-released versions) will retain any
browsers that are mentioned in the `browsers.json`.
- add a new flag "installByDefault", that is **only used** for default
installation.

With this change, the registry tasks are done like this:
- (1) auto-removal: if browser has a back reference, it is retained,
otherwise it is removed from registry
- (2) default installation: use only `installByDefault` to carry default installations
- (3) CLI installation: simply installs browsers. Since we retain
everythings that's referenced in (1), browsers aren't removed.

Fixes #5902
aslushnikov added a commit to aslushnikov/playwright that referenced this issue Mar 22, 2021
@aslushnikov
fix(installer): retain browsers installed via Playwrigth CLI (microso…
a425549 
…ft#5904)

Browser registry is responsible for 3 things:
1. Remove downloaded browsers if there are no packages that refer to them
2. Install default browsers needed for the current package
3. Install browsers on-demand when used through Playwright CLI

Currently, registry relies on a single "download" field in `browsers.json`
to carry both (1) and (2). However, browsers in (3) are marked as
`download: false` so that they aren't installed automatically in (2), so
auto-remove procedure in (1) removes them on subsequent installation.

One possible approach to fix this would be modifying package's `browsers.json` to
change `download: false` to `true` when browsers are installed with
Playwright CLI. This approach was explored here:
microsoft@bc04a51

We decided against this since we have a history of issues related to
package modifications after NPM installation. This breaks all
sorts of yarn/npm caching mechanisms.

Instead, this patch is a two-step refactor:
- remove the "download" field in `browsers.json`. Now, all registries
(including old ones from previously-released versions) will retain any
browsers that are mentioned in the `browsers.json`.
- add a new flag "installByDefault", that is **only used** for default
installation.

With this change, the registry tasks are done like this:
- (1) auto-removal: if browser has a back reference, it is retained,
otherwise it is removed from registry
- (2) default installation: use only `installByDefault` to carry default installations
- (3) CLI installation: simply installs browsers. Since we retain
everythings that's referenced in (1), browsers aren't removed.

Fixes microsoft#5902
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees

@aslushnikov aslushnikov

Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging a pull request may close this issue.

fix(installer): retain browsers installed via Playwrigth CLI aslushnikov/playwright
1 participant
@aslushnikov