-
Notifications
You must be signed in to change notification settings - Fork 3.9k
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
feat: re-make global browser installation #1506
feat: re-make global browser installation #1506
Conversation
This patch removes the `PLAYWRIGHT_GLOBAL_INSTALL=1` variable and instead introduces a new var - `PLAYWRIGHT_BROWSER_PATHS`. You can specify `PLAYWRIGHT_BROWSER_PATHS` to affect where playwright installs browsers and where it looks for browsers.
const webkitOptions = localDownloadOptions('webkit'); | ||
if (!(await existsAsync(chromiumOptions.downloadPath))) { | ||
await downloadBrowserWithProgressBar(chromiumOptions); | ||
await protocolGenerator.generateChromiumProtocol(chromiumOptions.executablePath).catch(console.warn); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why these catches?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
- we always install all three browsers on our CI and generate protocols for them
- the
firefox-linux
github action doesn't install chromium dependencies. I'd like to keep it this way so that our github action is lean and can be easily reused - the
generateChromiumProtocol
fails if there are no chromium dependencies installed
PS: these checks were before in a form of try-catch
.
playwright.firefox._executablePath = downloadedBrowsers.ffExecutablePath; | ||
playwright.webkit._executablePath = downloadedBrowsers.wkExecutablePath; | ||
} catch (e) { | ||
if (fs.existsSync(path.join(__dirname, '.local-browsers'))) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why this? Do we still expect playwright-core to work without external setup?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this index.js
is also used when playwright is installed from github (e.g. in case of our local development)
Works great, thank you! Just a minor issue I filed: #1651 |
This patch removes the
PLAYWRIGHT_GLOBAL_INSTALL=1
variableand instead introduces a new var -
PLAYWRIGHT_BROWSERS_PATH
.You can specify
PLAYWRIGHT_BROWSERS_PATH
to affect where playwrightinstalls browsers and where it looks for browsers.
Fixes #1102