Skip to content

Conversation

@Mantisus
Copy link
Collaborator

@Mantisus Mantisus commented Oct 15, 2025

Description

  • add chrome BrowserType for PlaywrightCrawler to use the Chrome browser

Issues

@Mantisus Mantisus self-assigned this Oct 15, 2025
@Mantisus Mantisus requested review from Pijukatel and vdusek October 15, 2025 07:57
ValueError, match=r'Cannot use `use_chrome` with `Configuration.default_browser_path` or `executable_path` set.'
):
PlaywrightBrowserPlugin(
browser_type='chromium',
Copy link
Collaborator

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.

Copy link
Collaborator Author

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.

Copy link
Collaborator

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.

Copy link
Collaborator Author

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?

Copy link
Collaborator

Choose a reason for hiding this comment

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

I agree with @Pijukatel

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Updated

@Mantisus Mantisus marked this pull request as draft October 17, 2025 12:25
@Mantisus Mantisus marked this pull request as ready for review October 17, 2025 14:27
@Mantisus Mantisus requested a review from Pijukatel October 17, 2025 14:27
@Mantisus Mantisus changed the title feat: add use_chrome to use the Chrome browser feat: add chrome BrowserType for PlaywrightCrawler to use the Chrome browser Oct 17, 2025
Copy link
Collaborator

@vdusek vdusek left a 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 <v.dusek96@gmail.com>
@Mantisus Mantisus requested a review from vdusek October 20, 2025 22:31
Copy link
Collaborator

@vdusek vdusek left a 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.

Copy link
Collaborator

@vdusek vdusek left a comment

Choose a reason for hiding this comment

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

LGTM now

@vdusek vdusek merged commit b06937b into apify:master Oct 29, 2025
19 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

3 participants