fix: Remove the default connection close header.

[dhedey]

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)

---

• fix "socket hang up" / ECONNRESET on consecutive requests with Node.js 19 and Node.js 20 #1735
• replaces fix: Use keep-alive by default for Connection header. #1473

[LinusU]

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]

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]

@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]

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

[matthewkeil]

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

[github-actions]

🎉 This PR is included in version 3.3.2 🎉

The release is available on:

• npm package (@latest dist-tag)
• GitHub release

Your semantic-release bot 📦🚀

[matthewkeil]

Thank you @jimmywarting and @dhedey 🎉

[nflaig]

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]

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]

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]

@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:

• Note that the Connection Close isn't included if an agent is provided, which has seen no reported issues.
• I don't believe Connection: Close was ever needed. Even on eg Node v4 docs for the default agent, it doesn't mention sending a Connection: Close header: https://nodejs.org/docs/latest-v4.x/api/http.html#http_http_globalagent.

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).

[jimmywarting]

If you feel like backporting it then go for it

[dhedey]

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

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

Thanks again!