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.

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

Switch from node-fetch to undici #402

Closed
wants to merge 1 commit into from
Closed

Switch from node-fetch to undici #402

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
2 changes: 1 addition & 1 deletion README.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -228,7 +228,7 @@ Request parameters that correspond to file uploads can be passed in many differe

```ts
import fs from 'fs';
import fetch from 'node-fetch';
import { fetch } from 'undici';
import OpenAI, { toFile } from 'openai';

const openai = new OpenAI();
Expand Down
10 changes: 6 additions & 4 deletions ecosystem-tests/node-ts-cjs/tests/test-node.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,11 +1,12 @@
import 'openai/shims/node';
import OpenAI, { toFile } from 'openai';
import { TranscriptionCreateParams } from 'openai/resources/audio/transcriptions';
import fetch from 'node-fetch';
import { File as FormDataFile, Blob as FormDataBlob } from 'formdata-node';
import * as fs from 'fs';
import { distance } from 'fastest-levenshtein';
import { ChatCompletion } from 'openai/resources/chat/completions';
import { Readable } from 'node:stream';
import { ReadableStream } from 'node:stream/web';

const url = 'https://audio-samples.github.io/samples/mp3/blizzard_biased/sample-1.mp3';
const filename = 'sample-1.mp3';
Expand Down Expand Up @@ -67,10 +68,11 @@ it(`raw response`, async function () {

// test that we can use node-fetch Response API
Copy link

Choose a reason for hiding this comment

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

This comment probably needs updating

const chunks: string[] = [];
response.body.on('data', (chunk) => chunks.push(chunk));
const body = Readable.fromWeb(response.body as ReadableStream<any>)
body.on('data', (chunk) => chunks.push(chunk));
await new Promise<void>((resolve, reject) => {
response.body.once('end', resolve);
response.body.once('error', reject);
body.once('end', resolve);
body.once('error', reject);
});
const json: ChatCompletion = JSON.parse(chunks.join(''));
expect(json.choices[0]?.message.content || '').toBeSimilarTo('This is a test', 10);
Expand Down
4 changes: 3 additions & 1 deletion ecosystem-tests/node-ts-esm-auto/tests/test.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -6,6 +6,8 @@ import { File as FormDataFile, Blob as FormDataBlob } from 'formdata-node';
import * as fs from 'fs';
import { distance } from 'fastest-levenshtein';
import { ChatCompletion } from 'openai/resources/chat/completions';
import { Readable } from 'node:stream';
import { ReadableStream } from 'node:stream/web';

const url = 'https://audio-samples.github.io/samples/mp3/blizzard_biased/sample-1.mp3';
const filename = 'sample-1.mp3';
Expand Down Expand Up @@ -67,7 +69,7 @@ it(`raw response`, async function () {

// test that we can use node-fetch Response API
const chunks: string[] = [];
const { body } = response;
const body = Readable.fromWeb(response.body as ReadableStream<any>)
if (!body) throw new Error(`expected response.body to be defined`);
body.on('data', (chunk) => chunks.push(chunk));
await new Promise<void>((resolve, reject) => {
Expand Down
4 changes: 3 additions & 1 deletion ecosystem-tests/node-ts-esm/tests/test-esnext.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3,6 +3,8 @@ import OpenAI from 'openai';
import { distance } from 'fastest-levenshtein';
import { ChatCompletion } from 'openai/resources/chat/completions';
import * as shims from 'openai/_shims/index';
import { Readable } from 'node:stream';
import { ReadableStream } from 'node:stream/web';

// The tests in this file don't typecheck with "moduleResolution": "node"

Expand Down Expand Up @@ -54,7 +56,7 @@ it(`raw response`, async function () {

// test that we can use node-fetch Response API
const chunks: string[] = [];
const { body } = response;
const body = Readable.fromWeb(response.body as ReadableStream<any>)
if (!body) throw new Error(`expected response.body to be defined`);
body.on('data', (chunk) => chunks.push(chunk));
await new Promise<void>((resolve, reject) => {
Expand Down
9 changes: 6 additions & 3 deletions ecosystem-tests/node-ts4.5-jest27/tests/test.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -6,6 +6,8 @@ import { File as FormDataFile, Blob as FormDataBlob } from 'formdata-node';
import * as fs from 'fs';
import { distance } from 'fastest-levenshtein';
import { ChatCompletion } from 'openai/resources/chat/completions';
import { Readable } from 'node:stream';
import { ReadableStream } from 'node:stream/web';

const url = 'https://audio-samples.github.io/samples/mp3/blizzard_biased/sample-1.mp3';
const filename = 'sample-1.mp3';
Expand Down Expand Up @@ -67,10 +69,11 @@ it(`raw response`, async function () {

// test that we can use node-fetch Response API
const chunks: string[] = [];
response.body.on('data', (chunk) => chunks.push(chunk));
const body = Readable.fromWeb(response.body as ReadableStream<any>)
body.on('data', (chunk) => chunks.push(chunk));
await new Promise<void>((resolve, reject) => {
response.body.once('end', resolve);
response.body.once('error', reject);
body.once('end', resolve);
body.once('error', reject);
});
const json: ChatCompletion = JSON.parse(chunks.join(''));
expect(json.choices[0]?.message.content || '').toBeSimilarTo('This is a test', 10);
Expand Down
2 changes: 0 additions & 2 deletions ecosystem-tests/vercel-edge/tests/test.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,5 +1,3 @@
import fetch from 'node-fetch';

const baseUrl = process.env.TEST_BASE_URL || 'http://localhost:3000';
console.log(baseUrl);

Expand Down
1 change: 1 addition & 0 deletions package.json
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -29,6 +29,7 @@
"form-data-encoder": "1.7.2",
"formdata-node": "^4.3.2",
"node-fetch": "^2.6.7",
"undici": "^5.28.1",
"web-streams-polyfill": "^3.2.1"
},
"devDependencies": {
Expand Down
36 changes: 16 additions & 20 deletions src/_shims/node-runtime.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,22 +1,18 @@
/**
* Disclaimer: modules in _shims aren't intended to be imported by SDK users.
*/
import * as nf from 'node-fetch';
import * as fd from 'formdata-node';
import { type File, type FilePropertyBag } from 'formdata-node';
import KeepAliveAgent from 'agentkeepalive';
import { AbortController as AbortControllerPolyfill } from 'abort-controller';
import { ReadStream as FsReadStream } from 'node:fs';
import { type Agent } from 'node:http';
import uf from 'undici';
import type { File, Agent, FormData } from 'undici';
import type { FilePropertyBag } from 'formdata-node';
import { FormDataEncoder } from 'form-data-encoder';
import { ReadStream as FsReadStream } from 'node:fs';
import { Readable } from 'node:stream';
import { ReadableStream } from 'node:stream/web';
import { Blob } from 'node:buffer';
import { type RequestOptions } from '../core';
import { MultipartBody } from './MultipartBody';
import { type Shims } from './registry';

// @ts-ignore (this package does not have proper export maps for this export)
import { ReadableStream } from 'web-streams-polyfill/dist/ponyfill.es2018.js';

type FileFromPathOptions = Omit<FilePropertyBag, 'lastModified'>;

let fileFromPathWarned = false;
Expand All @@ -40,11 +36,11 @@ async function fileFromPath(path: string, ...args: any[]): Promise<File> {
return await _fileFromPath(path, ...args);
}

const defaultHttpAgent: Agent = new KeepAliveAgent({ keepAlive: true, timeout: 5 * 60 * 1000 });
const defaultHttpsAgent: Agent = new KeepAliveAgent.HttpsAgent({ keepAlive: true, timeout: 5 * 60 * 1000 });
const defaultHttpAgent = new uf.Agent({ keepAliveTimeout: 5 * 60 * 1000 });
const defaultHttpsAgent = new uf.Agent({ keepAliveTimeout: 5 * 60 * 1000 });
Copy link
Collaborator

Choose a reason for hiding this comment

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

Is this necessary to get reasonable keepAlive behavior? IIRC, node-fetch didn't do keepAlive by default but if undici does, maybe we don't need to construct a default agent after all?

(note we also have weird timeout shenanigans elsewhere to bump the agent's timeout so that a long timeout for a given request isn't cut short by the agent's timeout; not sure whether that'd still be necessary or would need adjustment.)

Copy link
Author

Choose a reason for hiding this comment

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

I'd have to go look at the default agent timeout to be sure

Copy link

Choose a reason for hiding this comment

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

FYI: undici will honor the keep-alive hint from the server.


async function getMultipartRequestOptions<T extends {} = Record<string, unknown>>(
form: fd.FormData,
form: FormData,
opts: RequestOptions<T>,
): Promise<RequestOptions<T>> {
const encoder = new FormDataEncoder(form);
Expand All @@ -67,13 +63,13 @@ export function getRuntime(): Shims {
}
return {
kind: 'node',
fetch: nf.default,
Request: nf.Request,
Response: nf.Response,
Headers: nf.Headers,
FormData: fd.FormData,
Blob: fd.Blob,
File: fd.File,
fetch: uf.fetch,
Request: uf.Request,
Response: uf.Response,
Headers: uf.Headers,
FormData: uf.FormData,
Blob: Blob,
File: uf.File,
ReadableStream,
getMultipartRequestOptions,
getDefaultAgent: (url: string): Agent => (url.startsWith('https') ? defaultHttpsAgent : defaultHttpAgent),
Expand Down
39 changes: 19 additions & 20 deletions src/_shims/node-types.d.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,26 +1,25 @@
/**
* Disclaimer: modules in _shims aren't intended to be imported by SDK users.
*/
import * as nf from 'node-fetch';
import * as fd from 'formdata-node';
import undici from 'undici';

export { type Agent } from 'node:http';
export { type Readable } from 'node:stream';
export { type ReadableStream } from 'node:stream/web';
export { type ReadStream as FsReadStream } from 'node:fs';
export { ReadableStream } from 'web-streams-polyfill';
import { Blob } from 'node:buffer';

export const fetch: typeof nf.default;
export const fetch: typeof undici.fetch;

export type Request = nf.Request;
export type RequestInfo = nf.RequestInfo;
export type RequestInit = nf.RequestInit;
export type Request = undici.Request;
export type RequestInfo = undici.RequestInfo;
export type RequestInit = undici.RequestInit;

export type Response = nf.Response;
export type ResponseInit = nf.ResponseInit;
export type ResponseType = nf.ResponseType;
export type BodyInit = nf.BodyInit;
export type Headers = nf.Headers;
export type HeadersInit = nf.HeadersInit;
export type Response = undici.Response;
export type ResponseInit = undici.ResponseInit;
export type ResponseType = undici.ResponseType;
export type BodyInit = undici.BodyInit;
export type Headers = undici.Headers;
export type HeadersInit = undici.HeadersInit;

type EndingType = 'native' | 'transparent';
export interface BlobPropertyBag {
Expand All @@ -34,9 +33,9 @@ export interface FilePropertyBag extends BlobPropertyBag {

export type FileFromPathOptions = Omit<FilePropertyBag, 'lastModified'>;

export type FormData = fd.FormData;
export const FormData: typeof fd.FormData;
export type File = fd.File;
export const File: typeof fd.File;
export type Blob = fd.Blob;
export const Blob: typeof fd.Blob;
export type FormData = undici.FormData;
export const FormData: typeof undici.FormData;
export type File = undici.File;
export const File: typeof undici.File;
export type Blob = Blob;
export const Blob: typeof Blob;
2 changes: 0 additions & 2 deletions src/core.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -319,8 +319,6 @@ export abstract class APIClient {
...(body && { body: body as any }),
headers: reqHeaders,
...(httpAgent && { agent: httpAgent }),
// @ts-ignore node-fetch uses a custom AbortSignal type that is
// not compatible with standard web types
signal: options.signal ?? null,
};

Expand Down
2 changes: 1 addition & 1 deletion src/index.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -43,7 +43,7 @@ export interface ClientOptions {
/**
* Specify a custom `fetch` function implementation.
*
* If not provided, we use `node-fetch` on Node.js and otherwise expect that `fetch` is
* If not provided, we use `undici` on Node.js and otherwise expect that `fetch` is
* defined globally.
*/
fetch?: Core.Fetch | undefined;
Expand Down
4 changes: 2 additions & 2 deletions src/uploads.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -26,7 +26,7 @@ export type BlobPart = string | ArrayBuffer | ArrayBufferView | Blob | Uint8Arra
export type Uploadable = FileLike | ResponseLike | FsReadStream;

/**
* Intended to match web.Blob, node.Blob, node-fetch.Blob, etc.
* Intended to match web.Blob, node.Blob, undici.Blob, etc.
*/
export interface BlobLike {
/** [MDN Reference](https://developer.mozilla.org/docs/Web/API/Blob/size) */
Expand Down Expand Up @@ -125,7 +125,7 @@ export async function toFile(
}
}

return new File(bits, name, options);
return new File(bits as (string | Blob | NodeJS.ArrayBufferView)[], name, options);
Copy link
Author

Choose a reason for hiding this comment

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

I couldn't for the life of me figure out how to get this type to work. Help appreciated!

Copy link
Collaborator

Choose a reason for hiding this comment

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

heh, I believe it, can try to take a look soon… likely thursday…

Copy link

Choose a reason for hiding this comment

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

@Ethan-Arrowood I think undici's type for the fileBits parameter is wrong, their own JSDoc says it accepts ArrayBuffers, but the type annotation only accepts ArrayBufferViews, which ArrayBuffers aren't assignable to.

}

async function getBytes(value: ToFileInput): Promise<Array<BlobPart>> {
Expand Down
Loading
Loading