Revert "update make-fetch-happen to 11.0.3 (#2796)"

[legobeat]

This reverts commit 02480f6, as it introduced breaking changes blocking a release of node-gyp 9 from a clean main.

make-fetch-happen would have to be updated to a (hypothetical future) v10.x fix, or replaced with something else.

Checklist

• npm install && npm test passes
• tests are included
• documentation is changed or added
• commit message follows commit guidelines

Description of change

This reverts commit 02480f6, thereby rolling back dependency make-fetch-happen from ^11.0.3 to ^10.0.3.

The upgrade is breaking for node-fetch users as it has transitive
dependencies with syntax incompatible with supported Node.js versions.

Related:

• Sync deps and engines with npm #2770
• Using mocha for running some tests #2837
• chore: release 9.4.0 #2816
• subdependency regression breaks node-gyp 9 #2848
• feat!: update engines.node to ^14.17.0 || ^16.13.0 || >=18.0.0 #2827
• update make-fetch-happen to 11.0.3 #2796

[rvagg]

This seems reasonable given our "engines" claim (which is different to our CI versions, which isn't great). But doesn't this take us back to a version with a security vulnerability tag?

Comments @benjaminpjones, @lukekarrys, @cclauss?

[legobeat]

@rvagg It does, so currently addressing that would mean replacing the library. It's only a single call for the installation so shouldn't bee to hairy - a false start with got in #2850. perhaps it's more reasonable to inline a solution, thereby removing the need for introducing any new dependency here. I'd also consider https://github.com/paulmillr/micro-ftch.

FWIW the question of a backport was raised in npm/make-fetch-happen#243 and it doesn't seem to be happening under upstream, at least.

[rvagg]

or we could keep it in house with https://github.com/nodejs/undici perhaps?

[legobeat]

or we could keep it in house with https://github.com/nodejs/undici perhaps?

Hm, I'd be curious to hear maintainer's take on this one:

• node-gyp@latest supports node@^12.13
• undici@5.21.2 supports node >= 12.18
• undici@5.22.0 supports node >= 14.0 (breaking engines in semver-minor)
• undici@3.3.6 doesn't have any node-version restriction

So I guess:

• If maintainers are fine with breaking Node.js 12.13.*-12.17.* then we can pin at undici@5.21.2
• If following semver is a priority and undici 3 is acceptable, that seems straightforward. Would also be a clean transition towards native fetch once 16 can be dropped (node-gyp 11 or later, I assume)

[rvagg]

A mild dilemma, but IMO we should ditch engines entirely (in a semver-major probably), I'm not sure why we're still doing that other than to telegraph our contract, which we're not exactly sticking to! It might have been to sync with npm originally. I'd rather we ditch it and test our contract in CI than in package.json.

I think ^12.18 would be ok to switch, then we get to pull in the @nodejs/undici team whenever we have a problem as a bonus.

[lukekarrys]

Two things from my perspective:

• I think a backport of make-fetch-happen to "force" fix the vulnerabilities is appropriate here @legobeat. I know you already brought this up, but it's relatively little effort upstream for us so we should reconsider on in Backport non-breaking security-related fixes to v10 npm/make-fetch-happen#243. Obviously, this is irrelevant if make-fetch-happen is being dropped.
• @rvagg Ditching engines is difficult, because consumers are still likely to consider incompatible engine changes in transitive deps to be "breaking". This is an ecosystem issue, and difficult to work around in practice. In npm/cli we've settled on setting a fairly wide engines range of ^{LTS-1} || ^{LTS-2} || >={ACTIVE} (currently ^14.17.0 || ^16.13.0 || >=18.0.0). We have tooling that forces that to be our CI contract and tests all transitive deps fall within that range. Sync deps and engines with npm #2770 implements this strategy and IMO should be merged and released as a semver major.

[lukekarrys]

One more point on engines:

Anything wider than what npm/cli implements (eg ^12.18) would allow npm@9 to update to that upcoming version of node-gyp.

However as #2770 shows node-gyp uses a number of npm dependencies and we've implemented the same enginges range across all our supported packages. So landing #2770 would be problematic under that new engines range.

[legobeat]

Really appreciate the follow-up here @lukekarrys!

Ditching engines is difficult, because consumers are still likely to consider incompatible engine changes in transitive deps to be "breaking"

I'd remove the quotes. yarn 1.x is still widely used (I'd guess even more so among projects where this include-path is relevant) and will handle this as a fatal error by default:

The engines specify versions of clients that must be used with your package.

Other package managers or yarn versions may also, depending on configuration.

This is an ecosystem issue, and difficult to work around in practice.

My conclusion is that increasing engines.node is technically per definition breaking unless the project makes a qualifier indicating the intended interpretation or platform support (and then we're back with even less practical, and widely known and used mechanisms to do so. What I could imagine here with what's available today would be again engines to indicate package manager support, or package documentation. Neither is really sustainable).

More on engines

The above is not making an argument that it should be breaking or that there even exists a "correct" semantics of engines, just that in practice, it is. Separately from that, consider this arbitrary example:

Imagine user with an in-house node package @example/foo depending on node-gyp as well as another package @barsoft/bar. @barsoft/bar only supports node.engines<13, throwing actual runtime errors on newer node versions. The user is running this on Ubuntu 22.04 LTS, using nodejs and yarnpkg from package repositories. Perhaps as part of CI. They are affected by some issue which is resolved in the newest node-gyp release.

This would require some form of exceptional intervention of the user as part of the upgrade. I don't see how this can be considered anything other than breaking. In lack of any normative or de-facto interpretation for package manifests, the considerate thing to do here would be to actively consider the stronger interpretation until the general situation looks different (I am not personally aware of such developments but would love to learn about them).

Of course, every maintainer is ultimately free to make their own stance regarding which users and use-cases to support and how to handle the existing ambiguity. Choosing the stronger interpretation will require less assumptions to be non-breaking and would set an example for semver compliance (as I imagine many maintainers will follow the examples of leaders in the ecosystem).

[lukekarrys]

Now that #2770 has landed, we should figure out how to have multiple release lines. I plan on making a PR setting up tooling similar to how the npm CLI does it which supports multiple release lines.

Once that is complete, this should land on v9 as 9.4.1 and v10 will be the breaking engines changes.

[legobeat]

Please resolve the git conflict.

Rebased on main.

[legobeat]

@StefanStojanovic

[StefanStojanovic]

With this commit, my changes from 5df2b72 are no longer mandatory. They can be kept (probably should be), but the root cause for making them is removed.

[cclauss]

I am not qualified to review this. Is there a Node.js contributor that can?

[lukekarrys]

Apologies for being slow on reviewing this. This should definitely be merged, but I would like to wait until there is a release/v9 branch and change the base to that.

I will be creating that branch which will essentially be the current main branch with 192eec2 dropped from git history.

[imatlopez]

For historical reasons this library was picked due to its use in npm which allows deduping and also because its proxy support over node-fetch. Unsure where npm 10 lies with the issues reported.

[lukekarrys]

I just landed #2917 on the release/v9 branch, so I think this is safe to close for now.