fix: don't suggest npm update outside of valid engine range (#8050)

When doing manifest call for fetching npm manifest it was using latest
every-time, however fetching`npm@*` will still give `latest` if it's
satisfies the engine range otherwise it will give highest matching
version for the current engine range.
Pacote.manifest calls internally uses pick-manifest logic to pick the
appropriate version of the manifest for the engine range.

Author: milaninfy

lib/cli/update-notifier.js

@@ -40,7 +40,7 @@ const updateCheck = async (npm, spec, version, current) => {
   // and should get the updates from that release train.
   // Note that this isn't another http request over the network, because
   // the packument will be cached by pacote from previous request.
-  if (gt(version, latest) && spec === 'latest') {
+  if (gt(version, latest) && spec === '*') {
     return updateNotifier(npm, `^${version}`)
   }
 
@@ -71,7 +71,7 @@ const updateCheck = async (npm, spec, version, current) => {
   return message
 }
 
-const updateNotifier = async (npm, spec = 'latest') => {
+const updateNotifier = async (npm, spec = '*') => {
   // if we're on a prerelease train, then updates are coming fast
   // check for a new one daily.  otherwise, weekly.
   const { version } = npm
@@ -83,7 +83,7 @@ const updateNotifier = async (npm, spec = 'latest') => {
   }
 
   // while on a beta train, get updates daily
-  const duration = spec !== 'latest' ? DAILY : WEEKLY
+  const duration = current.prerelease.length ? DAILY : WEEKLY
 
   const t = new Date(Date.now() - duration)
   // if we don't have a file, then definitely check it.

tap-snapshots/test/lib/cli/update-notifier.js.test.cjs

@@ -5,6 +5,14 @@
  * Make sure to inspect the output below.  Do not ignore changes!
  */
 'use strict'
+exports[`test/lib/cli/update-notifier.js TAP notification situation with engine compatibility > must match snapshot 1`] = `
+
+New minor version of npm available! 123.420.70 -> 123.421.60
+Changelog: https://github.com/npm/cli/releases/tag/v123.421.60
+To update run: npm install -g [email protected]
+
+`
+
 exports[`test/lib/cli/update-notifier.js TAP notification situations 122.420.69 - color=always > must match snapshot 1`] = `
 
 New major version of npm available! 122.420.69 -> 123.420.69

test/lib/cli/update-notifier.js

@@ -2,21 +2,59 @@ const t = require('tap')
 const { basename } = require('node:path')
 const tmock = require('../../fixtures/tmock')
 const mockNpm = require('../../fixtures/mock-npm')
+const MockRegistry = require('@npmcli/mock-registry')
+const mockGlobals = require('@npmcli/mock-globals')
 
 const CURRENT_VERSION = '123.420.69'
 const CURRENT_MAJOR = '122.420.69'
 const CURRENT_MINOR = '123.419.69'
 const CURRENT_PATCH = '123.420.68'
 const NEXT_VERSION = '123.421.70'
+const NEXT_VERSION_ENGINE_COMPATIBLE = '123.421.60'
+const NEXT_VERSION_ENGINE_COMPATIBLE_MINOR = `123.420.70`
+const NEXT_VERSION_ENGINE_COMPATIBLE_PATCH = `123.421.58`
 const NEXT_MINOR = '123.420.70'
 const NEXT_PATCH = '123.421.69'
 const CURRENT_BETA = '124.0.0-beta.99999'
 const HAVE_BETA = '124.0.0-beta.0'
 
+const packumentResponse = {
+  _id: 'npm',
+  name: 'npm',
+  'dist-tags': {
+    latest: CURRENT_VERSION,
+  },
+  access: 'public',
+  versions: {
+    [CURRENT_VERSION]: { version: CURRENT_VERSION, engines: { node: '>1' } },
+    [CURRENT_MAJOR]: { version: CURRENT_MAJOR, engines: { node: '>1' } },
+    [CURRENT_MINOR]: { version: CURRENT_MINOR, engines: { node: '>1' } },
+    [CURRENT_PATCH]: { version: CURRENT_PATCH, engines: { node: '>1' } },
+    [NEXT_VERSION]: { version: NEXT_VERSION, engines: { node: '>1' } },
+    [NEXT_MINOR]: { version: NEXT_MINOR, engines: { node: '>1' } },
+    [NEXT_PATCH]: { version: NEXT_PATCH, engines: { node: '>1' } },
+    [CURRENT_BETA]: { version: CURRENT_BETA, engines: { node: '>1' } },
+    [HAVE_BETA]: { version: HAVE_BETA, engines: { node: '>1' } },
+    [NEXT_VERSION_ENGINE_COMPATIBLE]: {
+      version: NEXT_VERSION_ENGINE_COMPATIBLE,
+      engiges: { node: '<=1' },
+    },
+    [NEXT_VERSION_ENGINE_COMPATIBLE_MINOR]: {
+      version: NEXT_VERSION_ENGINE_COMPATIBLE_MINOR,
+      engines: { node: '<=1' },
+    },
+    [NEXT_VERSION_ENGINE_COMPATIBLE_PATCH]: {
+      version: NEXT_VERSION_ENGINE_COMPATIBLE_PATCH,
+      engines: { node: '<=1' },
+    },
+  },
+}
+
 const runUpdateNotifier = async (t, {
   STAT_ERROR,
   WRITE_ERROR,
   PACOTE_ERROR,
+  PACOTE_MOCK_REQ_COUNT = 1,
   STAT_MTIME = 0,
   mocks: _mocks = {},
   command = 'help',
@@ -51,24 +89,7 @@ const runUpdateNotifier = async (t, {
     },
   }
 
-  const MANIFEST_REQUEST = []
-  const mockPacote = {
-    manifest: async (spec) => {
-      if (!spec.match(/^npm@/)) {
-        t.fail('no pacote manifest allowed for non npm packages')
-      }
-      MANIFEST_REQUEST.push(spec)
-      if (PACOTE_ERROR) {
-        throw PACOTE_ERROR
-      }
-      const manifestV = spec === 'npm@latest' ? CURRENT_VERSION
-        : /-/.test(spec) ? CURRENT_BETA : NEXT_VERSION
-      return { version: manifestV }
-    },
-  }
-
   const mocks = {
-    pacote: mockPacote,
     'node:fs/promises': mockFs,
     '{ROOT}/package.json': { version },
     'ci-info': { isCI: false, name: null },
@@ -83,124 +104,125 @@ const runUpdateNotifier = async (t, {
     prefixDir,
     argv,
   })
+  const registry = new MockRegistry({
+    tap: t,
+    registry: mock.npm.config.get('registry'),
+  })
+
+  if (PACOTE_MOCK_REQ_COUNT > 0) {
+    registry.nock.get('/npm').times(PACOTE_MOCK_REQ_COUNT).reply(200, packumentResponse)
+  }
+
   const updateNotifier = tmock(t, '{LIB}/cli/update-notifier.js', mocks)
 
   const result = await updateNotifier(mock.npm)
 
   return {
     wroteFile,
     result,
-    MANIFEST_REQUEST,
   }
 }
 
 t.test('duration has elapsed, no updates', async t => {
-  const { wroteFile, result, MANIFEST_REQUEST } = await runUpdateNotifier(t)
+  const { wroteFile, result } = await runUpdateNotifier(t)
   t.equal(wroteFile, true)
   t.not(result)
-  t.equal(MANIFEST_REQUEST.length, 1)
 })
 
 t.test('situations in which we do not notify', t => {
   t.test('nothing to do if notifier disabled', async t => {
-    const { wroteFile, result, MANIFEST_REQUEST } = await runUpdateNotifier(t, {
+    const { wroteFile, result } = await runUpdateNotifier(t, {
+      PACOTE_MOCK_REQ_COUNT: 0,
       'update-notifier': false,
     })
     t.equal(wroteFile, false)
     t.equal(result, null)
-    t.strictSame(MANIFEST_REQUEST, [], 'no requests for manifests')
   })
 
   t.test('do not suggest update if already updating', async t => {
-    const { wroteFile, result, MANIFEST_REQUEST } = await runUpdateNotifier(t, {
+    const { wroteFile, result } = await runUpdateNotifier(t, {
+      PACOTE_MOCK_REQ_COUNT: 0,
       command: 'install',
       prefixDir: { 'package.json': `{"name":"${t.testName}"}` },
       argv: ['npm'],
       global: true,
     })
     t.equal(wroteFile, false)
     t.equal(result, null)
-    t.strictSame(MANIFEST_REQUEST, [], 'no requests for manifests')
   })
 
   t.test('do not suggest update if already updating with spec', async t => {
-    const { wroteFile, result, MANIFEST_REQUEST } = await runUpdateNotifier(t, {
+    const { wroteFile, result } = await runUpdateNotifier(t, {
+      PACOTE_MOCK_REQ_COUNT: 0,
       command: 'install',
       prefixDir: { 'package.json': `{"name":"${t.testName}"}` },
       argv: ['npm@latest'],
       global: true,
     })
     t.equal(wroteFile, false)
     t.equal(result, null)
-    t.strictSame(MANIFEST_REQUEST, [], 'no requests for manifests')
   })
 
   t.test('do not update if same as latest', async t => {
-    const { wroteFile, result, MANIFEST_REQUEST } = await runUpdateNotifier(t)
+    const { wroteFile, result } = await runUpdateNotifier(t)
     t.equal(wroteFile, true)
     t.equal(result, null)
-    t.strictSame(MANIFEST_REQUEST, ['npm@latest'], 'requested latest version')
   })
   t.test('check if stat errors (here for coverage)', async t => {
     const STAT_ERROR = new Error('blorg')
-    const { wroteFile, result, MANIFEST_REQUEST } = await runUpdateNotifier(t, { STAT_ERROR })
+    const { wroteFile, result } = await runUpdateNotifier(t, { STAT_ERROR })
     t.equal(wroteFile, true)
     t.equal(result, null)
-    t.strictSame(MANIFEST_REQUEST, ['npm@latest'], 'requested latest version')
   })
   t.test('ok if write errors (here for coverage)', async t => {
     const WRITE_ERROR = new Error('grolb')
-    const { wroteFile, result, MANIFEST_REQUEST } = await runUpdateNotifier(t, { WRITE_ERROR })
+    const { wroteFile, result } = await runUpdateNotifier(t, { WRITE_ERROR })
     t.equal(wroteFile, true)
     t.equal(result, null)
-    t.strictSame(MANIFEST_REQUEST, ['npm@latest'], 'requested latest version')
   })
   t.test('ignore pacote failures (here for coverage)', async t => {
     const PACOTE_ERROR = new Error('pah-KO-tchay')
-    const { wroteFile, result, MANIFEST_REQUEST } = await runUpdateNotifier(t, { PACOTE_ERROR })
+    const { wroteFile, result } = await runUpdateNotifier(t, {
+      PACOTE_ERROR, PACOTE_MOCK_REQ_COUNT: 0,
+    })
     t.equal(result, null)
     t.equal(wroteFile, true)
-    t.strictSame(MANIFEST_REQUEST, ['npm@latest'], 'requested latest version')
   })
   t.test('do not update if newer than latest, but same as next', async t => {
     const {
       wroteFile,
       result,
-      MANIFEST_REQUEST,
     } = await runUpdateNotifier(t, { version: NEXT_VERSION })
     t.equal(result, null)
     t.equal(wroteFile, true)
-    const reqs = ['npm@latest', `npm@^${NEXT_VERSION}`]
-    t.strictSame(MANIFEST_REQUEST, reqs, 'requested latest and next versions')
   })
   t.test('do not update if on the latest beta', async t => {
     const {
       wroteFile,
       result,
-      MANIFEST_REQUEST,
     } = await runUpdateNotifier(t, { version: CURRENT_BETA })
     t.equal(result, null)
     t.equal(wroteFile, true)
-    const reqs = [`npm@^${CURRENT_BETA}`]
-    t.strictSame(MANIFEST_REQUEST, reqs, 'requested latest and next versions')
   })
 
   t.test('do not update in CI', async t => {
-    const { wroteFile, result, MANIFEST_REQUEST } = await runUpdateNotifier(t, { mocks: {
+    const { wroteFile, result } = await runUpdateNotifier(t, { mocks: {
       'ci-info': { isCI: true, name: 'something' },
-    } })
+    },
+    PACOTE_MOCK_REQ_COUNT: 0 })
     t.equal(wroteFile, false)
     t.equal(result, null)
-    t.strictSame(MANIFEST_REQUEST, [], 'no requests for manifests')
   })
 
   t.test('only check weekly for GA releases', async t => {
     // One week (plus five minutes to account for test environment fuzziness)
     const STAT_MTIME = Date.now() - 1000 * 60 * 60 * 24 * 7 + 1000 * 60 * 5
-    const { wroteFile, result, MANIFEST_REQUEST } = await runUpdateNotifier(t, { STAT_MTIME })
+    const { wroteFile, result } = await runUpdateNotifier(t, {
+      STAT_MTIME,
+      PACOTE_MOCK_REQ_COUNT: 0,
+    })
     t.equal(wroteFile, false, 'duration was not reset')
     t.equal(result, null)
-    t.strictSame(MANIFEST_REQUEST, [], 'no requests for manifests')
   })
 
   t.test('only check daily for betas', async t => {
@@ -209,37 +231,48 @@ t.test('situations in which we do not notify', t => {
     const {
       wroteFile,
       result,
-      MANIFEST_REQUEST,
-    } = await runUpdateNotifier(t, { STAT_MTIME, version: HAVE_BETA })
+    } = await runUpdateNotifier(t, { STAT_MTIME, version: HAVE_BETA, PACOTE_MOCK_REQ_COUNT: 0 })
     t.equal(wroteFile, false, 'duration was not reset')
     t.equal(result, null)
-    t.strictSame(MANIFEST_REQUEST, [], 'no requests for manifests')
   })
 
   t.end()
 })
 
+t.test('notification situation with engine compatibility', async t => {
+  // no version which are greater than node 1.0.0 should be selected.
+  mockGlobals(t, { 'process.version': 'v1.0.0' }, { replace: true })
+
+  const {
+    wroteFile,
+    result,
+  } = await runUpdateNotifier(t, {
+    version: NEXT_VERSION_ENGINE_COMPATIBLE_MINOR,
+    PACOTE_MOCK_REQ_COUNT: 1 })
+
+  t.matchSnapshot(result)
+  t.equal(wroteFile, true)
+})
+
 t.test('notification situations', async t => {
   const cases = {
-    [HAVE_BETA]: [`^{V}`],
-    [NEXT_PATCH]: [`latest`, `^{V}`],
-    [NEXT_MINOR]: [`latest`, `^{V}`],
-    [CURRENT_PATCH]: ['latest'],
-    [CURRENT_MINOR]: ['latest'],
-    [CURRENT_MAJOR]: ['latest'],
+    [HAVE_BETA]: 1,
+    [NEXT_PATCH]: 2,
+    [NEXT_MINOR]: 2,
+    [CURRENT_PATCH]: 1,
+    [CURRENT_MINOR]: 1,
+    [CURRENT_MAJOR]: 1,
   }
 
-  for (const [version, reqs] of Object.entries(cases)) {
+  for (const [version, requestCount] of Object.entries(cases)) {
     for (const color of [false, 'always']) {
       await t.test(`${version} - color=${color}`, async t => {
         const {
           wroteFile,
           result,
-          MANIFEST_REQUEST,
-        } = await runUpdateNotifier(t, { version, color })
+        } = await runUpdateNotifier(t, { version, color, PACOTE_MOCK_REQ_COUNT: requestCount })
         t.matchSnapshot(result)
         t.equal(wroteFile, true)
-        t.strictSame(MANIFEST_REQUEST, reqs.map(r => `npm@${r.replace('{V}', version)}`))
       })
     }
   }