feat: use undici as fetch polyfill #9106
Conversation
|
ee3dec9 to
1e767c1
Compare
feat: use native node web stream
There was a problem hiding this comment.
this looks pretty sweet! 🙌 Nice work!
One small question: It seems like in addition to installing the fetch family in our polyfill, we should also install ReadableStream and WritableStream, as well as any other stream classes provided by undici. Do you agree @jacob-ebey ?
|
|
||
| installGlobals(); | ||
| require("@remix-run/node").installGlobals(); |
There was a problem hiding this comment.
It seems like installGlobals should also install TextDecoder, TextEncoder, ReadableStream, and WritableStream, no?
There was a problem hiding this comment.
This is a JSDom issue here.
|
Keep an eye out for this bug. I moved to Undici on a project and had to switch off because the API I was working with wasn't including a content-length. |
I added a test against this and it appears to have been fixed at some point: nodejs/undici#2995 |
Use undici as our fetch polyfill going forward. This will allow us to firstly (and my my favorite part), stop maintaining our own, and secondly and arguably more important, will allow us to smoothly upgrade our users to the Node.js fetch when they are on a node 20+ as they will already be using it via the polyfill. Undici is the node fetch implementation, and now all our installGlobals() will be doing is giving you the latest version regardless of your node version, and I think that's pretty sweet.
Closes: #
Testing Strategy: