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

fix: introduce ignoreDefaultArgs into launchParams #246

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

tomaswitek
Copy link

I encountered an issue when using German hyphens on Linux.

Snímek obrazovky 2024-07-11 v 13 34 02

The following images were generated with identical code on different environments.
The left side is Linux, and the right side is MacOS.
Here is a bug report on the Chromium website: https://issues.chromium.org/issues/40933841
There is a workaround for Puppeteer:

puppeteer.launch({
  ignoreDefaultArgs: ['--disable-component-update'],
})

Unfortunately,ignoreDefaultArgs are not passed via Grover.
This PR fixes it.

Copy link
Contributor

@abrom abrom left a comment

Choose a reason for hiding this comment

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

Thank you for the PR @tomaswitek !

The direction is looking good but I've flagged a few things I think could do with some tweaks.

I believe the OptionsFixer#fix_array_options! will also need to be updated to include this new option. Although AFAICT the expected types are slightly different in that the launch args should be an array of strings, whereas the ignoreDefaultArgs can be either an array of strings, or a boolean (false) to ignore ALL defaults. It may need a special handler to deal with that properly.. although I don't think so.

It would also be great to see an update to the README to explain how this can be used. I'd suggest this would apply everywhere the launch_args option is described. I'd also suggest referencing back to the puppeteer documentation: https://pptr.dev/api/puppeteer.launchoptions

Thoughts?

Comment on lines +78 to +80
if (options.ignoreDefaultArgs) {
launchParams.ignoreDefaultArgs = options.ignoreDefaultArgs;
}
Copy link
Contributor

Choose a reason for hiding this comment

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

The options parameter is eventually passed through to the conversion method. To avoid these sorts of other options bleeding through we need to follow the pattern seen elsewhere to store the option in a variable and then delete it (from options) before using it.

Suggested change
if (options.ignoreDefaultArgs) {
launchParams.ignoreDefaultArgs = options.ignoreDefaultArgs;
}
const ignoreDefaultArgs = options.ignoreDefaultArgs; delete options.ignoreDefaultArgs;
if (ignoreDefaultArgs) {
launchParams.ignoreDefaultArgs = ignoreDefaultArgs;
}

Comment on lines +446 to +448
it do
expect(pdf_text_content).to eq 'Testgetriebene Ent‐ wicklung ist eine Methode.'
end
Copy link
Contributor

Choose a reason for hiding this comment

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

Prefer the inline form where possible

Suggested change
it do
expect(pdf_text_content).to eq 'Testgetriebene Ent‐ wicklung ist eine Methode.'
end
it { expect(pdf_text_content).to eq 'Testgetriebene Ent‐ wicklung ist eine Methode.' }

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.

2 participants