Skip to content

fix: fix page_options for PlaywrightBrowserPlugin #796

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.

Sign up for GitHub

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

Jump to bottom
Merged
janbuchar merged 3 commits into from
Dec 16, 2024
Merged

fix: fix page_options for PlaywrightBrowserPlugin #796

janbuchar merged 3 commits into from
Dec 16, 2024

Conversation

@Mantisus
Copy link
Collaborator

@Mantisus Mantisus commented Dec 9, 2024

Description

  • fix page_options for PlaywrightBrowserPlugin

Issues

Testing

  • Add test for check workability page_options in PlaywrightBrowserPlugin

Checklist

  • CI passed

@Mantisus
Copy link
Collaborator Author

Mantisus commented Dec 9, 2024

My initial thought was to change the name of page_options to context_options. However, I think this might confuse users who are not so familiar with playwright. Especially considering that the parameters taken by new_context and new_page in playwright are identical.

So I've updated the docstring to explicitly state that these parameters are passed when the context is created.

@vdusek vdusek requested review from Pijukatel and vdusek December 10, 2024 08:32
Pijukatel
Pijukatel reviewed Dec 10, 2024
View reviewed changes
@Pijukatel Pijukatel self-requested a review December 10, 2024 17:08
@Mantisus Mantisus self-assigned this Dec 12, 2024
@janbuchar janbuchar merged commit bd3bdd4 into apify:master Dec 16, 2024
23 checks passed
vdusek
vdusek reviewed Dec 16, 2024
View reviewed changes
src/crawlee/browsers/_playwright_browser_controller.py
sec_ch_ua_headers = self._header_generator.get_sec_ch_ua_headers(browser_type=self.browser_type)
user_agent_header = self._header_generator.get_user_agent_header(browser_type=self.browser_type)
extra_http_headers = dict(common_headers | sec_ch_ua_headers | user_agent_header)
user_agent = user_agent_header.get('User-Agent')
Copy link
Collaborator

@vdusek vdusek Dec 16, 2024

Choose a reason for hiding this comment

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

This was here to replace the default "headless"-something user agent. Why is it removed? Does it mean we're now using the default (headless) user agent?

Copy link
Member

@B4nan B4nan Dec 17, 2024

Choose a reason for hiding this comment

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

We should have a test to double-check the default UA is not leaking, it burned as several times in the past in the JS version.

I think we have it tested on other places too, found this one now

https://github.com/apify/crawlee/blob/fdd895e94e848c7e5545a2e239f24130d341db61/test/core/crawlers/playwright_crawler.test.ts#L88

Copy link
Collaborator Author

@Mantisus Mantisus Dec 17, 2024

Choose a reason for hiding this comment

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

We have a similar test

https://github.com/apify/crawlee-python/blob/master/tests/unit/playwright_crawler/test_playwright_crawler.py#L106

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@B4nan B4nan B4nan left review comments

@vdusek vdusek vdusek left review comments

@Pijukatel Pijukatel Pijukatel approved these changes

Assignees

@Mantisus Mantisus

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

Replace page_options with context_options for PlaywrightBrowserPlugin

5 participants

@Mantisus @B4nan @vdusek @Pijukatel @janbuchar