fix: Expose impersonate flag on HTTP crawlers.#1957
Conversation
Pijukatel
left a comment
There was a problem hiding this comment.
Hi, I think it would be better to do just a documentation change and keep the current implementation.
I wrote the reasons into the issue, as the current wording of the issue is asking for a code change.
vdusek
left a comment
There was a problem hiding this comment.
Two comments from my side.
And regarding this:
Hi, I think it would be better to do just a documentation change and keep the current implementation.
I wrote the reasons into the issue, as the current wording of the issue is asking for a code change.
Exposing high-level convenience arguments on crawlers, which configure the underlying components, is a Crawlee design choice. And we follow this all over the place - crawlers (and Actor in SDK) already act as partial facades over the components they compose. A few examples:
PlaywrightCrawler-headless,browser_type,browser_launch_options,use_incognito_pages,user_data_dir, and similar BrowserPool/plugin internals directly on the crawler. With your approach, there should be onlybrowser_pool.StagehandCrawlerfollows the same pattern.BasicCrawlerhasuse_session_pool: boolnext to thesession_poolobject, which is the same shape asimpersonate.
So impersonate is not introducing a new pattern. It follows the same approach we already use for browsers, sessions, proxy configuration, concurrency, and more.
This design decision was made a long time ago, and we should be consistent and follow it, rather than diverging from it. And AFAIK @B4nan is a strong proponent of this approach.
This creates the ugly edge case
HttpCrawler(impersonate=False, http_client=...).
We should simply validate the argument combination and raise an error when it is invalid, like in other places.
...
TLDR; A convenience flag (with guard and documentation) is consistent with the rest of Crawlee. It also keeps the simple case simple: users can turn off impersonation without needing to know what an HTTP client is.
Those internal components usually have more arguments than what is exposed on the Crawler level, and many internal component arguments remain unexposed (which is fine). I do not think we have sufficient evidence to say that this specific internal component argument is so useful for the general user base that it deserves to be exposed on the Crawler level. JS tooling has been around for a while. Was anyone missing such an argument? We should exercise restraint when exposing those convenient arguments. The more we have, the harder it is to understand the code. |
|
Regarding the But since we’re already taking the approach of "giving the user a simple configuration option", this PR is fully consistent with that approach. |
Description
impersonateflag on the HTTP crawlers (HttpCrawler,BeautifulSoupCrawler,ParselCrawler) to turn browser impersonation on or off in the defaultImpitHttpClient. The flag applies only to the default client; if a customhttp_clientis passed, it is ignored.docs/guides/http_headers.mdx) with a runnable example.Issues
Testing
impersonateflag for all HTTP crawlers.