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.

Sign up for GitHub

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

Jump to bottom

feat!: Upgrade to node-fetch v3 #617

Merged
merged 28 commits into from
Oct 25, 2024
Merged

feat!: Upgrade to node-fetch v3 #617

merged 28 commits into from
Oct 25, 2024

Conversation

danielbankhead
Copy link
Contributor

@danielbankhead danielbankhead commented Apr 16, 2024 •
edited
Loading

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
@generated-files-bot Generated Files Bot
Copy link

generated-files-bot bot commented Apr 16, 2024 •
edited
Loading

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

@danielbankhead danielbankhead mentioned this pull request Apr 16, 2024
Closed
@danielbankhead
Copy link
Contributor 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
ddelgrosso1
ddelgrosso1 reviewed Apr 17, 2024
View reviewed changes
test/test.getch.ts Outdated Show resolved Hide resolved
@ddelgrosso1
Copy link
Contributor

ddelgrosso1 commented Apr 17, 2024 •
edited
Loading

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
Contributor 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
Contributor 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
danielbankhead
danielbankhead commented Apr 19, 2024
View reviewed changes
src/gaxios.ts Outdated Show resolved Hide resolved
@danielbankhead
Copy link
Contributor Author

Node 18 & 20 pass in this commit 👌🏽:

@Kurre Kurre mentioned this pull request May 8, 2024
Closed
@danielbankhead danielbankhead added semver: major Hint for users that this is an API breaking change. next major: breaking change this is a change that we should wait to bundle into the next major version labels May 15, 2024
@danielbankhead danielbankhead mentioned this pull request Jul 2, 2024
Merged
@product-auto-label product-auto-label bot added size: xl Pull request size is extra large. and removed size: l Pull request size is large. labels Sep 11, 2024
@danielbankhead danielbankhead requested a review from a team as a code owner October 24, 2024 23:20
@danielbankhead danielbankhead removed the do not merge Indicates a pull request not ready for merge, due to either quality or timing. label Oct 24, 2024
@andyconlin andyconlin mentioned this pull request Oct 25, 2024
Open
@danielbankhead danielbankhead merged commit 33702e6 into main Oct 25, 2024
14 checks passed
@danielbankhead danielbankhead deleted the upgrade-node-fetch branch October 25, 2024 17:02
@release-please release-please bot mentioned this pull request Oct 25, 2024
Open
@andyconlin
Copy link

Thanks for your work on this @danielbankhead and @ddelgrosso1!

@nwalters512 nwalters512 mentioned this pull request Oct 29, 2024
Open
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@ddelgrosso1 ddelgrosso1 ddelgrosso1 approved these changes

Assignees
No one assigned
Labels
next major: breaking change this is a change that we should wait to bundle into the next major version semver: major Hint for users that this is an API breaking change. size: xl Pull request size is extra large.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Upgrade node-fetch dependency to v3.2 ESM
4 participants
@danielbankhead @ddelgrosso1 @andyconlin @d-goog