Skip to content

fix(gax): remove node-fetch types and dependency #9785

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
aelmanaa wants to merge 1 commit into
base: main
Choose a base branch
from
Open

fix(gax): remove node-fetch types and dependency #9785

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 0 additions & 1 deletion core/packages/gax/package.json
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -16,7 +16,6 @@
"duplexify": "^4.1.3",
"google-auth-library": "^11.3.1",
"google-logging-utils": "^2.1.0",
"node-fetch": "^3.3.2",
"object-hash": "^3.0.0",
"proto3-json-serializer": "^4.1.0",
"protobufjs": "^7.5.4",
Expand Down
15 changes: 8 additions & 7 deletions core/packages/gax/src/fallbackServiceStub.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -14,10 +14,6 @@
* limitations under the License.
*/

import type {Response as NodeFetchResponse} from 'node-fetch' with {
'resolution-mode': 'import',
};

import {AuthClient, GoogleAuth, gaxios} from 'google-auth-library';
import * as serializer from 'proto3-json-serializer';

Expand All @@ -27,7 +23,8 @@ import {defaultToObjectOptions} from './fallback';
import {GoogleError} from './googleError';
import {setAttemptHttpMethod} from './observability/TracerHelper';
import {rpcCodeFromHttpStatusCode, Status} from './status';
import {pipeline, PipelineSource} from 'stream';
import {pipeline} from 'stream';
import type {ReadableStream as WebReadableStream} from 'stream/web';
import type {Agent as HttpAgent} from 'http';
import type {Agent as HttpsAgent} from 'https';

Expand Down Expand Up @@ -516,7 +513,7 @@ export function generateServiceStub(

auth
.fetch(url, fetchRequest)
.then((response: Response | NodeFetchResponse) => {
.then((response: gaxios.GaxiosResponse) => {
// There is a legacy Apiary configuration that some services
// use which allows 204 empty responses on success instead of
// a 200 OK. This most commonly is seen in delete RPCs,
Expand All @@ -529,7 +526,11 @@ export function generateServiceStub(
}
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,
Comment on lines 527 to +533

@gemini-code-assist gemini-code-assist Bot Oct 30, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

medium

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
  1. Always validate and reject missing or falsy arguments upfront before performing property checks, to prevent TypeErrors.

@aelmanaa aelmanaa Nov 1, 2026

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

streamArrayParser,
(err: unknown) => {
// A caller that cancelled has already torn this stream down
Expand Down
3 changes: 0 additions & 3 deletions pnpm-lock.yaml
View file
Open in desktop

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

Loading