Switch from `node-fetch` to `undici` #402

Ethan-Arrowood · 2023-10-24T17:27:05Z

Closes #392

rattrayalex · 2023-10-24T20:03:46Z

src/_shims/node-runtime.ts

-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 });


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.)

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

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

rattrayalex

This is looking clean & great so far! Thank you @Ethan-Arrowood !

rattrayalex · 2023-10-25T16:03:47Z

One thing to note is that with undici, it looks like response.body isn't a Node Readable stream, which would be breaking change for node-fetch users… any thoughts there?

Ethan-Arrowood · 2023-11-07T18:13:38Z

src/uploads.ts

@@ -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);


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

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

@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.

Ethan-Arrowood · 2023-11-07T18:19:39Z

One thing to note is that with undici, it looks like response.body isn't a Node Readable stream, which would be breaking change for node-fetch users… any thoughts there?

Yes this an important distinction. It will be a Node.js Web Stream instead. This is to spec so that code is actually interoperable. node-fetch has been a cause of much frustration for a very long time because they don't do this. Developers using fetch - in any context - should expect it to behave as specified

rattrayalex · 2023-11-08T05:58:10Z

This is to spec so that code is actually interoperable.

You mean, interoperable with web streams, not Node ones, right? So it will be breaking? Is there any way we can ease that migration for developers?

Ethan-Arrowood · 2023-11-08T16:39:32Z

Current, interoperable with web streams. Also meaning that when you use it in code that runs in both browser and on the server it behaves the same.

There does exist some utilities for users to transform one to the other. And they are fairly close in implementation so some users won't even notice a difference. The key here is that this breaking change is intentional. Understand if you want that to happen in a major version of this library. Otherwise, if this library tried to do something else you'd just be kicking the can down the road. True interoperability should not have anything other than Web Streams. If true interoperability isn't the absolute goal, we could add a transform to this library and keep Node users on Node streams.

rattrayalex · 2023-11-09T07:45:39Z

I'm probably leaning towards a major version bump in that case. I imagine shimming a web ReadableStream to also be compatible with a node Readable stream would be pretty gross. But if there is a library that does that sanely, it'd make the rollout much easier. For what it's worth I doubt a huge number of people are likely to be relying on the raw response so far, so I doubt it'll be a very painful major version.

…

On Wed, Nov 8 2023 at 8:39 AM, Ethan Arrowood ***@***.***> wrote: Current, interoperable with web streams. Also meaning that when you use it in code that runs in both browser and on the server it behaves the same. There does exist some utilities for users to transform one to the other. And they are fairly close in implementation so some users won't even notice a difference. The key here is that this breaking change is intentional. Understand if you want that to happen in a major version of this library. Otherwise, if this library tried to do something else you'd just be kicking the can down the road. True interoperability should not have anything other than Web Streams. If true interoperability isn't the absolute goal, we could add a transform to this library and keep Node users on Node streams. — Reply to this email directly, view it on GitHub <#402 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AAFL6LT2IDSS6VVMHH5UCI3YDOYVBAVCNFSM6AAAAAA6OBVVX6VHI2DSMVQWIX3LMV43OSLTON2WKQ3PNVWWK3TUHMYTQMBSGI3DCOJWGU> . You are receiving this because you commented.Message ID: ***@***.***>

rattrayalex · 2023-11-13T03:35:40Z

Hey, sorry, just checking in – are you blocked on me here?

Ethan-Arrowood · 2023-11-13T15:42:44Z

No - haven't gotten back around to this. Need to still figure out how to run tests (still haven't created an OpenAI key). Do you have CI you could enable for this PR? I'll update and mark it "ready for review" so GitHub prompts it too.

rattrayalex · 2023-11-14T03:46:41Z

Sadly we don't run CI in public for this repo yet, just internally.

The steps to test it locally are:

yarn
OPENAI_API_KEY=… yarn ts-node ecosystem-tests/cli.ts
npx prism mock https://github.com/openai/openai-openapi/raw/master/openapi.yaml & # or in a separate tab.
yarn test

Ethan-Arrowood · 2023-11-17T19:55:06Z

