feat: allow using other HTTP clients

[janbuchar]

• closes HTTP client switching #2659

See https://gist.github.com/janbuchar/3a4724927de2c3a0bb16c46bb5940236 for an example curl-impersonate client.

The following got-scraping options were ignored (they will still work, but they're not part of the new interface):

• decompress,
• resolveBodyOnly,
• allowGetBody,
• dnsLookup,
• dnsCache,
• dnsLookupIpVersion,
• retry,
• hooks,
• parseJson,
• stringifyJson,
• request,
• cache,
• cacheOptions,
• http2
• https
• agent
• localAddress
• createConnection
• pagination
• setHost
• maxHeaderSize
• methodRewriting
• enableUnixSockets
• context

[github-actions]

⚠️ Pull Request Tookit has failed!

Pull request is neither linked to an issue or epic nor labeled as adhoc!

[github-actions]

⚠️ Pull Request Tookit has failed!

Pull request is neither linked to an issue or epic nor labeled as adhoc!

[barjin]

So if I understand this correctly, we cannot get rid of got-scraping just yet because of the breaking change in BasicCrawlingContext.sendRequest, right?

Does it mean we have to model the BaseHttpClient to follow the got-scraping interfaces, though? It would make more sense to me to make BaseHttpClient independent (or have it follow some well-known API like fetch) and only care for the got-scraping interface where it matters, i.e. sendRequest.

In sendRequest, we would then translate from the got-scraping interface to the independent fetch API (and, in case of GotScrapingClient, later from fetch API back to the got-scraping calls).

[barjin]

Also, a completely unrelated thought / nit, but what file and directory casing convention are we following in Crawlee? It's already messy in the current codebase, but seeing snake_case files together with kebab-case in this PR made me think about it once again 😅

[janbuchar]

So if I understand this correctly, we cannot get rid of got-scraping just yet because of the breaking change in BasicCrawlingContext.sendRequest, right?

Well, yeah, since got is part of our public API, we cannot completely remove it 🤷. We'll see what our default http client for 4.0 will be.

Does it mean we have to model the BaseHttpClient to follow the got-scraping interfaces, though? It would make more sense to me to make BaseHttpClient independent (or have it follow some well-known API like fetch) and only care for the got-scraping interface where it matters, i.e. sendRequest.

Not really, but I'd prefer to implicitly support got stuff that won't be part of the http client interface (e.g., parseJson or cacheOptions, and this way, we can do it kinda easily with index signatures on http request/response. It would be doable if we used a fetch-like interface, but a bit more difficult.

Also, a completely unrelated thought / nit, but what file and directory casing convention are we following in Crawlee? It's already messy in the current codebase, but seeing snake_case files together with kebab-case in this PR made me think about it once again 😅

Well, I didn't notice any, so I just look at the nearest files and do my best effort.

[B4nan]

Historically things were snake case, but I believe nobody from the current team is in favor of that (I never like this). I would go with kebab case for anything new, and unify in v4 maybe (or sooner, I don't think it has any effect downstream, we don't allow deep imports anywhere).

[B4nan]

few comments from my end

[janbuchar]

This is a feature, not a refactor. We are adding a new public option that enables new behavior.

This also reminds me we need some docs around it. It would be nice to have a guide on how to create the fetch HTTP client.

Absolutely. I just have this hunch against advertising this interface - we will probably break it in just a couple of months.

[B4nan]

Well, we will break it with a breaking release, that's fine. We could as well state that this is an experimental API subject to change. Either way, we need to document this.

[janbuchar]

@barjin @B4nan I added a basic test and a guide (I'll pull out the code samples later). Can you check it out? The request type will sadly have to stay this way.

[B4nan]

can we use fetch or axios here or is that more tricky than it sounds like? the example without any implementation is not really helpful.

if the problematic part is streaming, maybe we could provide some default dummy implementation/helper that would just load things into memory and wrap it in a stream?

[janbuchar]

I mean, if I'm going to implement it, I would rather just ship it as code. And writing an incomplete example is not exactly helpful either.

[B4nan]

What's wrong with doing both? Docs are important, you won't get around that just by publishing some package, we need them. If you don't want to do it now, let's show the got scraping client implementation here (which can be simplified, referencing the full working code), but we need something better than what we have now.

[janbuchar]

Well, the curl-impersonate thing is (allegedly) slow and doesn't work on Windows - that's not something I'd like to put on display. A complete implementation of something else (fetch, axios, ...) would take some effort, and nobody asked for it (yet). That and the fact that we're going to change the interface pretty soon anyway (to be more ergonomic, among other things) kinda deter me.

Of course, I could just sketch something here and leave out the tricky parts (e.g., redirection + cookie handling), but that doesn't feel right either. It would look like we're leaving out the gnarly parts and leaving them for the user to figure out.

I guess just linking to that gist with curl-impersonate with a disclaimer could work?

[B4nan]

but that doesn't feel right either.

It would still look better than what we have now. I am fine with leaving out tricky parts for now, but leaving out everything feels weird.

Btw, people already asked indirectly about axios support, we have several reports about things not working with got scraping, but working with axios.

A link to that gist could also help, and I would surely add it here.