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

Puppeteer: Upgrade to v22 #556

Merged
merged 9 commits into from
Aug 23, 2024
Merged

Conversation

evictorero
Copy link
Contributor

@evictorero evictorero commented Aug 15, 2024

  • Upgrade Puppeteer to the next major with support.
  • Bump axios

We are doing this to keep using a maintained version and remove some old, unmaintained dependencies.

Fix #460

@@ -23,7 +23,7 @@ export class ReusableBrowser extends Browser {
try {
page = await this.withTimingMetrics<puppeteer.Page>(async () => {
this.validateImageOptions(options);
context = await this.browser.createIncognitoBrowserContext();
context = await this.browser.createBrowserContext();
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This function was renamed on version 22

@AgnesToulet
Copy link
Contributor

This new Puppeteer version also enables the new-headless mode by default that doesn't work for Kubernetes users. If we want to merge this quickly, we should use headless=chrome-headless-shell here, else I'll spend some time investigating the impact of this.

Also, I noticed an issue with the stat panel in the test screenshots, I'm looking into this.

@@ -16,9 +16,6 @@ async function main() {
const env = Object.assign({}, process.env);
const command = argv._[0];

// See https://github.com/grafana/grafana-image-renderer/issues/460
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This is no longer needed. We are using the shell value instead of the new headless mode.

Copy link
Contributor

Choose a reason for hiding this comment

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

@@ -137,7 +137,7 @@ export class Browser {
launcherOptions.executablePath = this.config.chromeBin;
}

launcherOptions.headless = !this.config.headed;
launcherOptions.headless = !this.config.headed ? "shell" : false;
Copy link
Contributor Author

Choose a reason for hiding this comment

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

shell is for the old headed mode

Copy link
Contributor

@AgnesToulet AgnesToulet left a comment

Choose a reason for hiding this comment

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

LGTM! This looks good and works fine 👍

There is a bug in the tests, it's why the screenshot with the error wasn't caught. I'll work on a separate PR to fix this.

Note: There is also this change which updates a setting in PDF. We may want to make this configurable as this can increase the file size but this would also be in another PR.

@@ -16,9 +16,6 @@ async function main() {
const env = Object.assign({}, process.env);
const command = argv._[0];

// See https://github.com/grafana/grafana-image-renderer/issues/460
Copy link
Contributor

Choose a reason for hiding this comment

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

src/browser/browser.ts Outdated Show resolved Hide resolved
@evictorero evictorero self-assigned this Aug 22, 2024
@evictorero evictorero merged commit 27928cf into master Aug 23, 2024
4 checks passed
@evictorero evictorero deleted the evictorero/upgrade-puppeteer-22 branch August 23, 2024 15:40
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
Status: 🚀 Done
Development

Successfully merging this pull request may close these issues.

Browser: Switch to new headless mode
3 participants