chore: replace node-fetch with undici
#348
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
🦋 Changeset detectedLatest commit: be2c20d The changes in this PR will be included in the next version bump. This PR includes changesets to release 1 package
Not sure what this means? Click here to learn what changesets are. Click here if you're a maintainer who wants to add another changeset to this PR |
threepointone
force-pushed
the
add-undici
branch
2 times, most recently
from
6b4a5e6
to
1ac3642
Compare
threepointone
changed the title
node-fetch with undicinode-fetch with undici
|
I get a little nervous with reviews like this because pages doesn't have test coverage. @GregBrimble I don't expect anything to break by upping |
threepointone
commented
Jan 31, 2022
| @@ -12,7 +12,13 @@ | |||
| "jsx": "react", | |||
| "resolveJsonModule": true, | |||
| "incremental": true, | |||
| "strictNullChecks": true | |||
| "strictNullChecks": true, | |||
| "skipLibCheck": true | |||
There was a problem hiding this comment.
I assume this will be the controversial part of this PR, so I'm calling it out right now. Have I misunderstood how this flag works, and does it make our codebase worse?
There was a problem hiding this comment.
petebacondarwin
approved these changes
Jan 31, 2022
threepointone
force-pushed
the
add-undici
branch
from
1ac3642
to
870d1b2
Compare
There are several reasons to replace `node-fetch` with `undici`:
- `undici`'s `fetch()` implementation is set to become node's standard `fetch()` implementation, which means we can just remove the dependency in the future (or optionally load it depending on which version of node is being used)
- `node-fetch` pollutes the global type space with a number of standard types
- we already bundle `undici` via `miniflare`/pages, so this means our bundle size could ostensibly become smaller.
This replaces `node-fetch` with `undici`.
- All instances of `import fetch from "node-fetch"` are replaced with `import {fetch} from "undici"`
- `undici` also comes with spec compliant forms of `FormData` and `File`, so we could also remove `formdata-node` in `form_data.ts`
- All the global types that were injected by `node-fetch` are now imported from `undici` (as well as some mistaken ones from `node:url`)
- NOTE: this also turns on `skipLibCheck` in `tsconfig.json`. Some dependencies oddly depend on browser globals like `Request`, `Response` (like `@miniflare/core`, `jest-fetch-mock`, etc), which now fail because `node-fetch` isn't injecting those globals anymore. So we enable `skipLibCheck` to bypass them. (I'd thought `skipLibCheck` completely ignores 'third party' types, but that's not true - it still uses the module graph to scan types. So we're still typesafe. We should enable `strict` sometime to avoid `any`s, but that's for later.)
- The bundle size isn't smaller because we're bundling 2 different versions of `undici`, but we'll fix that by separately upping the version of `undici` that miniflare bundles.
threepointone
force-pushed
the
add-undici
branch
from
870d1b2
to
be2c20d
Compare
Merged
This was referenced Nov 23, 2023
This was referenced Dec 1, 2023
This was referenced Jan 9, 2024
This was referenced Jan 17, 2024
This was referenced Jan 25, 2024
This was referenced Feb 1, 2024
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
There are several reasons to replace
node-fetchwithundici:undici'sfetch()implementation is set to become node's standardfetch()implementation, which means we can just remove the dependency in the future (or optionally load it depending on which version of node is being used)node-fetchpollutes the global type space with a number of standard typesundiciviaminiflare/pages, so this means our bundle size could ostensibly become smaller.This replaces
node-fetchwithundici.import fetch from "node-fetch"are replaced withimport {fetch} from "undici"undicialso comes with spec compliant forms ofFormDataandFile, so we could also removeformdata-nodeinform_data.tsnode-fetchare now imported fromundici(as well as some mistaken ones fromnode:url)skipLibCheckintsconfig.json. Some dependencies oddly depend on browser globals likeRequest,Response(like@miniflare/core,jest-fetch-mock, etc), which now fail becausenode-fetchisn't injecting those globals anymore. So we enableskipLibCheckto bypass them. (I'd thoughtskipLibCheckcompletely ignores 'third party' types, but that's not true - it still uses the module graph to scan types. So we're still typesafe. We should enablestrictsometime to avoidanys, but that's for later.)undici, but we'll fix that by separately upping the version ofundicithat miniflare bundles.