Repository navigation
Conversation
|
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 removes the node-fetch dependency from the gax package, updating fallbackServiceStub.ts to use gaxios.GaxiosResponse and cast the response body to WebReadableStream. The feedback points out a potential runtime TypeError if response.body is null, and suggests validating its presence before passing it to pipeline.
| if (response.ok && rpc.responseStream) { | ||
| pipeline( | ||
| response.body as PipelineSource<unknown>, | ||
| // `GaxiosResponse` is typed as the DOM `Response`, but the body | ||
| // is a Node.js Readable when gaxios uses node-fetch and an async | ||
| // iterable web ReadableStream when it uses native fetch; either | ||
| // is a valid `pipeline` source in Node.js. | ||
| response.body as WebReadableStream, |
There was a problem hiding this comment.
If response.body is null (which is possible according to the Response type definition, e.g., if the server returns an empty body or on certain network conditions), passing it directly to pipeline will cause a synchronous TypeError at runtime.
To prevent this and ensure robust error handling, we should check if response.body is present before calling pipeline. If it is missing, we can destroy the streamArrayParser with a GoogleError.
if (response.ok && rpc.responseStream) {
if (!response.body) {
streamArrayParser.destroy(
new GoogleError('Response body is empty')
);
return;
}
pipeline(
// `GaxiosResponse` is typed as the DOM `Response`, but the body
// is a Node.js Readable when gaxios uses node-fetch and an async
// iterable web ReadableStream when it uses native fetch; either
// is a valid `pipeline` source in Node.js.
response.body as WebReadableStream,References
- Always validate and reject missing or falsy arguments upfront before performing property checks, to prevent TypeErrors.
There was a problem hiding this comment.
I tested this with a local HTTP server, on main and on this branch. The server-streaming call returned three responses: a 200 with a body, a 200 with an empty body, and a 204.
On Node, gaxios uses node-fetch, and response.body is never null, including for the 204. The empty 200 and the 204 both end the stream cleanly. With a native-fetch Response, the kind Bun and browsers return, only the 204 has a null body. In that case pipeline throws inside the .then, and the existing .catch reports the error as an UNAVAILABLE GoogleError on the stream and on the callback. Nothing throws to the caller and nothing hangs.
Main and this branch gave the same output. This PR only changes the type of response, and the only difference in the compiled JavaScript is the added comment. So I'm leaving the PR as it is. The difference between node-fetch and native fetch for a 204 on a server stream already exists on main and can be handled in a separate change.
dc1be84 to
b6414ac
Compare
fallbackServiceStub imported a Response type from node-fetch only to type the response of auth.fetch(). Type it as gaxios.GaxiosResponse, which google-auth-library returns, and pass the body to pipeline as a stream/web ReadableStream. Remove node-fetch from google-gax's dependencies. The compiled JavaScript is unchanged apart from a comment. Fixes googleapis#7679
b6414ac to
0a9341c
Compare
Removes the type-only node-fetch import from fallbackServiceStub, the follow-up gax-nodejs#1766 left for later ("In a follow-up we can remove node-fetch altogether"). The response of auth.fetch() is now typed as gaxios.GaxiosResponse, which is what google-auth-library 11 returns. Its body goes to pipeline() as a stream/web ReadableStream, a type-only import, with a comment on the two shapes it has at runtime. node-fetch is removed from google-gax's dependencies and from the gax entry in pnpm-lock.yaml.
google-gax never imported node-fetch at runtime. The compiled fallbackServiceStub.js is identical to main apart from one comment, and the declaration files are identical.
This doesn't remove node-fetch from users' dependency trees on its own. gaxios 8 still depends on it and loads it in Node.js, so it stays installed through google-auth-library and gcp-metadata until gaxios moves to native fetch (#9101, #8107).
Fixes #7679