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.

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!: Upgrade to node-fetch v3 #617

Open
wants to merge 7 commits into
base: main
Choose a base branch
from
Open

Conversation

danielbankhead
Copy link
Member

@danielbankhead danielbankhead commented Apr 16, 2024

Fixes #429
Fixes #508

🦕

@danielbankhead danielbankhead requested a review from a team as a code owner April 16, 2024 22:29
@product-auto-label product-auto-label bot added the size: m Pull request size is medium. label Apr 16, 2024
Copy link

generated-files-bot bot commented Apr 16, 2024

Warning: This pull request is touching the following templated files:

@danielbankhead danielbankhead mentioned this pull request Apr 16, 2024
@danielbankhead
Copy link
Member Author

We'll merge this once we're ready to drop Node 14/16 (which is WIP)

@danielbankhead danielbankhead added the do not merge Indicates a pull request not ready for merge, due to either quality or timing. label Apr 16, 2024
test/test.getch.ts Outdated Show resolved Hide resolved
@ddelgrosso1
Copy link
Contributor

ddelgrosso1 commented Apr 17, 2024

General question / comment I know that both fetch and FormData are marked experimental stability in Node 18 but does it make more sense to free ourselves from node-fetch as opposed to upgrading? Edit: looks like it is marked stable as of Node 21.

@danielbankhead
Copy link
Member Author

General question / comment I know that both fetch and FormData are marked experimental stability in Node 18 but does it make more sense to free ourselves from node-fetch as opposed to upgrading? Edit: looks like it is marked stable as of Node 21.

I was a bit torn on this decision; technically it would be experimental, however the API surface hasn't really changed since its introduction. The catch is the experimental warnings for customers below v18.13 (which we'd likely have to support given engines would be >=18.0.0). However, I would be surprised if a lot of folks are actually using an old version of 18, without any security patches/minor features whatsoever - I would suspect those pre-LTS early adopters would've upgraded by now.

Outside from that, internally we would need to use a beta version of nock, which includes unidici support - it seems to work fine in testing.

@ddelgrosso1
Copy link
Contributor

Ah I hadn't even considered nock not supporting it. I wonder if by the time we are ready to move to Node 18 nock will be out of the beta phase.

@danielbankhead
Copy link
Member Author

Created a proof-of-concept for migrating to fetch, along with current limitations:

@product-auto-label product-auto-label bot added size: l Pull request size is large. and removed size: m Pull request size is medium. labels Apr 19, 2024
Object.assign(res, {config});

// best effort for `node-fetch`; `.data` is not writable...
return new Proxy(res, {
Copy link
Member Author

Choose a reason for hiding this comment

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

There's gotta be a cleaner way to do this... hmm. We want to expose all of res's types so that customers can take advantage of new properties as they're available

@danielbankhead
Copy link
Member Author

Node 18 & 20 pass in this commit 👌🏽:

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
do not merge Indicates a pull request not ready for merge, due to either quality or timing. size: l Pull request size is large.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Upgrade node-fetch dependency to v3.2 ESM
2 participants