Skip to content

fix: fix page_options for PlaywrightBrowserPlugin#796

Merged
janbuchar merged 3 commits intoapify:masterfrom
Mantisus:fix-playwright-bp
Dec 16, 2024
Merged

fix: fix page_options for PlaywrightBrowserPlugin#796
janbuchar merged 3 commits intoapify:masterfrom
Mantisus:fix-playwright-bp

Conversation

@Mantisus
Copy link
Copy Markdown
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
Copy Markdown
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
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
Copy Markdown
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
Copy Markdown
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
Copy Markdown
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