-
Notifications
You must be signed in to change notification settings - Fork 108
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
base: main
Are you sure you want to change the base?
Conversation
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.
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?
if (options.ignoreDefaultArgs) { | ||
launchParams.ignoreDefaultArgs = options.ignoreDefaultArgs; | ||
} |
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.
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.
if (options.ignoreDefaultArgs) { | |
launchParams.ignoreDefaultArgs = options.ignoreDefaultArgs; | |
} | |
const ignoreDefaultArgs = options.ignoreDefaultArgs; delete options.ignoreDefaultArgs; | |
if (ignoreDefaultArgs) { | |
launchParams.ignoreDefaultArgs = ignoreDefaultArgs; | |
} |
it do | ||
expect(pdf_text_content).to eq 'Testgetriebene Ent‐ wicklung ist eine Methode.' | ||
end |
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.
Prefer the inline form where possible
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.' } |
I encountered an issue when using German hyphens on Linux.
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:
Unfortunately,
ignoreDefaultArgs
are not passed via Grover.This PR fixes it.