Skip to content

feat(node): support timeout: null to disable the request timeout - #572

Merged
barjin merged 2 commits into
masterfrom
feat/node-null-timeout
Oct 14, 2026
Merged

barjin merged 2 commits into
masterfrom
feat/node-null-timeout

Conversation

@barjin

@barjin barjin commented Oct 14, 2026

Copy link
Copy Markdown
Member

fetch now accepts timeout: null to disable the request timeout, matching timeout=None in the Python bindings.

Closes #571

@barjin barjin self-assigned this Oct 14, 2026
@barjin
barjin marked this pull request as ready for review October 14, 2026 12:12
@apify-service-account apify-service-account added tested Temporary label used only programatically for some analytics. t-tooling Issues with this label are in the ownership of the tooling team. labels Oct 14, 2026
@barjin
barjin requested a review from Pijukatel October 14, 2026 12:28

@Pijukatel Pijukatel left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Just a readability nit.

Comment thread impit-node/src/lib.rs
Comment on lines +139 to +140
Either::A(millis) => Some(Duration::from_millis(millis.into())),
Either::B(_) => None,

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I do not know what is idiomatic in Rust, but to me it seems less readable.

When using Either::A(...), Either::B(...) I do not have enough context to read that line. I need to find the type in a different file and see what was the first option and what was the second option. I would prefer a named variant where I would get all the context from the variant name already.

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Agreed, but this Either<...,...> is from the napi-rs lib (has effect on the generated TS types etc). Making a custom type compatible with this would take more effort than this is worth imo :/

@barjin
barjin merged commit a974f50 into master Oct 14, 2026
33 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

t-tooling Issues with this label are in the ownership of the tooling team. tested Temporary label used only programatically for some analytics.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

[node] Support timeout: null to disable the request timeout

3 participants