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.

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

devops: remake downloading logic #1419

Merged
merged 12 commits into from
Mar 19, 2020

Conversation

aslushnikov
Copy link
Collaborator

@aslushnikov aslushnikov commented Mar 18, 2020

This patch:

With this patch, we take the following approach towards managing browser downloads:

  • playwright-core doesn't download any browsers. In playwright-core, playwright.chromium.executablePath() returns null (same for firefox and webkit).
  • clients of playwright-core (e.g. playwright and others) download browsers one way or another.
    They can then configure playwright with executable paths and re-export the playwright object to their clients.
  • playwright, playwright-firefox, playwright-chromium and playwright-webkit download
    browsers. Once browsers are downloaded, their executable paths are saved to a .downloaded-browsers.json file. This file is read in playwright/index.js to configure browser executable paths and re-export the API.
  • special case is install-from-github.js that also cleans up old browsers.

This patch:
- removes `browserType.downloadBrowserIfNeeded()` method. The method
  turned out to be ill-behaving and could not be used as we'd like to.
- adds a `browserType.setExecutablePath` method to set a browser
  exectuable.

Now, all clients of `playwright-core` should take care of downloading
a browser one way or another and setting the downloaded path with the
`browserType.setExecutablePath` method. The playwright object can then
be returned to client.
Copy link
Contributor

@dgozman dgozman left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good!

packages/playwright-chromium/index.js Outdated Show resolved Hide resolved
src/server/browserType.ts Outdated Show resolved Hide resolved
src/server/chromium.ts Outdated Show resolved Hide resolved
utils/check_availability.js Show resolved Hide resolved
install-from-github.js Outdated Show resolved Hide resolved
src/server/browserFetcher.ts Show resolved Hide resolved
docs/api.md Outdated Show resolved Hide resolved
aslushnikov and others added 9 commits March 18, 2020 20:08
Co-Authored-By: Joel Einbinder <joel.einbinder@gmail.com>
Co-Authored-By: Joel Einbinder <joel.einbinder@gmail.com>
Co-Authored-By: Joel Einbinder <joel.einbinder@gmail.com>
Co-Authored-By: Joel Einbinder <joel.einbinder@gmail.com>
@aslushnikov aslushnikov merged commit f5ecbff into microsoft:master Mar 19, 2020
@aslushnikov aslushnikov deleted the eeeeeeeeeeeee branch March 19, 2020 18:43
@hugomrdias
Copy link
Contributor

Am i seeing this wrong or you still downloading into node_modules?
can't we download into the system cache folder with env-paths module ? like i do here https://github.com/hugomrdias/playwright-test/blob/20d784a5d52af734e873c0ce0033660fe00206a3/src/utils.js#L172-L216 with a special case for CI where the cache folder can sometimes have special perms ?
this would make playwright binaries take less space from users hard drives, @electron/get module does exactly the same thing i do in playwright-test.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants