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鈥檒l occasionally send you account related emails.

Already on GitHub? Sign in to your account

馃摝 Package: Switch dependency versions to ^ ranges #5114

Open
JoshuaKGoldberg opened this issue Mar 4, 2024 · 11 comments 路 May be fixed by #5150, #5151, #5152, #5153 or #5154
Open

馃摝 Package: Switch dependency versions to ^ ranges #5114

JoshuaKGoldberg opened this issue Mar 4, 2024 · 11 comments 路 May be fixed by #5150, #5151, #5152, #5153 or #5154
Assignees
Labels
status: accepting prs Mocha can use your help with this one! type: feature enhancement proposal

Comments

@JoshuaKGoldberg
Copy link
Member

Spinning out of #5090: @orgads noted that the package.json versions of dependencies are all pinned to specific versions like 4.1.1 rather than "caret" ^ ranges like ^4.1.1:

mocha/package.json

Lines 53 to 56 in 3345eff

"dependencies": {
"ansi-colors": "4.1.1",
"browser-stdout": "1.3.1",
"chokidar": "3.5.3",

Why is that?

I'm accustomed to ^ ranges to help consumers deduplicate packages. E.g. if a consumer's package requirements are chokidar@^3.5.2 and chokidar@^3.6.0, us specifying chokidar@^3.5.3 would mean they could all resolve to the same package version.

@voxpelli
Copy link
Member

I'm a big 馃憤 to this. It was different in the pre-package-lock.json era, that's when it was good practice to try and lock down dependencies this way, now its better handled by the package-lock.json in our and other's projects.

Maybe implement this on a dependency by dependency basis when we update them? That way we will test that no breakage will occur

@JoshuaKGoldberg JoshuaKGoldberg added status: accepting prs Mocha can use your help with this one! type: feature enhancement proposal and removed status: in discussion Let's talk about it! labels May 24, 2024
@JoshuaKGoldberg
Copy link
Member Author

JoshuaKGoldberg commented May 24, 2024

No comments for a while, and two 馃憤 votes. Accepting PRs!

Let's have a single PR for each dependency edit: group so we can test them separately - and revert separately if needed.

@the-sinner
Copy link

the-sinner commented May 29, 2024

Hi, as far i understand, you want to add a caret symbol after each dependency below (except chokidar) in separate PRs ?

mocha/package.json

Lines 53 to 74 in 472a8be

"dependencies": {
"ansi-colors": "4.1.1",
"browser-stdout": "1.3.1",
"chokidar": "^3.5.3",
"debug": "4.3.4",
"diff": "5.0.0",
"escape-string-regexp": "4.0.0",
"find-up": "5.0.0",
"glob": "8.1.0",
"he": "1.2.0",
"js-yaml": "4.1.0",
"log-symbols": "4.1.0",
"minimatch": "5.0.1",
"ms": "2.1.3",
"serialize-javascript": "6.0.0",
"strip-json-comments": "3.1.1",
"supports-color": "8.1.1",
"workerpool": "6.2.1",
"yargs": "16.2.0",
"yargs-parser": "20.2.4",
"yargs-unparser": "2.0.0"
},

@the-sinner
Copy link

So, while reading about the caret symbol in package.json, i found this stack overflow comment

Posting here to hopefully catch people that don't quite think this through, but both ^ and ~ assumes you can trust minor and point releases from your dependencies. If you are publishing a library and want other people to trust you, DO NOT BLINDLY ACCEPT DOWNSTREAM DEPENDENCIES. A bad dot release from your dependency can cause a chain reaction upstream, and will have people knocking at YOUR door when things go pear shaped. This is another huge reason to use npm shrinkwrap on your production code. - tehfoo

Comment

So, it should be fine, right ?

@JoshuaKGoldberg JoshuaKGoldberg self-assigned this May 30, 2024
@JoshuaKGoldberg
Copy link
Member Author

@the-sinner that comment is from almost a decade ago. If you look at common npm package usage, it's very rare to have exact pinned versions the way Mocha traditionally has it now. Package lockfiles such as package-lock.json have since made it much more reliable & secure to have version ranges.

Self-assigning as I can just take this on. 馃檪

@JoshuaKGoldberg JoshuaKGoldberg linked a pull request May 30, 2024 that will close this issue
3 tasks
@plroebuck
Copy link
Contributor

IMHO this sounds like a very bad idea. Mocha should not be ok with its 3rd party libraries getting updated whenever their developers release new code. There's not enough manpower to go through all the permutations if something goes wrong; is it reasonable to expect that everyone used npm ci? Mocha is one of the most-depended-upon modules on npm -- if this change causes things to go south, do you have enough maintainers to drop everything until it's fixed?

NPM Package-lock and semver caret versions

iconic: Package.json: Caret vs. pinned versions?

Reddit: [AskJS] Do you use exact or range versions for your dependencies?

@JoshuaKGoldberg
Copy link
Member Author

JoshuaKGoldberg commented May 31, 2024

@plroebuck do you have examples of other widely-used open source projects that still pin their dependencies to exact versions?

Examples of packages that do pin their dependencies include:

The downside of exact version pinning is that downstream consumers can't deduplicate packages. Non-trivial JS projects get an exponential growth of duplicate packages in their node_modules/ because everyone pins to a different exact patch version.

Now that projects have package-lock.jsons or other package lockfiles, we don't have to worry about npm upgrade mocha causing a slew of new minor or patch versions for consumers.

@voxpelli
Copy link
Member

@plroebuck You don't have to run npm ci to have your package-lock.json respected. A normal npm install also respects it.

We can pin our direct dependencies, but not their sub-dependencies, so the guarantee from us pinning is not absolute and can actually harm the efforts of our dependencies to fix stuff from their dependencies.

package-lock.json (and similar in yarn and pnpm) gives comprehensive locking support for the full dependency tree to a degree that we can't achieve through pinning dependencies, and I consider that to be the best practice nowadays whereas dependency pinning was the best practice prior to lock files being common place.

Also: By not having to release new versions for every small little dependency update we get rid of a lot of noise both in this repo and in those using dependency updaters like Dependabot and Renovate. It lessens the release fatigue and helps ensure that important changes don't get lost in a sea of noisy updates.

@JoshuaKGoldberg
Copy link
Member Author

Dependencies bumps are split into the following PRs:

Package PR
ansi-colors 馃崕 #5150
browser-stdout 馃崕 #5150
chokidar 馃 #5143
debug 馃 #5154
diff 馃 #5154
escape-string-regexp 馃崕 #5150
find-up 馃崒 #5151
glob 馃崒 #5151
he 馃崕 #5150
js-yaml 馃崱 #5153
log-symbols 馃崕 #5150
minimatch 馃崒 #5151
ms 馃 #5154
serialize-javascript 馃崱 #5153
strip-json-comments 馃崱 #5153
supports-color 馃崕 #5150
workerpool 馃 #5154
yargs 馃珢 #5152
yargs-parser 馃珢 #5152
yargs-unparser 馃珢 #5152

@plroebuck
Copy link
Contributor

@plroebuck You don't have to run npm ci to have your package-lock.json respected. A normal npm install also respects it.

The first link would seem to disagree with you, unless you're stating the blog's out-of-date.

We can pin our direct dependencies, but not their sub-dependencies, so the guarantee from us pinning is not absolute and can actually harm the efforts of our dependencies to fix stuff from their dependencies.

Don't think this was ever guaranteed. This a tastes-great/less-filling argument though as far as dependencies fixing their stuff.

package-lock.json (and similar in yarn and pnpm) gives comprehensive locking support for the full dependency tree to a degree that we can't achieve through pinning dependencies, and I consider that to be the best practice nowadays whereas dependency pinning was the best practice prior to lock files being common place.

I would probably agree with the best practice statement... for everyone else. Let all the other packages deduplicate.

Also: By not having to release new versions for every small little dependency update we get rid of a lot of noise both in this repo and in those using dependency updaters like Dependabot and Renovate. It lessens the release fatigue and helps ensure that important changes don't get lost in a sea of noisy updates.

Rather disagree that the decrease in noise would be worth the potential increase in manhours tracking third party updates that broke things down needlessly. And less fatigue trying to troubleshoot other people's setups in bug reports was far more worthwhile to me.

Anyhow, if it's decided to move forward with this setup, hopefully it doesn't come back to bite you.

@voxpelli
Copy link
Member

@plroebuck You don't have to run npm ci to have your package-lock.json respected. A normal npm install also respects it.

The first link would seem to disagree with you, unless you're stating the blog's out-of-date.

See the npm documentation itself: https://docs.npmjs.com/cli/v10/commands/npm-install#package-lock

The only notable difference in this regard is that if the direct dependencies listed in package.json differs from the one used when the package-lock.json was last updated, then the package.json wins when using npm install and the lockfile gets updated accordingly. When using npm ci an error is instead thrown. See https://docs.npmjs.com/cli/v10/commands/npm-ci#description for an exhaustive list of all the differences.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment