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

fix: Remove the default connection close header. #1736

Merged
merged 1 commit into from
Jul 25, 2023

Conversation

dhedey
Copy link
Contributor

@dhedey dhedey commented Apr 21, 2023

Purpose

This fixes #1735 and likely replaces #1473.

Changes

Removes the behaviour to add a Connection: close header. This was causing issues in Node.js 19+ (see #1735), but possibly subtle issues in earlier Node.js too. Note that we still expect some users to use node-fetch in Node.js 19+ because enabling fetch requires an --experimental-fetch command line parameter passed to Node.js and some users (such as people consuming my library which requires a fetch implementation) require it.

Instead, we rely on the underlying http implementation in Node.js to handle this, as per the documentation at
https://nodejs.org/api/http.html#new-agentoptions

The original change introducing this provided no clear motivation for providing the header, and the implementation has since been changed to disable this header when an agent is provided, so I think there is sufficient evidence that removing this header is the correct behaviour.

If this change is accepted, I'm happy to back-port this into the 2.x branch, as I know some users of my library use 2.x because they can't use ESM.

Additional information

See #1735 for background.


  • I updated the readme
    (I didn't add unit tests because the behaviour now depends the presence of the header now depends on the default http agent behaviour, and therefore on the Node.js version being used and I didn't want to introduce that dependency into the tests)

Instead, we rely on the underlying http implementation in Node.js
to handle this, as per the documentation at
https://nodejs.org/api/http.html#new-agentoptions

This fixes node-fetch#1735 and likely replaces node-fetch#1473

The original change introducing this provided no clear motivation
for the override, and the implementation has since been changed to
disable this header when an agent is provided, so I think there
is sufficient evidence that removing this is the correct behaviour.
node-fetch@af21ae6
node-fetch@7f68577
Copy link
Member

@LinusU LinusU left a comment

Choose a reason for hiding this comment

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

Since this should only affect newer versions of Node.js which previously didn't work correctly, I don't think that this would constitute a major version bump.

LGTM 👍

@dhedey
Copy link
Contributor Author

dhedey commented May 19, 2023

Thanks @LinusU 👍

Is there anything more I can do to help get this merged? Would it help if I raised a separate PR to backport this to the 2.x branch now?

@LinusU
Copy link
Member

LinusU commented May 22, 2023

@jimmywarting the failing tests doesn't seem related to this PR, they fail for me locally as well. Could you take a look and possible merge and release?

@LinusU
Copy link
Member

LinusU commented May 22, 2023

@dhedey sorry for the delay, nothing more should be required from your side!

@matthewkeil
Copy link

Hi there. We are end users through cross-fetch having issues upgrading our system from Node 18 -> 20. This PR will ultimately help us get updated. We are curious if there is anything we can assist with to help @dhedey get this across the finish line. He did an excellent job researching and we are super grateful for his help!!! Please let us know as we eagerly await this merge

@jimmywarting jimmywarting merged commit 8b3320d into node-fetch:main Jul 25, 2023
@github-actions
Copy link

🎉 This PR is included in version 3.3.2 🎉

The release is available on:

Your semantic-release bot 📦🚀

@matthewkeil
Copy link

Thank you @jimmywarting and @dhedey 🎉

@nflaig
Copy link

nflaig commented Jul 26, 2023

Awesome work guys! It has been really insightful to follow the discussions here and on the node issue. I am wondering if you also plan to backport this to 2.x branch, this would be really appreciated. I guess with more people updating to node 20 it is also quite important to do.

@jimmywarting
Copy link
Collaborator

Dont know about backporting to v2 is really necessary. It is mostly frezed and dose not really get any new feature updates. Only critical bug fixes and security related stuff.
Tried fixing a BOM text issue by using same text decoder in v2 but it broke som things cuz it was not really well backward compatible for older NodeJS versions. So it has to be reverted quickly.

Cjs user can still import esm version if they use async import instead.

const fetch = (...args) => import('node-fetch').then(({default: fetch}) => fetch(...args))

Or update to a node version that has fetch built in

@nflaig
Copy link

nflaig commented Jul 27, 2023

I understand your concerns and might be good to not touch v2, assuming those users are on older node version anyways. The problem is rather with cross-fetch not updating to node-fetch v3, but this has been proposed a while ago already lquixada/cross-fetch#146.

@dhedey
Copy link
Contributor Author

dhedey commented Jul 27, 2023

@jimmywarting

Can I start this by saying - I've used node-fetch a lot over the years, and really appreciate the hard work and stewardship of you and the other contributors to this project 👍.

This is a critical bug in my opinion, simply breaking many integrations when using node-fetch with more recent versions of node, so in my opinion is really deserving of a fix to the version 2 branch.

I believe this change is low-risk, for a couple of reasons:

I'm happy to put in that PR to version 2 if that's a help.

To add some flavour regarding attention to version 2 - the reality is that:

  • Libraries like ours choose node-fetch as a polyfill or fallback (or, more specifically, we provide documentation on how our users can use node-fetch), and such libraries can't choose to upgrade to v3 without forcing all of our users off cjs / onto ESM, which simply isn't something we are able or willing to do.
  • Looking at the version history, across v2 versions, v2 has over 10x the downloads of v3 in the last week: https://www.npmjs.com/package/node-fetch?activeTab=versions - 2.6.12 got 10,648,684
  • Why?
    • One of the primary reasons for still using node-fetch now is to support these older applications, or, importantly for building libraries which need to be compatible with different nodejs versions / build / packaging paradigms :). This tail-end of older applications (or, importantly, libraries!) needing compatibility is the main node-fetch user base (based on the numbers above). And libraries often can't control which versions of NodeJS they're run under.
    • I would imagine v3 hasn't really taken off because if people are able to use ES modules or using async import of fetch then in most cases, they will likely be building an application themselves (not a library) and can just use node's built in fetch.

