Skip to content
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.

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

feat!: Upgrade to node-fetch v3 #617

Open
wants to merge 16 commits into
base: main
Choose a base branch
from
Open
Show file tree
Hide file tree
Changes from 2 commits
Commits
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
5 changes: 2 additions & 3 deletions README.md
Original file line number Diff line number Diff line change
Expand Up @@ -56,7 +56,7 @@ interface GaxiosOptions = {
headers: { 'some': 'header' },

// The data to send in the body of the request. Data objects will be
// serialized as JSON.
// serialized as JSON, except for `FormData`.
//
// Note: if you would like to provide a Content-Type header other than
// application/json you you must provide a string or readable stream, rather
Expand Down Expand Up @@ -151,8 +151,7 @@ interface GaxiosOptions = {
// Enables default configuration for retries.
retry: boolean,

// Cancelling a request requires the `abort-controller` library.
// See https://github.com/bitinn/node-fetch#request-cancellation-with-abortsignal
// Enables aborting via AbortController
signal?: AbortSignal

/**
Expand Down
5 changes: 1 addition & 4 deletions package.json
Original file line number Diff line number Diff line change
Expand Up @@ -47,18 +47,15 @@
"@types/mv": "^2.1.0",
"@types/ncp": "^2.0.1",
"@types/node": "^20.0.0",
"@types/node-fetch": "^2.5.7",
"@types/sinon": "^17.0.0",
"@types/tmp": "0.2.6",
"@types/uuid": "^9.0.0",
"abort-controller": "^3.0.0",
"assert": "^2.0.0",
"browserify": "^17.0.0",
"c8": "^8.0.0",
"cors": "^2.8.5",
"execa": "^5.0.0",
"express": "^4.16.4",
"form-data": "^4.0.0",
"gts": "^5.0.0",
"is-docker": "^2.0.0",
"karma": "^6.0.0",
Expand Down Expand Up @@ -89,7 +86,7 @@
"extend": "^3.0.2",
"https-proxy-agent": "^7.0.1",
"is-stream": "^2.0.0",
"node-fetch": "^2.6.9",
"node-fetch": "^3.3.2",
"uuid": "^9.0.1"
}
}
5 changes: 2 additions & 3 deletions src/common.ts
Original file line number Diff line number Diff line change
Expand Up @@ -119,15 +119,15 @@
}

export interface Headers {
[index: string]: any;

Check warning on line 122 in src/common.ts

View workflow job for this annotation

GitHub Actions / lint

Unexpected any. Specify a different type
}
export type GaxiosPromise<T = any> = Promise<GaxiosResponse<T>>;

Check warning on line 124 in src/common.ts

View workflow job for this annotation

GitHub Actions / lint

Unexpected any. Specify a different type

export interface GaxiosXMLHttpRequest {
responseURL: string;
}

export interface GaxiosResponse<T = any> {

Check warning on line 130 in src/common.ts

View workflow job for this annotation

GitHub Actions / lint

Unexpected any. Specify a different type
config: GaxiosOptions;
data: T;
status: number;
Expand All @@ -149,7 +149,7 @@
* Optional method to override making the actual HTTP request. Useful
* for writing tests.
*/
adapter?: <T = any>(

Check warning on line 152 in src/common.ts

View workflow job for this annotation

GitHub Actions / lint

Unexpected any. Specify a different type
options: GaxiosOptions,
defaultAdapter: (options: GaxiosOptions) => GaxiosPromise<T>
) => GaxiosPromise<T>;
Expand All @@ -170,8 +170,8 @@
| 'TRACE'
| 'PATCH';
headers?: Headers;
data?: any;

Check warning on line 173 in src/common.ts

View workflow job for this annotation

GitHub Actions / lint

Unexpected any. Specify a different type
body?: any;

Check warning on line 174 in src/common.ts

View workflow job for this annotation

GitHub Actions / lint

Unexpected any. Specify a different type
/**
* The maximum size of the http response content in bytes allowed.
*/
Expand All @@ -185,13 +185,13 @@
* A collection of parts to send as a `Content-Type: multipart/related` request.
*/
multipart?: GaxiosMultipartOptions[];
params?: any;

Check warning on line 188 in src/common.ts

View workflow job for this annotation

GitHub Actions / lint

Unexpected any. Specify a different type
paramsSerializer?: (params: {[index: string]: string | number}) => string;
timeout?: number;
/**
* @deprecated ignored
*/
onUploadProgress?: (progressEvent: any) => void;

Check warning on line 194 in src/common.ts

View workflow job for this annotation

GitHub Actions / lint

Unexpected any. Specify a different type
responseType?:
| 'arraybuffer'
| 'blob'
Expand All @@ -203,9 +203,8 @@
validateStatus?: (status: number) => boolean;
retryConfig?: RetryConfig;
retry?: boolean;
// Should be instance of https://developer.mozilla.org/en-US/docs/Web/API/AbortSignal
// interface. Left as 'any' due to incompatibility between spec and abort-controller.
signal?: any;
// Enables aborting via AbortController
signal?: AbortSignal;
size?: number;
/**
* Implementation of `fetch` to use when making the API call. By default,
Expand Down Expand Up @@ -272,7 +271,7 @@
*
* @experimental
*/
export type RedactableGaxiosResponse<T = any> = Pick<

Check warning on line 274 in src/common.ts

View workflow job for this annotation

GitHub Actions / lint

Unexpected any. Specify a different type
GaxiosResponse<T>,
'config' | 'data' | 'headers'
>;
Expand Down Expand Up @@ -336,7 +335,7 @@
init?: FetchRequestInit
) => Promise<FetchResponse>;

export type FetchRequestInfo = any;

Check warning on line 338 in src/common.ts

View workflow job for this annotation

GitHub Actions / lint

Unexpected any. Specify a different type

export interface FetchResponse {
readonly status: number;
Expand Down
106 changes: 59 additions & 47 deletions src/gaxios.ts
Original file line number Diff line number Diff line change
Expand Up @@ -14,11 +14,12 @@
import extend from 'extend';
import {Agent} from 'http';
import {Agent as HTTPSAgent} from 'https';
import nodeFetch from 'node-fetch';
import qs from 'querystring';
import isStream from 'is-stream';
import {URL} from 'url';

import type nodeFetch from 'node-fetch' with {'resolution-mode': 'import'};

import {
FetchResponse,
GaxiosMultipartOptions,
Expand All @@ -30,21 +31,11 @@ import {
defaultErrorRedactor,
} from './common';
import {getRetryConfig} from './retry';
import {PassThrough, Stream, pipeline} from 'stream';
import {Readable} from 'stream';
import {v4} from 'uuid';

/* eslint-disable @typescript-eslint/no-explicit-any */

const fetch = hasFetch() ? window.fetch : nodeFetch;

function hasWindow() {
return typeof window !== 'undefined' && !!window;
}

function hasFetch() {
return hasWindow() && !!window.fetch;
}

function hasBuffer() {
return typeof Buffer !== 'undefined';
}
Expand Down Expand Up @@ -94,8 +85,14 @@ export class Gaxios {
private async _defaultAdapter<T>(
opts: GaxiosOptions
): Promise<GaxiosResponse<T>> {
const fetchImpl = opts.fetchImplementation || fetch;
const res = (await fetchImpl(opts.url, opts)) as FetchResponse;
const fetchImpl = opts.fetchImplementation || (await Gaxios.#getFetch());

// node-fetch v3 warns when `data` is present
// https://github.com/node-fetch/node-fetch/issues/1000
const preparedOpts = {...opts};
delete preparedOpts.data;

const res = await fetchImpl(opts.url, preparedOpts);
const data = await this.getResponseData(opts, res);
return this.translateResponse<T>(opts, res, data);
}
Expand All @@ -122,10 +119,10 @@ export class Gaxios {
if (opts.responseType === 'stream') {
let response = '';
await new Promise(resolve => {
(translatedResponse?.data as Stream).on('data', chunk => {
(translatedResponse?.data as Readable).on('data', chunk => {
response += chunk;
});
(translatedResponse?.data as Stream).on('end', resolve);
(translatedResponse?.data as Readable).on('end', resolve);
});
translatedResponse.data = response as T;
}
Expand Down Expand Up @@ -164,15 +161,13 @@ export class Gaxios {
switch (opts.responseType) {
case 'stream':
return res.body;
case 'json': {
let data = await res.text();
case 'json':
try {
data = JSON.parse(data);
return await res.json();
} catch {
// continue
// fallback to returning text
return await res.text();
}
return data as {};
}
case 'arraybuffer':
return res.arrayBuffer();
case 'blob':
Expand Down Expand Up @@ -267,11 +262,17 @@ export class Gaxios {
}

opts.headers = opts.headers || {};

// FormData is available in Node.js versions 18.0.0+, however there is a runtime
// warning until 18.13.0. Additionally, `node-fetch` v3 only supports the official
// `FormData` or its own exported `FormData` class:
// - https://nodejs.org/api/globals.html#class-formdata
// - https://nodejs.org/en/blog/release/v18.13.0
// - https://github.com/node-fetch/node-fetch/issues/1167
// const isFormData = opts?.data instanceof FormData;
const isFormData = opts.data?.constructor?.name === 'FormData';

if (opts.multipart === undefined && opts.data) {
const isFormData =
typeof FormData === 'undefined'
? false
: opts?.data instanceof FormData;
if (isStream.readable(opts.data)) {
opts.body = opts.data;
} else if (hasBuffer() && Buffer.isBuffer(opts.data)) {
Expand All @@ -280,22 +281,19 @@ export class Gaxios {
if (!hasHeader(opts, 'Content-Type')) {
opts.headers['Content-Type'] = 'application/json';
}
} else if (typeof opts.data === 'object') {
// If www-form-urlencoded content type has been set, but data is
// provided as an object, serialize the content using querystring:
if (!isFormData) {
if (
getHeader(opts, 'content-type') ===
'application/x-www-form-urlencoded'
) {
opts.body = opts.paramsSerializer(opts.data);
} else {
// } else if (!(opts.data instanceof FormData)) {
if (!hasHeader(opts, 'Content-Type')) {
opts.headers['Content-Type'] = 'application/json';
}
opts.body = JSON.stringify(opts.data);
} else if (typeof opts.data === 'object' && !isFormData) {
if (
getHeader(opts, 'content-type') ===
'application/x-www-form-urlencoded'
) {
// If www-form-urlencoded content type has been set, but data is
// provided as an object, serialize the content
opts.body = opts.paramsSerializer(opts.data);
} else {
if (!hasHeader(opts, 'Content-Type')) {
opts.headers['Content-Type'] = 'application/json';
}
opts.body = JSON.stringify(opts.data);
}
} else {
opts.body = opts.data;
Expand All @@ -306,12 +304,8 @@ export class Gaxios {
// and the dependency on UUID removed
const boundary = v4();
opts.headers['Content-Type'] = `multipart/related; boundary=${boundary}`;
const bodyStream = new PassThrough();
opts.body = bodyStream;
pipeline(
this.getMultipartRequest(opts.multipart, boundary),
bodyStream,
() => {}
opts.body = Readable.from(
this.getMultipartRequest(opts.multipart, boundary)
);
}

Expand Down Expand Up @@ -475,6 +469,14 @@ export class Gaxios {
// using `import` to dynamically import the types here
static #proxyAgent?: typeof import('https-proxy-agent').HttpsProxyAgent;

/**
* A cache for the lazily-loaded fetch library.
*
* Should use {@link Gaxios[#getFetch]} to retrieve.
*/
//
static #fetch?: typeof nodeFetch | typeof fetch;

/**
* Imports, caches, and returns a proxy agent - if not already imported
*
Expand All @@ -485,4 +487,14 @@ export class Gaxios {

return this.#proxyAgent;
}

static async #getFetch() {
const hasWindow = typeof window !== 'undefined' && !!window;

this.#fetch ||= hasWindow
? window.fetch
: (await import('node-fetch')).default;

return this.#fetch;
}
}
2 changes: 1 addition & 1 deletion src/retry.ts
Original file line number Diff line number Diff line change
Expand Up @@ -68,7 +68,7 @@ export async function getRetryConfig(err: GaxiosError) {
const delay =
retryDelay + ((Math.pow(2, config.currentRetryAttempt) - 1) / 2) * 1000;

// We're going to retry! Incremenent the counter.
// We're going to retry! Increment the counter.
err.config.retryConfig!.currentRetryAttempt! += 1;

// Create a promise that invokes the retry after the backOffDelay
Expand Down
46 changes: 38 additions & 8 deletions test/test.getch.ts
Original file line number Diff line number Diff line change
Expand Up @@ -16,7 +16,6 @@ import nock from 'nock';
import sinon from 'sinon';
import stream, {Readable} from 'stream';
import {describe, it, afterEach} from 'mocha';
import fetch from 'node-fetch';
import {HttpsProxyAgent} from 'https-proxy-agent';
import {
Gaxios,
Expand All @@ -30,8 +29,6 @@ import {GAXIOS_ERROR_SYMBOL, Headers} from '../src/common';
import {pkg} from '../src/util';
import qs from 'querystring';
import fs from 'fs';
import {Blob} from 'node-fetch';
global.FormData = require('form-data');

nock.disableNetConnect();

Expand Down Expand Up @@ -604,25 +601,56 @@ describe('🥁 configuration options', () => {
});

it('should not stringify the data if it is appended by a form', async () => {
const FormData = (await import('node-fetch')).FormData;
danielbankhead marked this conversation as resolved.
Show resolved Hide resolved
const formData = new FormData();
formData.append('test', '123');
// I don't think matching formdata is supported in nock, so skipping: https://github.com/nock/nock/issues/887
const scope = nock(url).post('/').reply(200);

const scope = nock(url)
.post('/', body => {
/**
* Sample from native `node-fetch`
* body: '------3785545705014550845559551617\r\n' +
* 'Content-Disposition: form-data; name="test"\r\n' +
* '\r\n' +
* '123\r\n' +
* '------3785545705014550845559551617--',
*/

/**
* Sample from native `fetch`
* body: '------formdata-undici-0.39470493152687736\r\n' +
* 'Content-Disposition: form-data; name="test"\r\n' +
* '\r\n' +
* '123\r\n' +
* '------formdata-undici-0.39470493152687736--',
*/

return body.match('Content-Disposition: form-data;');
})
.reply(200);
const res = await request({
url,
method: 'POST',
data: formData,
});
scope.done();
assert.deepStrictEqual(res.config.data, formData);
assert.ok(res.config.body instanceof FormData);
assert.ok(res.config.data instanceof FormData);
assert.deepEqual(res.config.body, undefined);
});

it('should allow explicitly setting the fetch implementation to node-fetch', async () => {
let customFetchCalled = false;
const nodeFetch = (await import('node-fetch')).default;
const myFetch = (...args: Parameters<typeof nodeFetch>) => {
customFetchCalled = true;
return nodeFetch(...args);
};

const scope = nock(url).get('/').reply(200);
const res = await request({url, fetchImplementation: fetch});
const res = await request({url, fetchImplementation: myFetch});
scope.done();
assert(customFetchCalled);
assert.deepStrictEqual(res.status, 200);
});

Expand Down Expand Up @@ -777,7 +805,9 @@ describe('🎏 data handling', () => {
const res = await request({url});
scope.done();
assert.ok(res.data);
assert.strictEqual(res.statusText, 'OK');
// node-fetch and native fetch specs differ...
// https://github.com/node-fetch/node-fetch/issues/1066
assert.strictEqual(typeof res.statusText, 'string');
});

it('should return JSON when response Content-Type=application/json', async () => {
Expand Down
1 change: 0 additions & 1 deletion test/test.retry.ts
Original file line number Diff line number Diff line change
Expand Up @@ -11,7 +11,6 @@
// See the License for the specific language governing permissions and
// limitations under the License.

import {AbortController} from 'abort-controller';
import assert from 'assert';
import nock from 'nock';
import {describe, it, afterEach} from 'mocha';
Expand Down
6 changes: 4 additions & 2 deletions tsconfig.json
Original file line number Diff line number Diff line change
@@ -1,10 +1,12 @@
{
"extends": "./node_modules/gts/tsconfig-google.json",
"compilerOptions": {
"lib": ["es2015", "dom"],
"lib": ["es2020", "dom"],
"rootDir": ".",
"outDir": "build",
"esModuleInterop": true
"esModuleInterop": true,
"module": "Node16",
"moduleResolution": "Node16",
},
"include": [
"src/*.ts",
Expand Down