feat: add AbortSignal and rename timeout → timeoutMillis in SendRequestOptions#3457
feat: add AbortSignal and rename timeout → timeoutMillis in SendRequestOptions#3457
AbortSignal and rename timeout → timeoutMillis in SendRequestOptions#3457Conversation
AbortSignal and rename timeout → timeoutMillis in SendRequestOptions
There was a problem hiding this comment.
Pull request overview
This pull request adds support for custom AbortSignal cancellation to the HTTP client layer and renames the timeout parameter to timeoutMillis to align with the repository's established naming conventions. The changes address the lack of a mechanism for users to pass custom cancellation signals to BaseHttpClient, while also improving consistency across the codebase.
Changes:
- Added
signal?: AbortSignalfield toSendRequestOptionsinterface with TSDoc - Renamed
timeout→timeoutMillisinSendRequestOptionsto follow the establishedMillissuffix convention used throughout the codebase - Implemented
AbortSignal.any()to combine custom signals and timeout-based signals, ensuring requests abort on whichever fires first - Updated all call sites across
@crawlee/basic-crawler,@crawlee/http-crawler, and@crawlee/utilsto use the new parameter name
Reviewed changes
Copilot reviewed 6 out of 6 changed files in this pull request and generated 1 comment.
Show a summary per file
| File | Description |
|---|---|
| packages/types/src/http-client.ts | Added signal field and renamed timeout → timeoutMillis in SendRequestOptions interface with documentation |
| packages/http-client/src/base-http-client.ts | Updated createAbortSignal to accept and combine both signal and timeout; refactored resolveRequestContext to return signal instead of timeout |
| packages/basic-crawler/src/internals/send-request.ts | Updated to pass both timeoutMillis and signal from override options |
| packages/http-crawler/src/internals/http-crawler.ts | Updated call site to use timeoutMillis parameter name |
| packages/utils/src/internals/sitemap.ts | Updated call site to use timeoutMillis parameter name |
| packages/utils/test/mock-http-client.ts | Updated mock implementation to use timeoutMillis parameter name |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
nikitachapovskii-dev
left a comment
There was a problem hiding this comment.
Hi, sorry for a late review 🙂
Please check the nits, hope they are useful
| const cookieJar = options?.cookieJar ?? options?.session?.cookieJar ?? new CookieJar(); | ||
| const timeout = options?.timeout; | ||
| return { proxyUrl, cookieJar: cookieJar as CookieJar, timeout }; | ||
| const signal = this.createAbortSignal(options?.signal, options?.timeoutMillis); |
There was a problem hiding this comment.
It seems like the timeout behavior changed with this refactor. Previously, the timeout signal was created inside the redirect loop (per fetch attempt). Now the signal is created once before the loop and reused across all redirects
If I don't miss smth, this can lead to cases, when redirect chains that used to pass (each hop under timeout) may now fail if cumulative time exceeds timeoutMillis.
If this is not expected, we should create the timeout signal per iteration
There was a problem hiding this comment.
I would argue that this behaviour is more straightforward (user sets timeoutMillis as a sendRequest param, so they expect the sendRequest call to take up to this time).
I'm not sure how this worked before (it was probably implementation-specific), so this is imo a step forward (in v4 nonetheless, as we can afford breaking changes like this). Wdyt?
There was a problem hiding this comment.
I think this is a reasonable v4 direction, and I’m okay with keeping it 👍
…endRequestOptions Co-authored-by: barjin <61918049+barjin@users.noreply.github.com>
…Client Co-authored-by: barjin <61918049+barjin@users.noreply.github.com>
…ndRequestOptions` Co-authored-by: barjin <61918049+barjin@users.noreply.github.com>
0f928f8 to
3a5a416
Compare
…ndRequestOptions` (#3457) `SendRequestOptions` lacked an `AbortSignal` option, preventing users from passing custom cancellation signals to `BaseHttpClient`. The `timeout` field also didn't follow the repo convention of using `timeoutMillis`. --------- Co-authored-by: copilot-swe-agent[bot] <198982749+Copilot@users.noreply.github.com> Co-authored-by: barjin <61918049+barjin@users.noreply.github.com> Co-authored-by: Jindřich Bär <jindrichbar@gmail.com>
SendRequestOptionslacked anAbortSignaloption, preventing users from passing custom cancellation signals toBaseHttpClient. Thetimeoutfield also didn't follow the repo convention of usingtimeoutMillis.Changes
@crawlee/types—SendRequestOptions: renamedtimeout→timeoutMillis, addedsignal?: AbortSignal@crawlee/http-client—BaseHttpClient: updated to usetimeoutMillis; when bothsignalandtimeoutMillisare provided, combines them viaAbortSignal.any()@crawlee/basic-crawler,@crawlee/http-crawler,@crawlee/utils— updated all call sites to usetimeoutMillistest/core/base-http-client.test.ts— added tests covering custom signal propagation, signal + timeoutMillis combination, and timeout-based abortUsage
When both are supplied, the request is aborted on whichever fires first.
Original prompt
AbortSignaloption to theBaseHttpClientinterface #3456💡 You can make Copilot smarter by setting up custom instructions, customizing its development environment and configuring Model Context Protocol (MCP) servers. Learn more Copilot coding agent tips in the docs.