feat: improve heuristics when dist-tags.latest is in range

[isaacs]

Previously, there were two problems.

First problem, even though we heuristically choose the version that best
suits the stated 'engines' restriction, we were skipping that check when
the 'defaultTag' (ie, 'latest', typically) version was a semver match.

Second problem, the heuristic was improperly being set for 'staged' and
'restricted' packages, resulting in failure to sort those versions
properly.

Only choose the defaultTag version if it both a semver match, and
passes the engines/staged/restricted heuristics, and apply those
heuristics properly in the sort that comes later, if the defaultTag
version is not used.

Related-to: npm/rfcs#405

References

[isaacs]

rebased onto main (can't force push, no longer have access to this repo)

From 0637cf6083d8038494ddd4d2cbf57bef4f5dac4c Mon Sep 17 00:00:00 2001
From: isaacs <[email protected]>
Date: Wed, 28 Jul 2021 12:34:15 -0700
Subject: [PATCH] fix: improve heuristics when dist-tags.latest is in range

Previously, there were two problems.

First problem, even though we heuristically choose the version that best
suits the stated 'engines' restriction, we were skipping that check when
the 'defaultTag' (ie, 'latest', typically) version was a semver match.

Second problem, the heuristic was improperly being set for 'staged' and
'restricted' packages, resulting in failure to sort those versions
properly.

Only choose the defaultTag version if it both a semver match, _and_
passes the engines/staged/restricted heuristics, and apply those
heuristics properly in the sort that comes later, if the defaultTag
version is not used.

Related-to: https://github.com/npm/rfcs/pull/405
---
 lib/index.js  | 16 +++++++++++-----
 test/index.js | 13 ++++++++++++-
 2 files changed, 23 insertions(+), 6 deletions(-)

diff --git a/lib/index.js b/lib/index.js
index 42e41b1..8280797 100644
--- a/lib/index.js
+++ b/lib/index.js
@@ -123,9 +123,15 @@ const pickManifest = (packument, wanted, opts) => {
   const defaultVer = distTags[defaultTag]
   if (defaultVer &&
       (range === '*' || semver.satisfies(defaultVer, range, { loose: true })) &&
+      !restricted[defaultVer] &&
       !shouldAvoid(defaultVer, avoid)) {
     const mani = versions[defaultVer]
-    if (mani && isBefore(verTimes, defaultVer, time)) {
+    const ok = mani &&
+      isBefore(verTimes, defaultVer, time) &&
+      engineOk(mani, npmVersion, nodeVersion) &&
+      !mani.deprecated &&
+      !staged[defaultVer]
+    if (ok) {
       return mani
     }
   }
@@ -155,10 +161,10 @@ const pickManifest = (packument, wanted, opts) => {
       const [verb, manib] = b
       const notavoida = !shouldAvoid(vera, avoid)
       const notavoidb = !shouldAvoid(verb, avoid)
-      const notrestra = !restricted[a]
-      const notrestrb = !restricted[b]
-      const notstagea = !staged[a]
-      const notstageb = !staged[b]
+      const notrestra = !restricted[vera]
+      const notrestrb = !restricted[verb]
+      const notstagea = !staged[vera]
+      const notstageb = !staged[verb]
       const notdepra = !mania.deprecated
       const notdeprb = !manib.deprecated
       const enginea = engineOk(mania, npmVersion, nodeVersion)
diff --git a/test/index.js b/test/index.js
index 366bcca..682c8d5 100644
--- a/test/index.js
+++ b/test/index.js
@@ -130,6 +130,9 @@ test('ETARGET if range does not match anything', t => {
 
 test('E403 if version is forbidden', t => {
   const metadata = {
+    'dist-tags': {
+      latest: '2.1.0' // do not default the latest if restricted
+    },
     policyRestrictions: {
       versions: {
         '2.1.0': { version: '2.1.0' },
@@ -141,6 +144,9 @@ test('E403 if version is forbidden', t => {
       '2.0.5': { version: '2.0.5' },
     },
   }
+  t.equal(pickManifest(metadata, '2').version, '2.0.5')
+  t.equal(pickManifest(metadata, '').version, '2.0.5')
+  t.equal(pickManifest(metadata, '1 || 2').version, '2.0.5')
   t.throws(() => {
     pickManifest(metadata, '2.1.0')
   }, { code: 'E403' }, 'got correct error on match failure')
@@ -423,6 +429,9 @@ test('accepts opts.before option to do date-based cutoffs', t => {
 
 test('prefers versions that satisfy the engines requirement', t => {
   const pack = {
+    'dist-tags': {
+      latest: '1.5.0' // do not default latest if engine mismatch
+    },
     versions: {
       '1.0.0': { version: '1.0.0', engines: { node: '>=4' } },
       '1.1.0': { version: '1.1.0', engines: { node: '>=6' } },
@@ -430,7 +439,9 @@ test('prefers versions that satisfy the engines requirement', t => {
       '1.3.0': { version: '1.3.0', engines: { node: '>=10' } },
       '1.4.0': { version: '1.4.0', engines: { node: '>=12' } },
       '1.5.0': { version: '1.5.0', engines: { node: '>=14' } },
-    },
+      // not tagged as latest, won't be chosen by default
+      '1.5.1': { version: '1.5.0', engines: { node: '>=14' } },
+    }
   }
 
   t.equal(pickManifest(pack, '1.x', { nodeVersion: '14.0.0' }).version, '1.5.0')
-- 
2.39.3 (Apple Git-146)

[wraithgar]

Rebased, fixed linting (which ironically would have prevented the merge conflict. I begrudgingly have accepted that trailing commas on every line make sense).

[ljharb]

i'd consider this a semver-minor, fwiw.

also the farther back in npm majors this can be backported (i understand nobody wants to do that), the sooner the ecosystem may be able to actually treat dropping an engine as a nonbreaking change.

[jakebailey]

I think this turned into a breaking change for DefinitelyTyped's tooling; before, we'd call pacote.manifest on a package and it'd resolve to the latest regardless of the deprecated status and then we'd check if that was deprecated, but now !mani.deprecated seems to filter those out, which led to our publisher repeatedly attempting to deprecate package it thought it needed to update (which were actually already updated).

Why did this package filter out deprecated packages? Past DT's use case, wouldn't you want the latest version of things even if deprecated, otherwise you'll just pick random old versions and people won't get the deprecation warnings?

[isaacs]

@jakebailey If something's deprecated, and there's a version that isn't deprecated that will satisfy the version requirement, then that's the one you ought to be using.

npm used to work this way, and then regressed and no one noticed for a while. Then I fixed it, but the fix took a long time to land, apparently resulting in some things like DT coming to rely on the not-working-as-intended-or-documented behavior.

Moving slow breaks things sometimes. This should've been fixed years ago, before you had a chance to rely on it. It's caused quite a few problems by going unfixed this long, and cannot be reverted without bringing those problems back.

Assume that "deprecated" is a soft form of unpublishing. Do not rely on resolving to deprecated versions. That was always the intent. (Or at least, if there is a use case for this, you'll have to either use the old version that had the bug you rely on, roll your own npm-pick-manifest implementation, or start a conversation about adding "ignoreDeprecation" as a new option.)

[jakebailey]

The odd thing is just that for DefinitelyTyped packages, users will never get the deprecated versions that say "you don't need me anymore" or install a related library to provide global types or similar.

I guess I just find it hard to believe that this specific thing was a bug for so long, given how long this behavior has been used by DefinitelyTyped (it has to be nearly a decade now...)

The fix on the DT publisher side is at least "simple" (just ask for latest no matter what, don't let pacote choose), but, I was just thinking ahead to other uses...