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.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

fix(update-notifier): parallelize check for updates #3348

Closed
wants to merge 1 commit into from
Closed

fix(update-notifier): parallelize check for updates #3348

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion lib/cli.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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]
Expand Down
4 changes: 3 additions & 1 deletion lib/utils/error-handler.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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)
Expand Down
34 changes: 22 additions & 12 deletions lib/utils/update-notifier.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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) ||
Expand All @@ -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
Expand Down Expand Up @@ -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
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

it seems very weird to me to mutate npm outside of the file that defines npm.

}
1 change: 1 addition & 0 deletions test/lib/cli.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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,
Expand Down
53 changes: 29 additions & 24 deletions test/lib/utils/update-notifier.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -86,17 +86,22 @@ 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)
t.strictSame(MANIFEST_REQUEST, [], 'no requests for manifests')
})

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',
Expand All @@ -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',
Expand All @@ -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')
})
Expand All @@ -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')
})

Expand All @@ -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'])
})

Expand Down