-
Notifications
You must be signed in to change notification settings - Fork 9.1k
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: add channel parameter for puppeteer.launch #7389
feat: add channel parameter for puppeteer.launch #7389
Conversation
Thanks for your pull request. It looks like this may be your first contribution to a Google open source project (if not, look below for help). Before we can look at your pull request, you'll need to sign a Contributor License Agreement (CLA). 📝 Please visit https://cla.developers.google.com/ to sign. Once you've signed (or fixed any issues), please reply here with What to do if you already signed the CLAIndividual signers
Corporate signers
ℹ️ Googlers: Go here for more info. |
@googlebot I signed it! |
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 change looks good to me, thanks for your contribution!
I left some comments and maybe we should also think about whether PUPPETEER_EXECUTABLE_PATH
should take precedence over the channel or not.
Let me know what you think!
Thank you for your kind review. I considered again about the priority of channel and executablePath.
[1] is actually adopted in Playwright. This is really simple. I think [1] is better. The use-case of [2] is like below: However in [2], users can also specify MS Edge as executablePath even when This is just my observation but I didn't see the people who change the Chrome install path, and I thought [2] is useful for only a few people. IMO, simplicity is better than considering the edge case. |
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.
Looks good to me from a technical point of view now.
But please also update the documentation in docs/api.md
to explain the change.
Regarding the priority: Let's make them mutually exclusive. |
That's a good idea. I will add assertion for it. |
All (the pull request submitter and all commit authors) CLAs are signed, but one or more commits were authored or co-authored by someone other than the pull request submitter. We need to confirm that all authors are ok with their commits being contributed to this project. Please have them confirm that by leaving a comment that contains only Note to project maintainer: There may be cases where the author cannot leave a comment, or the comment is not properly detected as consent. In those cases, you can manually confirm consent of the commit author(s), and set the ℹ️ Googlers: Go here for more info. |
All (the pull request submitter and all commit authors) CLAs are signed, but one or more commits were authored or co-authored by someone other than the pull request submitter. We need to confirm that all authors are ok with their commits being contributed to this project. Please have them confirm that by leaving a comment that contains only Note to project maintainer: There may be cases where the author cannot leave a comment, or the comment is not properly detected as consent. In those cases, you can manually confirm consent of the commit author(s), and set the ℹ️ Googlers: Go here for more info. |
1 similar comment
All (the pull request submitter and all commit authors) CLAs are signed, but one or more commits were authored or co-authored by someone other than the pull request submitter. We need to confirm that all authors are ok with their commits being contributed to this project. Please have them confirm that by leaving a comment that contains only Note to project maintainer: There may be cases where the author cannot leave a comment, or the comment is not properly detected as consent. In those cases, you can manually confirm consent of the commit author(s), and set the ℹ️ Googlers: Go here for more info. |
@jschfflr Thank you for your kind review. |
Sure :) |
All (the pull request submitter and all commit authors) CLAs are signed, but one or more commits were authored or co-authored by someone other than the pull request submitter. We need to confirm that all authors are ok with their commits being contributed to this project. Please have them confirm that by leaving a comment that contains only Note to project maintainer: There may be cases where the author cannot leave a comment, or the comment is not properly detected as consent. In those cases, you can manually confirm consent of the commit author(s), and set the ℹ️ Googlers: Go here for more info. |
Add 'channel' parameter for Puppeteer.launch Issues: #7340
Add check for detected Chrome install path.
channel should be always respected, even when executablePath is specified.
Rebased. |
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 change LGTM now!
Your change landed 🎉 |
This change adds a new `channel` parameter to `puppeteer.launch`. When specified, Puppeteer will search for the locally installed release channel of Google Chrome and use it to launch. Available values are `chrome`, `chrome-beta`, `chrome-canary`, `chrome-dev`. This parameter is mutually exclusive with `executablePath`.
Any particular reason this feature only checks Playwright appears to look in both (as well as in |
Add 'channel' parameter for Puppeteer.launch
Resolves #7340
Google Chrome is often used for using puppeteer as a browser automation tool,
We have to specify executablePath in such case. However we can't share the code among Windows/macOS users, because executablePath is different.
microsoft/playwright already introduced the
channel
parameter for solving this problem.Let's provide the similar functionality for Puppeteer users.