Skip to content

fix: do not call onResponse when no response was received - #89

Merged
L-Blondy merged 3 commits into
masterfrom
laurent/fix-onresponse-undefined-76e3
Sep 1, 2026
Merged

L-Blondy merged 3 commits into
masterfrom
laurent/fix-onresponse-undefined-76e3

Conversation

@L-Blondy

@L-Blondy L-Blondy commented Aug 25, 2026 •

Copy link
Copy Markdown
Owner

Fixes #88

Problem

onResponse is typed (response: Response, request: Request) but was invoked with undefined when fetchFn rejected (e.g. ECONNREFUSED), via an unsound non-null assertion in up.ts. A hook trusting the type then threw a TypeError that masked the original network error.

Repro (before this fix):

const upfetch = up(fetch)
await upfetch('http://localhost:59999/nope', {
   onResponse(response) {
      // response is undefined here, despite being typed as Response
   },
})
// throws ECONNREFUSED

Fix

Throw the stored error before invoking onResponse:

if (error) throw error
await defaultOpts.onResponse?.(response!, request)
await fetcherOpts.onResponse?.(response!, request)
  • After the retry loop, error === undefined implies a response was received, so the remaining response! assertions become sound.
  • onResponse now fires iff the request concluded with a response; it still runs before onError for rejected (non-2xx) responses.
  • This also covers the retry edge case where an earlier attempt received a response but the final attempt failed with a network error: onResponse no longer fires with the stale response, and a throwing hook can no longer mask the original error.
  • The network-failure path is already covered by onError (and the error is still thrown), so no information is lost.

This was chosen over widening the type to Response | undefined, which would be a breaking change forcing every consumer to handle undefined.

Also updates the onResponse doc comments and README tables to clarify that it only runs when a response is received.

Testing

  • Regression test: a request to a port with no listener (real ECONNREFUSED) must not trigger onResponse (default or fetcher-level), and the call still rejects.
  • Regression test: attempt 1 returns a 500, the retry fails with a network error — onResponse must not fire with the stale first response.
  • Full suite passes: 160 tests, biome check, and tsc --noEmit.
Open in Web Open in Cursor 

cursoragent and others added 3 commits August 25, 2026 08:58
When fetch itself fails (e.g. network error like ECONNREFUSED),
onResponse was called with undefined despite its typing guaranteeing
a Response. Only invoke onResponse when a response actually exists;
network failures still flow through onError.

Co-authored-by: Laurent Blondy <L-Blondy@users.noreply.github.com>
Reword the onResponse doc comments and README entries, drop the
inline comment in up.ts, and test against a real refused connection
(127.0.0.1:59999) instead of an msw-simulated network error.

Co-authored-by: Laurent Blondy <L-Blondy@users.noreply.github.com>
Hoisting the throw above the onResponse calls (instead of guarding
with if (response)) establishes the invariant that no error implies
a response was received, making the remaining non-null assertions
sound. It also covers the retry case where an earlier attempt got a
response but the final attempt failed: onResponse no longer fires
with a stale response, and a throwing hook can no longer mask the
original network error.

Co-authored-by: Laurent Blondy <L-Blondy@users.noreply.github.com>
@L-Blondy
L-Blondy marked this pull request as ready for review August 25, 2026 09:42
@L-Blondy
L-Blondy merged commit f0803bd into master Sep 1, 2026
6 checks passed
@L-Blondy
L-Blondy deleted the laurent/fix-onresponse-undefined-76e3 branch September 1, 2026 10:25
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

onResponse misleading typing

2 participants