-
Notifications
You must be signed in to change notification settings - Fork 1k
feat: use native Request
/ Response
interface
#3163
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.
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
base: v4
Are you sure you want to change the base?
Conversation
]); | ||
expect(isFromCache).toEqual({ first: false, second: true }); | ||
}); | ||
// test('should work with cacheable-request', async () => { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This seems got-scraping
-specific, I'm not sure if we're using this anywhere (at scale).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I haven't managed to read the whole thing yet, sorry
crawlingContext: CheerioCrawlingContext, | ||
) { | ||
const body = await readStreamToString(response); | ||
protected override async _parseHTML(response: Response, isXml: boolean, crawlingContext: CheerioCrawlingContext) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This will cause a merge conflict with the ContextPipeline PR. I would like to go first if that's possible (the vessel that's harder to steer has the right of way).
@@ -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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I would love to see this go as well
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Phasing out
got-scraping
-specific interfaces in favour of nativefetch
API.Closes #3071