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(deps): Allow Node 20.9.0 and higher #3918

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

RobinTail
Copy link

@RobinTail RobinTail commented Dec 3, 2024

This relates to...

#3880

Rationale

While dropping Node 18 support for v7, almost the entire Node 20.x support was also dropped unreasonably.
The engines should declare the minimum requirements for the environment to run the software.
It should not be the latest patch available at the moment, it should not be a favourite version of a particular individual, but a reasonable limitation based on the internal architecture and further development strategy.
Node 20.9.0 is the LTS version of 20.x and undici works perfectly fine with it, so I'm suggesting to correct the minimum supported Node version from 20.18.1 to 20.9.0.

Changes

  • Allow Node.js starting from 20.9.0
  • Test against that version as well

Features

None

Bug Fixes

None

Breaking Changes and Deprecations

None

Status

@RobinTail RobinTail mentioned this pull request Dec 3, 2024
@mcollina
Copy link
Member

mcollina commented Dec 3, 2024

I'm not comfortable with this change, as it would consider us recommending the use a version of Node.js that is full of security issues:

➜  ~ nvm use 20.9.0
Now using node v20.9.0 (npm v10.1.0)
➜  ~ npx is-my-node-vulnerable                                                                                                                                        Need to install the following packages:
[email protected]
Ok to proceed? (y) y

                                                                                                                                                                      ██████   █████  ███    ██  ██████  ███████ ███████
██   ██ ██   ██ ████   ██ ██       ██      ██   ██
██   ██ ███████ ██ ██  ██ ██   ███ █████   ███████
██   ██ ██   ██ ██  ██ ██ ██    ██ ██      ██   ██
██████  ██   ██ ██   ████  ██████  ███████ ██   ██

                                                                                                                                                                      The current Node.js version (v20.9.0) is vulnerable to the following CVEs:

(...)

The last security release is v20.15.1. I'm ok in using that as a criteria, even if we should recommend to always use the latest version of a given LTS line.

@RobinTail
Copy link
Author

RobinTail commented Dec 3, 2024

as it would consider us recommending

the real meaning on engines keyword

@mcollina , Please don't invent nonsense when the real meaning of things is clearly documented.

@mcollina
Copy link
Member

mcollina commented Dec 3, 2024

Stating that the module works on anything after XX means that we guarantee that it will keep working on XX, which we don't want to ensure (and that was my intention on the PR). v20.18.1 marks the point in which v20 goes in maintenance mode, and therefore the likelihood of changes are minimal.

To reiterate: the requirement is not unreasonable as stated in the OP. It was actually well reasoned, but just using criteria that are not of your liking.

Supporting old versions of Node.js is a lot of hard work, and reducing the maintenance burden is actually a priority. You can keep using undici v6 for quite a few more years, so there is no real urgency of updating.

@mcollina
Copy link
Member

mcollina commented Dec 3, 2024

I've tried to run the undici test suite on v20.9.0 and one of the wpt fails:

================================================================================================
Started /Users/matteo/Repositories/undici/test/fixtures/wpt/mimesniff/mime-types/parsing.any.js
[1/1] FAILED - /Users/matteo/Repositories/undici/test/fixtures/wpt/mimesniff/mime-types/parsing.any.js
File timed out after 60000ms

@KhafraDev
Copy link
Member

I've tried to run the undici test suite on v20.9.0 and one of the wpt fails:

that test should be version-independent, it's purely string parsing. I can take a look at it, because realistically it should never fail.

@metcoder95
Copy link
Member

Unless using --engine-strict, you should still be able to use the v7 of undici in a "non-supported" version, npm does not fail unless explicitly said.

I get the point you are trying to make. On top of what was stated by @mcollina, this was set in a major version for the same reason, as it is a breaking change, the contract is always respected.

@RobinTail
Copy link
Author

RobinTail commented Dec 3, 2024

Unless using --engine-strict, you should still be able to use the v7 of undici in a "non-supported" version, npm does not fail unless explicitly said.

It's opposite for yarn. It does not allow engines violation without --ignore-engines, @metcoder95

I get the point you are trying to make

The point I'm making is that npm and yarn are specific cases of a package managers, their behaviour should not justify anything. On contrary, engines is a well documented public declaration of software compatibility that should not be misused.

@metcoder95
Copy link
Member

It's opposite for yarn. It does not allow engines violation without --ignore-engines

Yeah, that's one of the biggest downsides when standardization has not been set.

On contrary, engines is a well documented public declaration of software compatibility that should not be misused.

See what you mean, but I wouldn't say is misused; the change was introduced as part of a semver major, meaning that the communication of a possible incompatibility problem was well stated through the use of an standardised communication (semver).

I'd say that a clear misusage would be if we added this as part of v6.x minor update, as we will be violating a communication standard previously agreed we follow.

@RobinTail
Copy link
Author

RobinTail commented Dec 4, 2024

meaning that the communication of a possible incompatibility problem was well stated

But there is no incompatibility with 20.9.0, @metcoder95 .
So the communication itself was the act of misleading.

@artur-ma
Copy link
Contributor

artur-ma commented Dec 10, 2024

meaning that the communication of a possible incompatibility problem was well stated

But there is no incompatibility with 20.9.0, @metcoder95 . So the communication itself was the act of misleading.

I wouldn't call this

You can specify the version of node that your stuff works on

as "clearly" documented, its more a "quick description" rather than a standard like RFC

In the "engine" section the authors put the versions they are ready to support, the fact that current version runs on version 20.9 does not mean the next minor will support it. By setting the "engine" to a specific version, you are indicating that usage on a "non-supported" version is at the user's own risk.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants