-
Notifications
You must be signed in to change notification settings - Fork 508
feat: add chrome
BrowserType
for PlaywrightCrawler
to use the Chrome browser
#1487
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
base: master
Are you sure you want to change the base?
Conversation
ValueError, match=r'Cannot use `use_chrome` with `Configuration.default_browser_path` or `executable_path` set.' | ||
): | ||
PlaywrightBrowserPlugin( | ||
browser_type='chromium', |
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.
To me, this seems a little strange from the user perspective. It is defining one type of browser in one argument and then overriding it in another argument, which is valid only for a specific value of the first argument. I would prefer to just add browser_type=chrome
and connect any logic to such a specific browser_type internally.
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.
Since browser_type
in Crawlee is a parameter that directly corresponds to a similar parameter in Playwright, I think adding a new option to it will confuse users familiar with Playwright.
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.
I think that users familiar with Playwright are probably OK with the current state of defining channel
in browser_launch_options
- as that is the Playwright way of using Chrome and offers the best flexibility without us needing to change anything.
Experienced Playwright users aside, when considering anyone else, I think it is not a very intuitive way to select Chrome with this change.
Just consider the following snippets:
To use Firefox:
PlaywrightCrawler(browser_type="firefox")
But to use Chrome
PlaywrightCrawler(browser_type="chromium", use_chrome=True)
I think that most users would expect just this instead:
PlaywrightCrawler(browser_type="chrome")
Anyway, I do not have a very strong opinion about this, but I would suggest introducing new arguments only if we see big improvements in readability, which I do not think is the case now.
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.
Maybe @vdusek, @janbuchar have some ideas about which approach would be best to choose?
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.
I agree with @Pijukatel
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.
Updated
use_chrome
to use the Chrome browserchrome
BrowserType
for PlaywrightCrawler
to use the Chrome browser
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.
A few comments
Co-authored-by: Vlada Dusek <[email protected]>
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.
LGTM
We have quite a lot of "fix" open PRs, so let's wait for them at first, and release one more patch release. This will be part of v1.1, together with Redis.
Description
chrome
BrowserType
forPlaywrightCrawler
to use the Chrome browserIssues