Abort timed-out requests with TimeoutError DOMException

[snoack]

This mimics the intrinsic behavior of AbortSignal.timeout() which we plan to eventually switch to.

This change is 

[pkaminski]

@codex review

Triggered automatically because this PR was opened ready for review or was marked ready for review.

[chatgpt-codex-connector]

💡 Codex Review

hubkit/hubkit.js

Line 665 in 30435e2

if (config.timeout) init.signal = AbortSignal.timeout(config.timeout);

Preserve timeout fallback for older browser runtimes

Using AbortSignal.timeout() unconditionally regresses browser compatibility for clients that support fetch/AbortController but not the newer static timeout API; in those environments, any request with timeout set now throws TypeError: AbortSignal.timeout is not a function before the network call starts. The previous AbortController + setTimeout path worked there, so this change breaks timeout-enabled requests unless you add a feature-detected fallback.

ℹ️ About Codex in GitHub

Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you

• Open a pull request for review
• Mark a draft as ready
• Comment "@codex review".

If Codex has suggestions, it will comment; otherwise it will react with 👍.

Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".

[snoack]

I did not bump the version yet, because as far as I'm concerned it's fine if this simplification makes it into the next release. However, if you want to do another release before we update the dependency in Reviewable, let me know.

Using AbortSignal.timeout() unconditionally regresses browser compatibility

AbortSignal.timeout requires Chrome 103, Firefox 100 or Safari 15.4 — which are all from early 2022 and well past the "last 2 versions" cutoff.

@snoack made 1 comment.
Reviewable status: 0 of 3 files reviewed, all discussions resolved.

[pkaminski]

Please always bump the version as it's not possible to overwrite a previously published version with new code.

caniuse says it's more like a 2024 baseline and 88% penetration. I usually accept that for features with minor side-effects (e.g., degraded visuals or a very specific action failing) but for whole-app-breaking stuff I prefer to set the bar at 93% or so unless it makes our life significantly easier. The "last 2 versions" cutoff is mostly for CSS.

@pkaminski reviewed 3 files and all commit messages, and made 2 comments.
Reviewable status: all files reviewed, 1 unresolved discussion (waiting on snoack).

---

README.md line 131 at r2 (raw file):

* `host`: The URL to prepend to all request paths; defaults to `https://api.github.com`.
* `graphHost`: The URL to use for all GraphQL requests; defaults to using the value of `host` which works fine for `github.com`, but you'll need to set a separate value when working with GitHub Enterprise.
* `timeout`: The timeout in milliseconds to apply to the request; none by default.  If the timeout is reached, the request will abort with a `TimeoutError`.

I guess that's technically another major version bump.

[snoack]

Codex did lied to me. The browser versions stated above are wrong, instead it requires Chrome 124 which was released in 2024. Fair enough, I'm closing this PR.

@snoack made 1 comment.
Reviewable status: all files reviewed, 1 unresolved discussion (waiting on snoack).

[snoack]

I reopened the PR, and made the change backwards compatible. What do you think?

@snoack made 2 comments.
Reviewable status: 1 of 4 files reviewed, 1 unresolved discussion (waiting on pkaminski).

---

README.md line 131 at r2 (raw file):

Previously, pkaminski (Piotr Kaminski) wrote…

I guess that's technically another major version bump.

Do you think it would be acceptable to only bump the minor version here? I don't want to run through major versions unnecessarily fast, and if we release this soon, 7.0.0 was only briefly available and never used by Reviewable.

[pkaminski]

I'm not convinced it's worth it: it increases code complexity and introduces a noticeable discrepancy between real TimeoutError instances and AbortError instances with a name override that will fail an instanceof test. I'd just wait a year or two and migrate to AbortSignal.timeout wholesale once it's supported well enough..

@pkaminski reviewed 2 files and all commit messages, made 2 comments, and resolved 1 discussion.
Reviewable status: 3 of 4 files reviewed, all discussions resolved (waiting on snoack).

---

README.md line 131 at r2 (raw file):

Previously, snoack (Sebastian Noack) wrote…

Do you think it would be acceptable to only bump the minor version here? I don't want to run through major versions unnecessarily fast, and if we release this soon, 7.0.0 was only briefly available and never used by Reviewable.

Yeah, I think it'd be fine to make an exception in this case.

[snoack]

We could throw new DOMException('The operation timed out.', 'TimeoutError') to 100% match the original exception. One benefit of using AbortSignal.timeout in Node.js is that unlikesetTimeout it doesn't keep the process alive, also it is lightweighter. But the complexity of having two different code paths is a valid counter point.

@snoack made 1 comment.
Reviewable status: 3 of 4 files reviewed, all discussions resolved (waiting on pkaminski).

[pkaminski]

Ah, I thought that TimeoutError was a separate class — my mistake. I don't think setTimeout keeping the process alive is an issue since we use System.exit consistently instead of relying on self-termination.

How about this: let's stick with AbortController to retain a single code path but faithfully emulate TimeoutError when throwing. That way, all the clients can be migrate to the future API right away, and when the time comes to switch to AbortSignal we'll be able to do so easily. What do you think?

@pkaminski made 1 comment.
Reviewable status: 3 of 4 files reviewed, all discussions resolved (waiting on snoack).

[snoack]

Done.

@snoack made 1 comment.
Reviewable status: 3 of 4 files reviewed, all discussions resolved (waiting on pkaminski).

[pkaminski]

@pkaminski reviewed 2 files and all commit messages.
Reviewable status: complete! all files reviewed, all discussions resolved (waiting on snoack).