feat: use native Request / Response interface
#3163
Conversation
janbuchar
left a comment
janbuchar
left a comment
There was a problem hiding this comment.
I haven't managed to read the whole thing yet, sorry
| @@ -1,5 +1,5 @@ | |||
| import type { BatchAddRequestsResult, Dictionary } from '@crawlee/types'; | |||
| import type { OptionsInit, Response as GotResponse } from 'got-scraping'; | |||
| import type { OptionsInit } from 'got-scraping'; | |||
There was a problem hiding this comment.
I would love to see this go as well
There was a problem hiding this comment.
I'd prefer to do that as a part of a separate PR. Removing got-scraping (and all the type todos) is no small feat, which would make it hard to review, if done all-in-one.
| * Perform an HTTP Request and return after the response headers are received. The body may be read from a stream contained in the response. | ||
| */ | ||
| stream(request: HttpRequest, onRedirect?: RedirectHandler): Promise<StreamingHttpResponse>; | ||
| stream(request: HttpRequest, onRedirect?: RedirectHandler): Promise<Response>; |
There was a problem hiding this comment.
Isn't the stream method obsolete? The web Response class can be streamed using response.body when the caller chooses to do so
There was a problem hiding this comment.
Good point, it actually is 👍 I'd prefer to do this in a separate PR, for the same reasons as the total got-scraping phase-out.
janbuchar
left a comment
janbuchar
left a comment
There was a problem hiding this comment.
Looks mostly good to me, it's a huge simplification! I'll leave it up to you which of the unresolved comments deserve immediate attention and which should be left for another PR.
| pushData: this.pushData.bind(this), | ||
| useState: this.useState.bind(this), | ||
| sendRequest: createSendRequest(this.httpClient, request!, session), | ||
| sendRequest: createSendRequest(this.httpClient, request!, session) as CrawlingContext['sendRequest'], |
There was a problem hiding this comment.
I have no problem with adding a cast, but I can't help myself asking... what is the issue?
There was a problem hiding this comment.
The got-accepted CookieJar class and the Crawlee-supplied CookieJar interface are not completely compatible. I'd prefer to deal with this in the got-scraping removal PR, as we'll likely touch this code again.
There was a problem hiding this comment.
okay, that's probably issue material then
| /** | ||
| * The stream object returned from got does not have the below properties. | ||
| * At the same time, you can't read data directly from the response stream, | ||
| * because they won't get emitted unless you also read from the primary | ||
| * got stream. To be able to work with only one stream, we move the expected props | ||
| * from the response stream to the got stream. | ||
| * @internal | ||
| */ | ||
| function addResponsePropertiesToStream(stream: Readable, response: StreamingHttpResponse) { | ||
| const properties: (keyof PlainResponse)[] = [ | ||
| 'statusCode', | ||
| 'statusMessage', | ||
| 'headers', | ||
| 'complete', | ||
| 'httpVersion', | ||
| 'rawHeaders', | ||
| 'rawTrailers', | ||
| 'trailers', | ||
| 'url', | ||
| 'request', | ||
| ]; | ||
|
|
||
| stream.on('end', () => { | ||
| // @ts-expect-error | ||
| if (stream.rawTrailers) stream.rawTrailers = response.rawTrailers; // TODO BC with got - remove in 4.0 | ||
|
|
||
| // @ts-expect-error | ||
| if (stream.trailers) stream.trailers = response.trailers; | ||
|
|
||
| // @ts-expect-error | ||
| stream.complete = response.complete; | ||
| }); | ||
|
|
||
| for (const prop of properties) { | ||
| if (!(prop in stream)) { | ||
| (stream as any)[prop] = (response as any)[prop]; | ||
| } | ||
| } | ||
|
|
||
| return stream as unknown as PlainResponse; | ||
| } | ||
|
|
There was a problem hiding this comment.
Seeing this go is pure joy.
| cleanup: this.cleanupDownload.bind(this), | ||
| action: async (context) => this.initiateDownload(context), | ||
| cleanup: async (context) => { | ||
| await (context.response.body ? finished(context.response.body as any) : Promise.resolve()); |
There was a problem hiding this comment.
But what if the request handler doesn't even start reading the stream? Or if it crashes midway? We should make sure we prevent hanging in that case.
There was a problem hiding this comment.
Yes, currently, this will hang if nobody touches the response stream (even reading it "by proxy" via .text() or .bytes() is fine).
Since this is the current behaviour in v3 as well, I'd keep this here and make a note somewhere to revisit this eventually.
There was a problem hiding this comment.
make it an issue please 😄
|
Also, can you mention breaking changes in the upgrading guide, change the PR type to |
Phasing out
got-scraping-specific interfaces in favour of nativefetchAPI.Closes #3071