It's worth noting that I tried just using const fetch = (...args) => import('node-fetch').then(({default: fetch}) => fetch(...args)) but importing an ESM module didn't work when I needed to support CJS builds for upstream users. Maybe I did something wrong - but I don't believe broadly that this approach works for libraries.

Don't get me wrong, I'm not trying to criticise v3 at all! But I just wanted to emphasise, that the main user base is still on v2, and in my opinion, almost certainly will always be - because only v2 really captures the USP of "why do I want to use node-fetch?" - which (in my experience) is either:

  • (A) To provide a fetch implementation for my application built on a legacy stack running an old version of node, for reasons outside of my control (grr managers / legacy systems). Such applications are unlikely to be able to support ESM. The workaround above might be possible with some installations/bundlers, but is sufficiently hard to find/understand that many devs I know would just stick on v2 which "seems to work out of the box". OR
  • (B) To provide a universally runnable fetch implementation for a library capable of running in different node/build configurations, many of which don't support ESM or have fetch enabled

And v3 doesn't work so well for either (A) or (B), so most projects are still running v2 (or would migrate off to node's built in fetch if they upgraded).

dhedey added a commit to dhedey/node-fetch that referenced this pull request Jul 27, 2023
Instead, we rely on the underlying http implementation in Node.js
to handle this, as per the documentation at
https://nodejs.org/api/http.html#new-agentoptions

This fixes node-fetch#1735 and likely replaces node-fetch#1473

The original change introducing this provided no clear motivation
for the override, and the implementation has since been changed to
disable this header when an agent is provided, so I think there
is sufficient evidence that removing this is the correct behaviour.
node-fetch@af21ae6
node-fetch@7f68577

This commit is backported to the v2 branch from node-fetch#1736 against v3.
@jimmywarting
Copy link
Collaborator

If you feel like backporting it then go for it

@dhedey
Copy link
Contributor Author

dhedey commented Jul 27, 2023

Fantastic, thanks @jimmywarting - Appreciate your support on this.

I've raised the PR here: #1765 - over to you to approve and release.

Thanks again!

jimmywarting pushed a commit that referenced this pull request Aug 18, 2023
Instead, we rely on the underlying http implementation in Node.js
to handle this, as per the documentation at
https://nodejs.org/api/http.html#new-agentoptions

This fixes #1735 and likely replaces #1473

The original change introducing this provided no clear motivation
for the override, and the implementation has since been changed to
disable this header when an agent is provided, so I think there
is sufficient evidence that removing this is the correct behaviour.
af21ae6
7f68577

This commit is backported to the v2 branch from #1736 against v3.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

"socket hang up" / ECONNRESET on consecutive requests with Node.js 19 and Node.js 20
5 participants