cheeriojs / cheerio

The fast, flexible, and elegant library for parsing and manipulating HTML and XML.
https://cheerio.js.org
MIT License
28.4k stars 1.64k forks source link

Could you support Node V16 for v1.0.0 #3993

Open mapuboy opened 1 month ago

mapuboy commented 1 month ago

Thank the great lib Cheerio first. Our VM is not support Nodejs V18, but the Cheerio is "engines": { "node": ">=18.17" }, so our installation encounter this npm install 10:14:02 npm ERR! code EBADENGINE 10:14:02 npm ERR! engine Unsupported engine 10:14:02 npm ERR! engine Not compatible with your version of node/npm: cheerio@1.0.0 10:14:02 npm ERR! notsup Not compatible with your version of node/npm: cheerio@1.0.0 10:14:02 npm ERR! notsup Required: {"node":">=18.17"} 10:14:02 npm ERR! notsup Actual: {"npm":"8.5.5","node":"v16.15.0"} 10:14:02
10:14:02 npm ERR! A complete log of this run can be found in: 10:14:02 npm ERR! /root/.npm/_logs/2024-08-12T02_11_11_118Z-debug-0.log

So, could you continue to support Node V16? Thanks again.

pnappa commented 1 month ago

Node v16 is unsupported, and does not even receive security updates anymore, and as such is unsafe. It's highly recommended you upgrade (I'm sure there's a way for your VM to use a more modern version of NodeJS) to a supported version. https://endoflife.date/nodejs

Looking at the dependencies, it's not up to cheerio to downgrade their required engines versions, they've also got three dependencies which require >=v18:

┌──────────────────────────────┬──────────────┐
│ Conflicting dependencies (3) │ engines.node │
├──────────────────────────────┼──────────────┤
│ undici                       │ >=18.17      │
├──────────────────────────────┼──────────────┤
│ whatwg-encoding              │ >=18         │
├──────────────────────────────┼──────────────┤
│ whatwg-mimetype              │ >=18         │
└──────────────────────────────┴──────────────┘
mapuboy commented 1 month ago

Thanks. I will find a way for our VM to use a more modern version of NodeJs.

txw2018 commented 1 month ago

package.json "pnpm": { "overrides": { "cheerio": "1.0.0-rc.12" } }

That will solve it

davidkovacstw commented 1 month ago

Looking at the dependencies, it's not up to cheerio to downgrade their required engines versions, they've also got three dependencies which require >=v18:

In semantic versioning, if you have a breaking change, you should bump your major version too.

Many users of this library expected that this principle will be respected, so it's fine to use ^1.0.0 as a version number. Now releasing breaking changes without changing the major version broke all these users. Please note that some of these users are transitive ones, so they might not even have full control of the used node version.

So I would respectfully ask you to:

Thanks for your work.

Alevale commented 1 month ago

I really don't wanna be that guy, but when you package has ~8338955 weekly downloads you can't expect your community to address issues in an hour, or be all up to date with Node, nor have everything perfect as @pnappa is suggesting here.

To be fair, I think that some of the changes that have been part of the release candidate packages feel to me like they could be under testing packages.

Please, in this case as @davidkovacstw suggested, revert the changes from 1.0.0 as a new version 1.0.1 and keep the new changes (all of the breaking ones) under 2.0.0

Like dropping node, or removing the lib folder.

pnappa commented 1 month ago

@davidkovacstw I agree with the comment about semantic versioning! When you break consumers of the package, a major version should be published so as not to break things.

However, isn't the case here that cheerio updated major version to version 1.0.0, and thus releasing a new major version, meaning they can introduce breaking changes? If you review the release notes, this change is listed: https://github.com/cheeriojs/cheerio/releases/tag/v1.0.0

Of which, bumping the minimum version of NodeJS as a result of bumping the undici dependency due to a low severity security vulnerability. https://github.com/nodejs/undici/security/advisories/GHSA-3g92-w8c5-73pq

Maintaining open source software is a thankless task, and we should be making their lives easier by not requesting they also make changes to support old & unsupported versions of software.

Thanks again to the cheerio devs for your software!

Alevale commented 1 month ago

@davidkovacstw I agree with the comment about semantic versioning! When you break consumers of the package, a major version should be published so as not to break things.

However, isn't the case here that cheerio updated major version to version 1.0.0, and thus releasing a new major version, meaning they can introduce breaking changes? If you review the release notes, this change is listed: https://github.com/cheeriojs/cheerio/releases/tag/v1.0.0

Of which, bumping the minimum version of NodeJS as a result of bumping the undici dependency due to a low severity security vulnerability. GHSA-3g92-w8c5-73pq

Yes and no.

I agree, that 1.0.0 wasn't final and there were RC packages till it became "stable enough" what I don't agree with is with the fact that it is acceptable to keep 1.0.0 as a similar or comparable version to what v1.0.0-rc.12 was previously.

1.0.0 deviated considerably from what was previously v1.0.0-rc.12 and it did so with the satement underneath (moving from Node 16 to Node 18)

The minimum NodeJS version is now 18.17 or higher https://github.com/cheeriojs/cheerio/pull/3959

It became a mandatory major by requiring a different major version of a previously used library/part of your system (Node in this case). By doing so you are breaking previous compatibility with any of your library prior clients, which in return should translate on you changing your package to become a major.

If you want to let them know that they need to updated you can deprecate or put a note on your current version, but you should not break semver and release a major on a minor.

There's no room for discussion, this is literally the definition of management of patches, minors and majors under semver.

Btw, I'm also grateful for all the amazing open source software and contributors. That is totally independent of this issue.

pnappa commented 1 month ago

Thanks for the insight @Alevale.

