fix: consume stream response body via streamToString instead of res.response.text() - #10810
Conversation
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request replaces direct calls to .text() on response objects with streamToString to read response bodies as streams across several files, and adds corresponding error-handling tests. The review feedback correctly points out that the response body can be null or undefined, which would lead to runtime TypeErrors when passed to streamToString. To adhere to the repository's strict null checks style guide, you should explicitly handle these nullish cases before attempting to read the stream.
| }); | ||
| if (res.status >= 400) { | ||
| throw new FirebaseError(`Failed to export storage: ${await res.response.text()}`); | ||
| const errorMsg = res.body ? await streamToString(res.body as NodeJS.ReadableStream) : ""; |
There was a problem hiding this comment.
Instead of casting, please set the generic type on client.request (as we do already in profiler.ts)
| } | ||
| if (res.status !== 200) { | ||
| const errorMessage = await res.response.text(); | ||
| const errorMessage = res.body ? await streamToString(res.body as NodeJS.ReadableStream) : ""; |
There was a problem hiding this comment.
Instead of casting, please set the generic type on client.request (as we do already in profiler.ts)
| if (res.status >= 400) { | ||
| // TODO(bkendall): consider moving stream-handling logic to responseToError. | ||
| const r = await res.response.text(); | ||
| const r = res.body ? await utils.streamToString(res.body) : ""; |
There was a problem hiding this comment.
Needing to do this at the call site is ugly. Let's move it into the util.streamtoString function
3 participants
Fixes #10809
Root Cause
When native
fetchis used withresponseType: "stream",apiv2.tsconverts the Fetch API'sReadableStream(res.body) into a Node.jsReadablestream usingReadable.fromWeb(res.body). Under Web Streams API semantics, this locks the underlyingReadableStream. Subsequent calls tores.response.text()on the nativeResponseobject throwTypeError: Body is unusable: Body has already been read.Fix
Updated handlers that consume stream responses to read from
res.bodyviastreamToString(res.body)rather than callingres.response.text().Verification
npm test) passed.npm run lint) passed cleanly.