Updated failing types I believe. Still cannot run tests. Not sure what's going wrong. Maybe my local env is in a bad state. I think this is very close, maybe a maintainer more familiar with the shim and test matrix can take it over the finish line.

The most important detail here is that Node.js now has an actual spec compliant Fetch implementation. Its types are different and most importantly it now returns Web Streams instead of Node.js Streams (spec compliant).

This is more expected IMO as fetch should do as fetch is documented to do. Node.js makes available a Readable.fromWeb() that can be used to convert the web streams back to node streams if needed.

rattrayalex · 2023-11-18T22:56:22Z

Amazing, thank you so much @Ethan-Arrowood ! Finishing off those tests ourselves sounds totally fair; we have a lot on our plate right not but will try to take a look at getting this over the line soon.

athyuttamre · 2023-11-19T02:08:06Z

I've approved running the CI workflow. Looks like it is failing with a module error:

yarn install v1.22.19
[1/4] Resolving packages...
[2/4] Fetching packages...
error [email protected]: The engine "node" is incompatible with this module. Expected version ">= 18". Got "1[6](https://github.com/openai/openai-node/actions/runs/6908472736/job/18819218282?pr=402#step:4:7).20.2"
error Found incompatible module.
info Visit https://yarnpkg.com/en/docs/cli/install for documentation about this command.
Error: Process completed with exit code 1.

https://github.com/openai/openai-node/actions/runs/6908472736/job/18819218282?pr=402

Perhaps we need to update the workflow to use Node 18 rather than 16?

https://github.com/openai/openai-node/blob/master/.github/workflows/ci.yml

athyuttamre · 2023-11-28T00:57:42Z

@Ethan-Arrowood can you rebase on master so that the latest CI workflow can be run with Node 18?

athyuttamre · 2023-11-28T21:39:23Z

Looks like CI now runs correctly! Some type errors to fix next it looks like.

Ethan-Arrowood · 2023-11-28T21:48:51Z

Why are they all coming from the dist folder?

rattrayalex · 2023-11-29T00:45:10Z

Thanks Atty!

I think @jedwards1211 is working on these type errors etc – Ethan, you shouldn't need to take a look.

mmo80 · 2024-03-22T12:51:09Z

When will this be merged into master? The DeprecationWarning: The `punycode` module is deprecated. Please use a userland alternative instead. warning is so annoying. Everybody that uses langchain gets this warning.

rattrayalex · 2024-03-24T03:22:10Z

Sorry for the annoyance.

We're hoping to merge this within the next 6-8 weeks.

It will be a breaking change and there are some deep complexities, which is why it'll take longer than one might hope, but we are actively working on it behind the scenes.

wojtekmaj · 2024-04-25T09:23:04Z

ecosystem-tests/node-ts-cjs/tests/test-node.ts