Treating semantic versioning as the letter of the law (of course, it's an entirely optional spec & the authors don't need to follow it), pre-release versions in semantic versioning have no bearing on compatibility across versions. As per https://semver.org/#spec-item-9, in bold:

  1. A pre-release version MAY be denoted by appending a hyphen and a series of dot separated identifiers immediately following the patch version. Identifiers MUST comprise only ASCII alphanumerics and hyphens [0-9A-Za-z-]. Identifiers MUST NOT be empty. Numeric identifiers MUST NOT include leading zeroes. Pre-release versions have a lower precedence than the associated normal version. A pre-release version indicates that the version is unstable and might not satisfy the intended compatibility requirements as denoted by its associated normal version. Examples: 1.0.0-alpha, 1.0.0-alpha.1, 1.0.0-0.3.7, 1.0.0-x.7.z.92, 1.0.0-x-y-z.--.

For the purpose of semantic versioning, the prior full release of 1.0.0 was 0.22.0 (released in 2016!). Unless I'm parsing the above paragraph incorrectly (or have missed something in the rest of the versioning spec, which is entirely possible!), 1.0.0 is a major version bump, and the release candidates have no bearing on compatibility guarantees between release candidates, or between the release candidate and the final version.

There was a mistake in the version tagging that did mark pre-release versions as latest, when they should have been next - presumably what's caused the implicit version update kerfuffle for consumers of this package.

Alevale commented 1 month ago

Thanks for sharing this @pnappa and you are right on that release candidates are indeed not stable by nature and anything can be dropped at any point.

My issue is that when you release once in a blue moon (or in the case of this package once every 2 years) I would argue that your RC packages should be treated as actual packages.

Keep in mind that the first release candidate was in 2017 and included a ton of breaking changes (which makes total sense cause it was different from 0.22 and it was the place to put them)

To be fair, there's also some instances where the RC could have been tagged as a major, but let's move on with the discussion.

My way of thinking is that if you can make things easier for the 8 million weekly downloads that should be the way to go, and if that means being in v11 that's fine.

I think it's not sensible to have to write down "cheerio": "=1.0.0-rc.12" in the package json of every single dependency of this, and resolutions or overrides for all the community...

If that's acceptable because someone was right I think we are failing as a community

Alevale commented 1 month ago

@matthewmueller @fb55 @jugglinmike In light of the discussion, and based on what is happening to everyone who trusted using your package for their builds, could you please do as suggested by this discussion and move the changes to 2.0.0 as suggested by @davidkovacstw ?

Semver is cool and shit, till you have a RC active for 7 years, 8 Million downloads, and people assumed you wouldn't change it much anymore.

CodeMedic42 commented 4 weeks ago

I created a ticket(#4014) for something similar and @Alevale linked to this one there. I wanted to post what I have done to resolve this.

We use pnpm and I have added the following to our package.json

{
    "pnpm": {
        "overrides": {
            "cheerio": "1.0.0-rc.12"
        },
}

This obviously has a negative side effect for forcing ANYTHING using cheerio to use this version, so use carefully. Also it will always use this version until you remove it. Be sure to reevaluate this issue periodically to see if it has been resolved and you can remove this constraint. All very obvious statements but I like to be through.

cossssmin commented 4 weeks ago

We use cheerio in Juice (>1M downloads/week), which is now broken because of this unexpected breaking change release from an RC - we need to pin it to 1.0.0-rc.12 and release a patch ourselves now to fix this.

Please, in this case as @davidkovacstw suggested, revert the changes from 1.0.0 as a new version 1.0.1and keep the new changes (all of the breaking ones) under 2.0.0

Yes, please.

mapuboy commented 3 weeks ago

package.json "pnpm": { "overrides": { "cheerio": "1.0.0-rc.12" } }

That will solve it

Yes. I have done that to resolve it for temp fixing. But I upgraded our VMs too.

kabbagepatch commented 2 weeks ago

Thanks for the insight @Alevale.

Treating semantic versioning as the letter of the law (of course, it's an entirely optional spec & the authors don't need to follow it), pre-release versions in semantic versioning have no bearing on compatibility across versions. As per https://semver.org/#spec-item-9, in bold:

  1. A pre-release version MAY be denoted by appending a hyphen and a series of dot separated identifiers immediately following the patch version. Identifiers MUST comprise only ASCII alphanumerics and hyphens [0-9A-Za-z-]. Identifiers MUST NOT be empty. Numeric identifiers MUST NOT include leading zeroes. Pre-release versions have a lower precedence than the associated normal version. A pre-release version indicates that the version is unstable and might not satisfy the intended compatibility requirements as denoted by its associated normal version. Examples: 1.0.0-alpha, 1.0.0-alpha.1, 1.0.0-0.3.7, 1.0.0-x.7.z.92, 1.0.0-x-y-z.--.

For the purpose of semantic versioning, the prior full release of 1.0.0 was 0.22.0 (released in 2016!). Unless I'm parsing the above paragraph incorrectly (or have missed something in the rest of the versioning spec, which is entirely possible!), 1.0.0 is a major version bump, and the release candidates have no bearing on compatibility guarantees between release candidates, or between the release candidate and the final version.

There was a mistake in the version tagging that did mark pre-release versions as latest, when they should have been next - presumably what's caused the implicit version update kerfuffle for consumers of this package.

I think we all understand that technically the devs did nothing wrong. That said, we're all developers here and I like to think that our goal, in general, is to make people's lives easier. Going from a 1.0.0-rc that has millions of downloads a week to a 1.0.0 release that breaks a good chunk of those consumers' projects isn't making people's lives easier. On the other hand, changing the tag to 2.0.0 before releasing would've taken little to no effort. Just wanted to add my two cents