fix: prefer native fetch over node-fetch in Node.js - #9101
Conversation
node-fetch can intermittently reject requests with `Premature close`, notably POSTs to the OAuth2 token endpoint. Native fetch is unaffected and has been stable since Node 18, this package's minimum supported version. node-fetch remains a fallback when no global fetch exists. Two differences between the implementations are normalized so the default path behaves as before: - responseType: 'stream' converts the ReadableStream back to a stream.Readable. A caller-provided fetchImplementation is untouched. - A non-Error AbortSignal reason is propagated instead of being replaced with a generic message.
|
Thanks for your pull request! It looks like this may be your first contribution to a Google open source project. Before we can look at your pull request, you'll need to sign a Contributor License Agreement (CLA). View this failed invocation of the CLA check for more information. For the most up to date status, view the checks section at the bottom of the pull request. |
There was a problem hiding this comment.
Code Review
This pull request updates Gaxios to prefer the native globalThis.fetch over node-fetch in environments where it is available (Node 18+), updating error handling and tests to align with native fetch's behavior. To maintain the stream.Readable contract when using native fetch, Readable.fromWeb is used to convert the response body. The review feedback highlights a potential runtime crash in browser environments where Readable might be undefined, causing instanceof Readable to throw a TypeError. It is recommended to safely check if Readable and Readable.fromWeb are defined before performing the check and conversion.
| if (usingInternalFetch && res.body && !(res.body instanceof Readable)) { | ||
| // Native `fetch` resolves a `ReadableStream`, so convert it to retain | ||
| // the `stream.Readable` contract. A caller-provided | ||
| // `fetchImplementation` keeps its own body type. | ||
| return Readable.fromWeb( | ||
| res.body as unknown as import('stream/web').ReadableStream, | ||
| ); | ||
| } |
There was a problem hiding this comment.
In browser environments or environments where the stream module is stubbed or not polyfilled, Readable may be undefined. Referencing instanceof Readable when Readable is undefined will throw a TypeError: Right-hand side of 'instanceof' is not an object.
To prevent runtime crashes in browser environments, we should safely check if Readable is defined and is a function, and whether Readable.fromWeb is available before using them.
if (
usingInternalFetch &&
res.body &&
typeof Readable === 'function' &&
typeof Readable.fromWeb === 'function' &&
!(res.body instanceof Readable)
) {
// Native fetch resolves a ReadableStream, so convert it to retain
// the stream.Readable contract. A caller-provided
// fetchImplementation keeps its own body type.
return Readable.fromWeb(
res.body as unknown as import('stream/web').ReadableStream,
);
}Readable.fromWeb is unavailable in browser bundles, where stream is stubbed or polyfilled by stream-browserify, and Readable itself may be undefined. Detect a web stream by its getReader method and confirm fromWeb exists before converting, so browsers keep resolving a ReadableStream as they did previously.
|
Addressed in e8ec31e — the conversion now checks that fromWeb is available before using it, and no longer references instanceofReadable. This is my first contribution to a project of this size, so apologies for any review overhead. Thanks for taking a look. |
Problem
Gaxios.#getFetch()only checks for a browserwindowbefore falling back tonode-fetch, so Node.js always usesnode-fetcheven though nativefetchhasbeen available since Node 18 — this package's minimum supported version.
node-fetchintermittently rejects withPremature closeon some networks. Thisreproduces as a consistent
clasp loginfailure, whose OAuth2 token exchange runsthrough google-auth-library and gaxios:
The same POST succeeds via curl and via Node's native
fetchon the same machine,so the failure is specific to
node-fetch.Change
Prefer native
fetchin Node.js, keepingnode-fetchas a fallback when no globalfetchexists. Browser behavior is unchanged.Two implementation differences are normalized to preserve existing behavior:
responseType: 'stream'converts nativefetch'sReadableStreamback to astream.Readable. A caller-suppliedfetchImplementationstill returns its ownbody type, as covered by the existing test.
AbortSignalreason that is not anErroris propagated rather than replacedwith a generic message. The existing test for this case carried a comment
anticipating the change, which is now applied.
Testing
npm test— 129 passing.