new npm-specific update-notifier implementation #1632
Conversation
This drops our usage of the update-notifier module, in favor of checking ourselves, using the modules and UX patterns that npm already has in place. - While on a prerelease version, updates are checked for every day, instead of every week, and always checks for a new beta in the current release family. Ie, ^7.0.0-beta.2 instead of latest. - Latest version is suggested if newer than current. - If current version is newer than latest, then we check again for an update in the current version family. Ie, ^7.0.0 instead of latest, if current is 7.0.0 and latest is 6.x. - Output is printed using log.notice, at the end of all other log output, so that it's both less visually disruptive, and less likely to be missed among other warnings and notices. This has the side effect of requiring that we set npm.flatOptions as soon as config is loaded, rather than waiting for a command to be run. Since the cli runs a command immediately after loading anyway, this is not a relevant change for our purposes, but worth mentioning here.
isaacs
force-pushed
the
isaacs/new-terse-update-notifier
branch
from
d94ed8c to
655964d
Compare
|
New styling: |
| if (!er && !this[_flatOptions]) { | ||
| this[_flatOptions] = require('./config/flat-options.js')(this) | ||
| require('./config/set-envs.js')(this) | ||
| } |
There was a problem hiding this comment.
will this also allow us to move even more configs to flatOptions in the future ?
| if (er) return errorHandler(er) | ||
|
|
||
| updateNotifier(npm) | ||
| npm.updateNotification = await updateNotifier(npm) |
There was a problem hiding this comment.
hmmm this got me thinking... is there a possibility we could move this to a threaded api (workers/child_process/etc) so the cli never has to wait on it? Simply saying: print it if you had the time, don't bother blocking cli exit in case pacote isn't done fetching the packument yet.
Or is it already the case and I'm missing how something about how it works?
There was a problem hiding this comment.
Hm. Yeah, we could do something like:
updateNotifier(npm).then(message => { npm.updateNotification = message })
and then it'll just get killed when the process exits if it doesn't finish in time.
But it means you won't ever get notified if you're running npm ls. Maybe that's good? Have to make sure the timer file doesn't get touched before pacote finishes though.
There was a problem hiding this comment.
So, the updateTimeout method would have to be split into two functions, one to check the timer and another to reset it.
There was a problem hiding this comment.
Maybe that's good?
I do think so! but we can def revisit it later 😊 def not a blocker so I ended up merging the PR 😁
PR-URL: #1632 Credit: @isaacs Close: #1632 Reviewed-by: @ruyadorno
This drops our usage of the update-notifier module, in favor of checking
ourselves, using the modules and UX patterns that npm already has in
place.
instead of every week, and always checks for a new beta in the current
release family. Ie, ^7.0.0-beta.2 instead of latest.
update in the current version family. Ie, ^7.0.0 instead of latest,
if current is 7.0.0 and latest is 6.x.
output, so that it's both less visually disruptive, and less likely to
be missed among other warnings and notices.
This has the side effect of requiring that we set npm.flatOptions as
soon as config is loaded, rather than waiting for a command to be run.
Since the cli runs a command immediately after loading anyway, this is
not a relevant change for our purposes, but worth mentioning here.