@@ -67,10 +68,11 @@ it(`raw response`, async function () {

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


This comment probably needs updating

rattrayalex reviewed Oct 24, 2023

View reviewed changes

This was referenced Nov 3, 2023

The official openai package no longer uses axios dexaai/openai-fetch#34

Closed

Version 2.0 dexaai/openai-fetch#32

Merged

Ethan-Arrowood commented Nov 7, 2023

View reviewed changes

Ethan-Arrowood marked this pull request as ready for review November 13, 2023 15:42

Ethan-Arrowood requested review from schnerd, athyuttamre and logankilpatrick as code owners November 13, 2023 15:42

athyuttamre changed the base branch from master to next November 14, 2023 19:40

athyuttamre changed the base branch from next to master November 14, 2023 19:41

This was referenced Nov 22, 2023

Allow readable streams to be uploaded #524

Closed

The punycode module is deprecated in Node.js 21 (type = module) #527

Closed

athyuttamre mentioned this pull request Nov 27, 2023

[github] Update Node version to 18 in workflows #539

Merged

Ethan-Arrowood force-pushed the use-undici branch from ff0be7c to 1a1de82 Compare November 28, 2023 21:16

Replace node-fetch with undici fetch

c2e41fa

Ethan-Arrowood force-pushed the use-undici branch from 1a1de82 to c2e41fa Compare November 28, 2023 23:25

athyuttamre removed request for schnerd and logankilpatrick December 8, 2023 22:24

athyuttamre removed their request for review December 21, 2023 17:08

rattrayalex mentioned this pull request Jan 10, 2024

Remove web-streams-polyfill and abort controller polyfill #614

Closed

rattrayalex mentioned this pull request Feb 13, 2024

Stop throwing when fetch isnt detected in the environment #666

Closed

1 task

rattrayalex mentioned this pull request Mar 24, 2024

Pkg loading wrong runtime.js #734

Closed

1 task

dgellow mentioned this pull request Apr 1, 2024

Proposed changed to update punycode anthropics/anthropic-sdk-typescript#366

Closed

marck283 mentioned this pull request Apr 18, 2024

DeprecationWarning: The punycode module is deprecated. #780

Closed

1 task

johnmarcou mentioned this pull request Apr 19, 2024

Punycode is deprecated MinaFoundation/gptSurveySummarizer#23

Open

wojtekmaj reviewed Apr 25, 2024

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Switch from `node-fetch` to `undici` #402

Switch from `node-fetch` to `undici` #402

Ethan-Arrowood commented Oct 24, 2023

rattrayalex Oct 24, 2023

Ethan-Arrowood Oct 24, 2023

ronag Oct 25, 2023

rattrayalex left a comment

rattrayalex commented Oct 25, 2023

Ethan-Arrowood Nov 7, 2023

rattrayalex Nov 8, 2023

jedwards1211 Nov 26, 2023

Ethan-Arrowood commented Nov 7, 2023

rattrayalex commented Nov 8, 2023

Ethan-Arrowood commented Nov 8, 2023

rattrayalex commented Nov 9, 2023 via email

rattrayalex commented Nov 13, 2023

Ethan-Arrowood commented Nov 13, 2023

rattrayalex commented Nov 14, 2023

Ethan-Arrowood commented Nov 17, 2023

rattrayalex commented Nov 18, 2023

athyuttamre commented Nov 19, 2023 •

edited

athyuttamre commented Nov 28, 2023

athyuttamre commented Nov 28, 2023

Ethan-Arrowood commented Nov 28, 2023

rattrayalex commented Nov 29, 2023

mmo80 commented Mar 22, 2024

rattrayalex commented Mar 24, 2024

wojtekmaj Apr 25, 2024

		@@ -67,10 +68,11 @@ it(`raw response`, async function () {

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

Switch from node-fetch to undici #402

Are you sure you want to change the base?

Switch from node-fetch to undici #402

Conversation

Ethan-Arrowood commented Oct 24, 2023

rattrayalex Oct 24, 2023

Choose a reason for hiding this comment

Ethan-Arrowood Oct 24, 2023

Choose a reason for hiding this comment

ronag Oct 25, 2023

Choose a reason for hiding this comment

rattrayalex left a comment

Choose a reason for hiding this comment

rattrayalex commented Oct 25, 2023

Ethan-Arrowood Nov 7, 2023

Choose a reason for hiding this comment

rattrayalex Nov 8, 2023

Choose a reason for hiding this comment

jedwards1211 Nov 26, 2023

Choose a reason for hiding this comment

Ethan-Arrowood commented Nov 7, 2023

rattrayalex commented Nov 8, 2023

Ethan-Arrowood commented Nov 8, 2023

rattrayalex commented Nov 9, 2023 via email

rattrayalex commented Nov 13, 2023

Ethan-Arrowood commented Nov 13, 2023

rattrayalex commented Nov 14, 2023

Ethan-Arrowood commented Nov 17, 2023

rattrayalex commented Nov 18, 2023

athyuttamre commented Nov 19, 2023 • edited

athyuttamre commented Nov 28, 2023

athyuttamre commented Nov 28, 2023

Ethan-Arrowood commented Nov 28, 2023

rattrayalex commented Nov 29, 2023

mmo80 commented Mar 22, 2024

rattrayalex commented Mar 24, 2024

wojtekmaj Apr 25, 2024

Choose a reason for hiding this comment

Switch from `node-fetch` to `undici` #402

Switch from `node-fetch` to `undici` #402

athyuttamre commented Nov 19, 2023 •

edited