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

api: export all browsers from every package #3128

Merged
merged 1 commit into from
Jul 24, 2020
Merged

api: export all browsers from every package #3128

merged 1 commit into from
Jul 24, 2020

Conversation

dgozman
Copy link
Contributor

@dgozman dgozman commented Jul 23, 2020

This makes it easier to reason about our packages.
The only difference is what each package downloads.
When the browser is not downloaded, it will fail to launch.

Each browser gets a 'download' attribute in the browser.json file.

@mxschmitt
Copy link
Member

Relates and fixes probably #3084 and #2905

@dgozman dgozman marked this pull request as ready for review July 24, 2020 00:42
packages/installation-tests/installation-tests.sh Outdated Show resolved Hide resolved
packages/playwright-chromium/index.d.ts Outdated Show resolved Hide resolved
src/install/installer.ts Outdated Show resolved Hide resolved
packages/installation-tests/installation-tests.sh Outdated Show resolved Hide resolved
@@ -100,7 +100,7 @@ async function checkRangeAvailability(fromRevision, toRevision, stopWhenAllAvail
* @return {boolean}
*/
async function checkAndDrawRevisionAvailability(table, name, revision) {
const promises = fetcherOptions.map(platform => browserFetcher.canDownload(name, revision, platform));
const promises = fetcherOptions.map(platform => browserFetcher.canDownload({ name, revision, download: true }, platform));
Copy link
Contributor

Choose a reason for hiding this comment

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

the download: true argument to the canDownload method does look uncanny to me.

How do you feel splitting download bit from BrowserDescriptor into a separate field? It'll solve all the weird typing issues since absolute majority of clients of BrowserDescriptor object (e.g. revisionURL() and canDownload() functions) don't need download bit.

For example, browsers.json could look like this:

{
  "comment": "Do not edit this file, use utils/roll_browser.js",
  "download": ["chromium", "firefox", "webkit"],
  "browsers": [
    {
      "name": "chromium",
      "revision": "791201"
    },
    {
      "name": "firefox",
      "revision": "1140"
    },
    {
      "name": "webkit",
      "revision": "1317"
    }
  ]
}

If you like the idea but don't have time to follow-up with it, I'll send you a PR as this lands.

src/server/playwright.ts Show resolved Hide resolved
src/install/installer.ts Outdated Show resolved Hide resolved
src/install/browserFetcher.ts Show resolved Hide resolved
This makes it easier to reason about our packages.
The only difference is what each package downloads.
When the browser is not downloaded, it will fail to launch.

Each browser gets a 'download' attribute in the browser.json file.
@dgozman dgozman merged commit fd2e65b into microsoft:master Jul 24, 2020
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