Repository navigation
Conversation
There was a problem hiding this comment.
🟡 Changes recommended
The exit handler can discard a notice queued on stderr under backpressure while still marking it shown, suppressing further notifications for a day.
1 open issue
What changed in this PR
This PR changes npm’s update notifier to record when a notice is displayed rather than when a check starts, allowing later commands to retry notifications that were not shown.
Changes:
- Return notifications as
{ message, onShown }and write the sentinel through the callback. - Use a daily interval for both stable and prerelease versions, without recording silent notifications.
- Update tests for notification handoff, display callbacks, and sentinel timing.
| File | Description |
|---|---|
test/lib/cli/update-notifier.js |
Tests deferred sentinel writes and daily check intervals. |
test/lib/cli/exit-handler.js |
Tests displayed, silent, and failing notification callbacks. |
test/lib/cli/entry.js |
Tests notification-object handoff to npm. |
lib/cli/update-notifier.js |
Returns a display callback and adopts daily checks. |
lib/cli/exit-handler.js |
Calls the notification callback after logging, except in silent mode. |
lib/cli/entry.js |
Stores the returned notification object. |
Review effort: Balanced
Previously the `_update-notifier-last-checked` sentinel was written as
soon as a check *started*, regardless of whether a notice was ever
shown to the user. This meant fast commands (e.g. `npm version`) would
silently consume the check window without the user ever seeing the
update notice, since the sentinel's mtime only ever meant "a check was
attempted", not "a notice was shown".
This redefines the sentinel's mtime to mean "last time a notice was
actually displayed":
- `updateCheck()` now returns `{ message, onShown }` instead of a bare
message string. The sentinel is written lazily via `onShown()`,
invoked from the exit handler synchronously (via `writeFileSync`,
required because `process.on('exit', ...)` can't await async work)
only after the notice has actually been logged.
- The weekly/daily split between GA and prerelease checks is removed
in favor of a single daily check for both, since the throttle is now
tied to actual visibility rather than check attempts.
- `npm.updateNotification` is now a single bundled object
(`{ message, onShown }`) instead of a bare string, removing the need
for a separate `updateNotificationShown` field and the duplication
that came with keeping two npm instance fields in sync.
- When silenced (`npm.silent`), the notice is not shown and the
sentinel is not written, so a later non-silent run will still get a
chance to show (and record) the notice.
- Node.js engine compatibility continues to be evaluated fresh on
every run (never cached), since the active Node version can change
between invocations (e.g. via nvm).
Ref: github/npm#15901 (comment)
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
…r path Full-suite coverage caught two branches the initial tests missed: - entry.js: the branch that actually assigns npm.updateNotification when the notifier resolves a real notification (previously only exercised the null/no-op path since update-notifier.js isn't mocked in entry.js's own tests). - exit-handler.js: the try/catch around onShown() when it throws, which only logs a verbose message and must not crash the exit path. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
nkolev92
force-pushed
the
nkolev92/update-notifier-sentinel-on-shown
branch
from
October 30, 2026 00:25
b20ceee to
3c4b299
Compare
…rocess.exit() to ensure the message is shown more consistently.
Record eligible notifications in silent mode and skip recording when stderr flushing fails. Preserve the existing exit code and cover both paths with regression tests. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Copilot-Session: b7af3fb9-3cc3-40e6-9d7f-dbb8eee631cd
nkolev92
changed the title
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Copilot-Session: 5f9e5ec7-eea1-4067-8157-b9368a874338
jeffkl
approved these changes
Nov 4, 2026
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Avoid consuming the update-notifier throttle when a command exits before its notification is ready.
updateCheck()returns{ message, onShown }, deferring the sentinel write until the notification is handled at exit.Behavior
log notice -> flush streams -> record sentinel -> process.exit(). Late results arriving during the flush are not recorded.Checks without a handled notice retry on every eligible run, even when no update is available, potentially increasing registry traffic.
Progress on internal issue 15901