-
Notifications
You must be signed in to change notification settings - Fork 3
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: dropping support for node 16 #421
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
took me a while to digest this (see some of my real time reaction in the comments) but overall LGTM! can't wait until this repo will start needing test suites for other runtime environments like bun + deno 😵💫
"optionalDependencies": { | ||
"formdata-node": "^4.3.2" | ||
"readable-stream": "^3.6.0", | ||
"undici": "^5.24.0" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
aren't you only pulling types from undici
? could this be a dev-dep?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
No it's pulling the File
API out of undici
if we don't have it -- which will be all Node environments because native fetch doesn't ship that. But it ships a global FormData
? 🤷
} | ||
} | ||
|
||
if (!globalThis.File) { | ||
try { | ||
// eslint-disable-next-line @typescript-eslint/no-var-requires, import/no-extraneous-dependencies | ||
globalThis.File = require('formdata-node').File; | ||
// Node's native `fetch` implementation unfortunately does not make this API global so we need |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ah damn wait i think you answered this question i had lol — kinda confusing how undici
is a separate npm package
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
why weren't browser tests working in this file?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I commented them out when I did the Vitest migration a few weeks back because I couldn't get @vitest/browser
working and didn't feel at the time like debugging why.
}, | ||
"optionalDependencies": { | ||
"formdata-node": "^4.3.2" | ||
"readable-stream": "^3.6.0", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
there's no way to conditionally load this right?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Could do an await import()
but I think even if this source gets compiled by Webpack it would still have to load it in the compiled source.
🧰 Changes
Now that Node 16 is hitting EOL today1 we can now drop support for it and all of the quirky behavior that we've had to introduce to support non-native
fetch
solutions through node-fetch, form-data, and formdata-encoder.This library now just supports pure
fetch
. 🐬Footnotes
https://endoflife.date/nodejs ↩