Skip to content

Commit 81c95c7

Browse files
wraithgarwraithgar
authored
fix: don't reset update notifier duration on every check (#7063)
Instead only reset if we actually checked the registry for a new version.
1 parent f696b51 commit 81c95c7

File tree

2 files changed

+58
-26
lines changed

2 files changed

+58
-26
lines changed

‎lib/utils/update-notifier.js

+8-9
Original file line numberDiff line numberDiff line change
@@ -98,11 +98,16 @@ const updateNotifier = async (npm, spec = 'latest') => {
9898
return null
9999
}
100100

101+
// intentional. do not await this. it's a best-effort update. if this
102+
// fails, it's ok. might be using /dev/null as the cache or something weird
103+
// like that.
104+
writeFile(lastCheckedFile(npm), '').catch(() => {})
105+
101106
return updateCheck(npm, spec, version, current)
102107
}
103108

104109
// only update the notification timeout if we actually finished checking
105-
module.exports = async npm => {
110+
module.exports = npm => {
106111
if (
107112
// opted out
108113
!npm.config.get('update-notifier')
@@ -113,14 +118,8 @@ module.exports = async npm => {
113118
// CI
114119
|| ciInfo.isCI
115120
) {
116-
return null
121+
return Promise.resolve(null)
117122
}
118123

119-
const notification = await updateNotifier(npm)
120-
121-
// intentional. do not await this. it's a best-effort update. if this
122-
// fails, it's ok. might be using /dev/null as the cache or something weird
123-
// like that.
124-
writeFile(lastCheckedFile(npm), '').catch(() => {})
125-
return notification
124+
return updateNotifier(npm)
126125
}

‎test/lib/utils/update-notifier.js

+50-17
Original file line numberDiff line numberDiff line change
@@ -23,6 +23,7 @@ const runUpdateNotifier = async (t, {
2323
prefixDir,
2424
version = CURRENT_VERSION,
2525
argv = [],
26+
wroteFile = false,
2627
...config
2728
} = {}) => {
2829
const mockFs = {
@@ -37,6 +38,7 @@ const runUpdateNotifier = async (t, {
3738
return { mtime: new Date(STAT_MTIME) }
3839
},
3940
writeFile: async (path, content) => {
41+
wroteFile = true
4042
if (content !== '') {
4143
t.fail('no write file content allowed')
4244
}
@@ -86,106 +88,132 @@ const runUpdateNotifier = async (t, {
8688
const result = await updateNotifier(mock.npm)
8789

8890
return {
91+
wroteFile,
8992
result,
9093
MANIFEST_REQUEST,
9194
}
9295
}
9396

94-
t.test('does not notify by default', async t => {
95-
const { result, MANIFEST_REQUEST } = await runUpdateNotifier(t)
97+
t.test('duration has elapsed, no updates', async t => {
98+
const { wroteFile, result, MANIFEST_REQUEST } = await runUpdateNotifier(t)
99+
t.equal(wroteFile, true)
96100
t.not(result)
97101
t.equal(MANIFEST_REQUEST.length, 1)
98102
})
99103

100104
t.test('situations in which we do not notify', t => {
101105
t.test('nothing to do if notifier disabled', async t => {
102-
const { result, MANIFEST_REQUEST } = await runUpdateNotifier(t, {
106+
const { wroteFile, result, MANIFEST_REQUEST } = await runUpdateNotifier(t, {
103107
'update-notifier': false,
104108
})
109+
t.equal(wroteFile, false)
105110
t.equal(result, null)
106111
t.strictSame(MANIFEST_REQUEST, [], 'no requests for manifests')
107112
})
108113

109114
t.test('do not suggest update if already updating', async t => {
110-
const { result, MANIFEST_REQUEST } = await runUpdateNotifier(t, {
115+
const { wroteFile, result, MANIFEST_REQUEST } = await runUpdateNotifier(t, {
111116
command: 'install',
112117
prefixDir: { 'package.json': `{"name":"${t.testName}"}` },
113118
argv: ['npm'],
114119
global: true,
115120
})
121+
t.equal(wroteFile, false)
116122
t.equal(result, null)
117123
t.strictSame(MANIFEST_REQUEST, [], 'no requests for manifests')
118124
})
119125

120126
t.test('do not suggest update if already updating with spec', async t => {
121-
const { result, MANIFEST_REQUEST } = await runUpdateNotifier(t, {
127+
const { wroteFile, result, MANIFEST_REQUEST } = await runUpdateNotifier(t, {
122128
command: 'install',
123129
prefixDir: { 'package.json': `{"name":"${t.testName}"}` },
124130
argv: ['npm@latest'],
125131
global: true,
126132
})
133+
t.equal(wroteFile, false)
127134
t.equal(result, null)
128135
t.strictSame(MANIFEST_REQUEST, [], 'no requests for manifests')
129136
})
130137

131138
t.test('do not update if same as latest', async t => {
132-
const { result, MANIFEST_REQUEST } = await runUpdateNotifier(t)
139+
const { wroteFile, result, MANIFEST_REQUEST } = await runUpdateNotifier(t)
140+
t.equal(wroteFile, true)
133141
t.equal(result, null)
134142
t.strictSame(MANIFEST_REQUEST, ['npm@latest'], 'requested latest version')
135143
})
136144
t.test('check if stat errors (here for coverage)', async t => {
137145
const STAT_ERROR = new Error('blorg')
138-
const { result, MANIFEST_REQUEST } = await runUpdateNotifier(t, { STAT_ERROR })
146+
const { wroteFile, result, MANIFEST_REQUEST } = await runUpdateNotifier(t, { STAT_ERROR })
147+
t.equal(wroteFile, true)
139148
t.equal(result, null)
140149
t.strictSame(MANIFEST_REQUEST, ['npm@latest'], 'requested latest version')
141150
})
142151
t.test('ok if write errors (here for coverage)', async t => {
143152
const WRITE_ERROR = new Error('grolb')
144-
const { result, MANIFEST_REQUEST } = await runUpdateNotifier(t, { WRITE_ERROR })
153+
const { wroteFile, result, MANIFEST_REQUEST } = await runUpdateNotifier(t, { WRITE_ERROR })
154+
t.equal(wroteFile, true)
145155
t.equal(result, null)
146156
t.strictSame(MANIFEST_REQUEST, ['npm@latest'], 'requested latest version')
147157
})
148158
t.test('ignore pacote failures (here for coverage)', async t => {
149159
const PACOTE_ERROR = new Error('pah-KO-tchay')
150-
const { result, MANIFEST_REQUEST } = await runUpdateNotifier(t, { PACOTE_ERROR })
160+
const { wroteFile, result, MANIFEST_REQUEST } = await runUpdateNotifier(t, { PACOTE_ERROR })
151161
t.equal(result, null)
162+
t.equal(wroteFile, true)
152163
t.strictSame(MANIFEST_REQUEST, ['npm@latest'], 'requested latest version')
153164
})
154165
t.test('do not update if newer than latest, but same as next', async t => {
155-
const { result, MANIFEST_REQUEST } = await runUpdateNotifier(t, { version: NEXT_VERSION })
166+
const {
167+
wroteFile,
168+
result,
169+
MANIFEST_REQUEST,
170+
} = await runUpdateNotifier(t, { version: NEXT_VERSION })
156171
t.equal(result, null)
172+
t.equal(wroteFile, true)
157173
const reqs = ['npm@latest', `npm@^${NEXT_VERSION}`]
158174
t.strictSame(MANIFEST_REQUEST, reqs, 'requested latest and next versions')
159175
})
160176
t.test('do not update if on the latest beta', async t => {
161-
const { result, MANIFEST_REQUEST } = await runUpdateNotifier(t, { version: CURRENT_BETA })
177+
const {
178+
wroteFile,
179+
result,
180+
MANIFEST_REQUEST,
181+
} = await runUpdateNotifier(t, { version: CURRENT_BETA })
162182
t.equal(result, null)
183+
t.equal(wroteFile, true)
163184
const reqs = [`npm@^${CURRENT_BETA}`]
164185
t.strictSame(MANIFEST_REQUEST, reqs, 'requested latest and next versions')
165186
})
166187

167188
t.test('do not update in CI', async t => {
168-
const { result, MANIFEST_REQUEST } = await runUpdateNotifier(t, { mocks: {
189+
const { wroteFile, result, MANIFEST_REQUEST } = await runUpdateNotifier(t, { mocks: {
169190
'ci-info': { isCI: true, name: 'something' },
170191
} })
192+
t.equal(wroteFile, false)
171193
t.equal(result, null)
172194
t.strictSame(MANIFEST_REQUEST, [], 'no requests for manifests')
173195
})
174196

175197
t.test('only check weekly for GA releases', async t => {
176198
// One week (plus five minutes to account for test environment fuzziness)
177199
const STAT_MTIME = Date.now() - 1000 * 60 * 60 * 24 * 7 + 1000 * 60 * 5
178-
const { result, MANIFEST_REQUEST } = await runUpdateNotifier(t, { STAT_MTIME })
200+
const { wroteFile, result, MANIFEST_REQUEST } = await runUpdateNotifier(t, { STAT_MTIME })
201+
t.equal(wroteFile, false, 'duration was not reset')
179202
t.equal(result, null)
180203
t.strictSame(MANIFEST_REQUEST, [], 'no requests for manifests')
181204
})
182205

183206
t.test('only check daily for betas', async t => {
184207
// One day (plus five minutes to account for test environment fuzziness)
185208
const STAT_MTIME = Date.now() - 1000 * 60 * 60 * 24 + 1000 * 60 * 5
186-
const res = await runUpdateNotifier(t, { STAT_MTIME, version: HAVE_BETA })
187-
t.equal(res.result, null)
188-
t.strictSame(res.MANIFEST_REQUEST, [], 'no requests for manifests')
209+
const {
210+
wroteFile,
211+
result,
212+
MANIFEST_REQUEST,
213+
} = await runUpdateNotifier(t, { STAT_MTIME, version: HAVE_BETA })
214+
t.equal(wroteFile, false, 'duration was not reset')
215+
t.equal(result, null)
216+
t.strictSame(MANIFEST_REQUEST, [], 'no requests for manifests')
189217
})
190218

191219
t.end()
@@ -204,8 +232,13 @@ t.test('notification situations', async t => {
204232
for (const [version, reqs] of Object.entries(cases)) {
205233
for (const color of [false, 'always']) {
206234
await t.test(`${version} - color=${color}`, async t => {
207-
const { result, MANIFEST_REQUEST } = await runUpdateNotifier(t, { version, color })
235+
const {
236+
wroteFile,
237+
result,
238+
MANIFEST_REQUEST,
239+
} = await runUpdateNotifier(t, { version, color })
208240
t.matchSnapshot(result)
241+
t.equal(wroteFile, true)
209242
t.strictSame(MANIFEST_REQUEST, reqs.map(r => `npm@${r.replace('{V}', version)}`))
210243
})
211244
}

0 commit comments

Comments
 (0)