diff --git a/lib/cli.js b/lib/cli.js index f42132f944390..d4a67645858ae 100644 --- a/lib/cli.js +++ b/lib/cli.js @@ -53,7 +53,7 @@ module.exports = (process) => { npm.config.set('usage', false, 'cli') } - npm.updateNotification = await updateNotifier(npm) + updateNotifier(npm) const cmd = npm.argv.shift() const impl = npm.commands[cmd] diff --git a/lib/utils/error-handler.js b/lib/utils/error-handler.js index 1fc31df44ffb9..da716679d2705 100644 --- a/lib/utils/error-handler.js +++ b/lib/utils/error-handler.js @@ -119,7 +119,9 @@ const errorHandler = (er) => { if (cbCalled) er = er || new Error('Callback called more than once.') - if (npm.updateNotification) { + // only show the notification if it finished before the other stuff we + // were doing. no need to hang on `npm -v` or something. + if (typeof npm.updateNotification === 'string') { const { level } = log log.level = log.levels.notice log.notice('', npm.updateNotification) diff --git a/lib/utils/update-notifier.js b/lib/utils/update-notifier.js index 1af948a2db45a..ed5806ced2a7d 100644 --- a/lib/utils/update-notifier.js +++ b/lib/utils/update-notifier.js @@ -21,23 +21,25 @@ const isGlobalNpmUpdate = npm => { const DAILY = 1000 * 60 * 60 * 24 const WEEKLY = DAILY * 7 -const updateTimeout = async (npm, duration) => { +// don't put it in the _cacache folder, just in npm's cache +const lastCheckedFile = npm => + resolve(npm.flatOptions.cache, '../_update-notifier-last-checked') + +const checkTimeout = async (npm, duration) => { const t = new Date(Date.now() - duration) - // don't put it in the _cacache folder, just in npm's cache - const f = resolve(npm.flatOptions.cache, '../_update-notifier-last-checked') + const f = lastCheckedFile(npm) // if we don't have a file, then definitely check it. const st = await stat(f).catch(() => ({ mtime: t - 1 })) + return t > st.mtime +} - if (t > st.mtime) { - // best effort, if this fails, it's ok. - // might be using /dev/null as the cache or something weird like that. - await writeFile(f, '').catch(() => {}) - return true - } else - return false +const updateTimeout = async npm => { + // best effort, if this fails, it's ok. + // might be using /dev/null as the cache or something weird like that. + await writeFile(lastCheckedFile(npm), '').catch(() => {}) } -const updateNotifier = module.exports = async (npm, spec = 'latest') => { +const updateNotifier = async (npm, spec = 'latest') => { // never check for updates in CI, when updating npm already, or opted out if (!npm.config.get('update-notifier') || isGlobalNpmUpdate(npm) || @@ -57,7 +59,7 @@ const updateNotifier = module.exports = async (npm, spec = 'latest') => { const duration = spec !== 'latest' ? DAILY : WEEKLY // if we've already checked within the specified duration, don't check again - if (!(await updateTimeout(npm, duration))) + if (!(await checkTimeout(npm, duration))) return null // if they're currently using a prerelease, nudge to the next prerelease @@ -113,3 +115,11 @@ const updateNotifier = module.exports = async (npm, spec = 'latest') => { return messagec } + +// only update the notification timeout if we actually finished checking +module.exports = async npm => { + const notification = await updateNotifier(npm) + // intentional. do not await this. it's a best-effort update. + updateTimeout(npm) + npm.updateNotification = notification +} diff --git a/test/lib/cli.js b/test/lib/cli.js index f491c6174b85e..42e05cc5d14c3 100644 --- a/test/lib/cli.js +++ b/test/lib/cli.js @@ -45,6 +45,7 @@ const npmlogMock = { const cli = t.mock('../../lib/cli.js', { '../../lib/npm.js': npmock, + '../../lib/utils/update-notifier.js': async () => null, '../../lib/utils/did-you-mean.js': () => '\ntest did you mean', '../../lib/utils/unsupported.js': unsupportedMock, '../../lib/utils/error-handler.js': errorHandlerMock, diff --git a/test/lib/utils/update-notifier.js b/test/lib/utils/update-notifier.js index 9748a92a8ae7b..dc0a64ff46127 100644 --- a/test/lib/utils/update-notifier.js +++ b/test/lib/utils/update-notifier.js @@ -86,9 +86,14 @@ t.afterEach(() => { WRITE_ERROR = null }) +const runUpdateNotifier = async npm => { + await updateNotifier(npm) + return npm.updateNotification +} + t.test('situations in which we do not notify', t => { t.test('nothing to do if notifier disabled', async t => { - t.equal(await updateNotifier({ + t.equal(await runUpdateNotifier({ ...npm, config: { get: (k) => k !== 'update-notifier' }, }), null) @@ -96,7 +101,7 @@ t.test('situations in which we do not notify', t => { }) t.test('do not suggest update if already updating', async t => { - t.equal(await updateNotifier({ + t.equal(await runUpdateNotifier({ ...npm, flatOptions: { ...flatOptions, global: true }, command: 'install', @@ -106,7 +111,7 @@ t.test('situations in which we do not notify', t => { }) t.test('do not suggest update if already updating with spec', async t => { - t.equal(await updateNotifier({ + t.equal(await runUpdateNotifier({ ...npm, flatOptions: { ...flatOptions, global: true }, command: 'install', @@ -116,31 +121,31 @@ t.test('situations in which we do not notify', t => { }) t.test('do not update if same as latest', async t => { - t.equal(await updateNotifier(npm), null) + t.equal(await runUpdateNotifier(npm), null) t.strictSame(MANIFEST_REQUEST, ['npm@latest'], 'requested latest version') }) t.test('check if stat errors (here for coverage)', async t => { STAT_ERROR = new Error('blorg') - t.equal(await updateNotifier(npm), null) + t.equal(await runUpdateNotifier(npm), null) t.strictSame(MANIFEST_REQUEST, ['npm@latest'], 'requested latest version') }) t.test('ok if write errors (here for coverage)', async t => { WRITE_ERROR = new Error('grolb') - t.equal(await updateNotifier(npm), null) + t.equal(await runUpdateNotifier(npm), null) t.strictSame(MANIFEST_REQUEST, ['npm@latest'], 'requested latest version') }) t.test('ignore pacote failures (here for coverage)', async t => { PACOTE_ERROR = new Error('pah-KO-tchay') - t.equal(await updateNotifier(npm), null) + t.equal(await runUpdateNotifier(npm), null) t.strictSame(MANIFEST_REQUEST, ['npm@latest'], 'requested latest version') }) t.test('do not update if newer than latest, but same as next', async t => { - t.equal(await updateNotifier({ ...npm, version: NEXT_VERSION }), null) + t.equal(await runUpdateNotifier({ ...npm, version: NEXT_VERSION }), null) 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 => { - t.equal(await updateNotifier({ ...npm, version: CURRENT_BETA }), null) + t.equal(await runUpdateNotifier({ ...npm, version: CURRENT_BETA }), null) const reqs = [`npm@^${CURRENT_BETA}`] t.strictSame(MANIFEST_REQUEST, reqs, 'requested latest and next versions') }) @@ -150,21 +155,21 @@ t.test('situations in which we do not notify', t => { ciMock = null }) ciMock = 'something' - t.equal(await updateNotifier(npm), null) + t.equal(await runUpdateNotifier(npm), 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) STAT_MTIME = Date.now() - (1000 * 60 * 60 * 24 * 7) + (1000 * 60 * 5) - t.equal(await updateNotifier(npm), null) + t.equal(await runUpdateNotifier(npm), null) t.strictSame(MANIFEST_REQUEST, [], 'no requests for manifests') }) t.test('only check daily for betas', async t => { // One day (plus five minutes to account for test environment fuzziness) STAT_MTIME = Date.now() - (1000 * 60 * 60 * 24) + (1000 * 60 * 5) - t.equal(await updateNotifier({ ...npm, version: HAVE_BETA }), null) + t.equal(await runUpdateNotifier({ ...npm, version: HAVE_BETA }), null) t.strictSame(MANIFEST_REQUEST, [], 'no requests for manifests') }) @@ -174,43 +179,43 @@ t.test('situations in which we do not notify', t => { t.test('notification situations', t => { t.test('new beta available', async t => { const version = HAVE_BETA - t.matchSnapshot(await updateNotifier({ ...npm, version }), 'color') - t.matchSnapshot(await updateNotifier({ ...npmNoColor, version }), 'no color') + t.matchSnapshot(await runUpdateNotifier({ ...npm, version }), 'color') + t.matchSnapshot(await runUpdateNotifier({ ...npmNoColor, version }), 'no color') t.strictSame(MANIFEST_REQUEST, [`npm@^${version}`, `npm@^${version}`]) }) t.test('patch to next version', async t => { const version = NEXT_PATCH - t.matchSnapshot(await updateNotifier({ ...npm, version }), 'color') - t.matchSnapshot(await updateNotifier({ ...npmNoColor, version }), 'no color') + t.matchSnapshot(await runUpdateNotifier({ ...npm, version }), 'color') + t.matchSnapshot(await runUpdateNotifier({ ...npmNoColor, version }), 'no color') t.strictSame(MANIFEST_REQUEST, ['npm@latest', `npm@^${version}`, 'npm@latest', `npm@^${version}`]) }) t.test('minor to next version', async t => { const version = NEXT_MINOR - t.matchSnapshot(await updateNotifier({ ...npm, version }), 'color') - t.matchSnapshot(await updateNotifier({ ...npmNoColor, version }), 'no color') + t.matchSnapshot(await runUpdateNotifier({ ...npm, version }), 'color') + t.matchSnapshot(await runUpdateNotifier({ ...npmNoColor, version }), 'no color') t.strictSame(MANIFEST_REQUEST, ['npm@latest', `npm@^${version}`, 'npm@latest', `npm@^${version}`]) }) t.test('patch to current', async t => { const version = CURRENT_PATCH - t.matchSnapshot(await updateNotifier({ ...npm, version }), 'color') - t.matchSnapshot(await updateNotifier({ ...npmNoColor, version }), 'no color') + t.matchSnapshot(await runUpdateNotifier({ ...npm, version }), 'color') + t.matchSnapshot(await runUpdateNotifier({ ...npmNoColor, version }), 'no color') t.strictSame(MANIFEST_REQUEST, ['npm@latest', 'npm@latest']) }) t.test('minor to current', async t => { const version = CURRENT_MINOR - t.matchSnapshot(await updateNotifier({ ...npm, version }), 'color') - t.matchSnapshot(await updateNotifier({ ...npmNoColor, version }), 'no color') + t.matchSnapshot(await runUpdateNotifier({ ...npm, version }), 'color') + t.matchSnapshot(await runUpdateNotifier({ ...npmNoColor, version }), 'no color') t.strictSame(MANIFEST_REQUEST, ['npm@latest', 'npm@latest']) }) t.test('major to current', async t => { const version = CURRENT_MAJOR - t.matchSnapshot(await updateNotifier({ ...npm, version }), 'color') - t.matchSnapshot(await updateNotifier({ ...npmNoColor, version }), 'no color') + t.matchSnapshot(await runUpdateNotifier({ ...npm, version }), 'color') + t.matchSnapshot(await runUpdateNotifier({ ...npmNoColor, version }), 'no color') t.strictSame(MANIFEST_REQUEST, ['npm@latest', 'npm@latest']) })