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.

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

replace update-notifier #61

Closed
wants to merge 3 commits into from
Closed

Conversation

olore
Copy link
Contributor

@olore olore commented Aug 28, 2018

See discussion on npm community

  • Use a detached process
    • I think because it's using a separate process, we don't need a timeout, but please let me know.
  • Ignore errors
  • Limit how often it does these checks. update-notifier does this by using configstore to write a lastUpdateCheck date and then compares it to opts.updateCheckInterval.
  • Remove update-notifier dependencies from git
    • I removed update-notifier from package.json, but I am not sure how to cleanly remove it and its dependencies because it's in bundleDependencies
  • update-notifier uses boxen to create the nice output, I am not sure we want to add it as a dependency
  • Fix up after 6.4.1 is released (has CI checks prior to running update-notifier)

I tested this by changing package.json version to 6.3.0 :

$ ./bin/npm-cli.js --version
6.3.0

$ ./bin/npm-cli.js uninstall test-version-checker
audited 8072 packages in 9.318s
found 0 vulnerabilities

New minor version of npm available! 6.3.0 → 6.4.0
Changelog: https://github.com/npm/cli/releases/tag/v6.4.0
Run npm install -g npm to update!

@olore olore requested a review from a team as a code owner August 28, 2018 23:55
@zkat
Copy link
Contributor

zkat commented Aug 29, 2018

@olore some pointers wrt your remaining items, in case it helps:

  1. This is super straightforward to do if you use cacache directly. Something like:
const cacache = require('cacache')
const cache = path.join(npm.config.get('cache'), '_cacache') // I know, it's weird. It'll be better soon.

// writing
cacache.put(cache, 'update-notifier:last-check', Date.toUTCString()).then(....)

// reading
cacache.get(cache, 'update-notifier:last-check').then(time => {
  console.log('last check time was', new Date(time.toString('utf8')))
})
  1. Just npm rm update-notifier && git add -A package* node_modules && git commit-m 'update-notifier@REMOVED' and you're all set. It'll remove it from bundleDeps and take care of everything for you.

  2. I think it's fine to keep the boxen dependency. It'll still be a net loss in package size since it was already there. I have no beef with boxen itself.

@olore
Copy link
Contributor Author

olore commented Aug 29, 2018

Thanks @zkat ! This will definitely help.

  1. When I run uninstall there are no changes made to package.json, or anything that I can add/rm/commit
$ npm uninstall update-notifier
npm WARN npm@6.4.0 Non-dependency in bundleDependencies: update-notifier

audited 7968 packages in 8.778s
found 0 vulnerabilities

$ git st
On branch remove-update-notifier
Your branch is up to date with 'olore/remove-update-notifier'.

nothing to commit, working tree clean

I feel like I am just missing something simple 😃

@zkat zkat added in-progress semver:patch semver patch level for changes labels Aug 29, 2018
@olore
Copy link
Contributor Author

olore commented Sep 1, 2018

I think I got everything, it's ready for review.

I still don't think I properly removed update-notifier (it's still in node_modules). Any pointers are appreciated.

@ewanharris
Copy link

Would it be possible to have a way to make sure that this doesn't get run, similar to how the --no-update-notifier flag or the NO_UPDATE_NOTIFIER env var can be used today?

@olore
Copy link
Contributor Author

olore commented Sep 27, 2018

@ewanharris - yes the previous flag is still supported

@olore
Copy link
Contributor Author

olore commented Oct 25, 2018

I don't know how to successfully complete the final task (I created for myself!)
Is it possible this gets taken care of by the build?

If so, this should be good to go. Let me know if there is anything else to do here.

@zkat
Copy link
Contributor

zkat commented Nov 13, 2018

Hey, sorry for taking so long to get back to this.

I have one last request: Can you write a basic test for this? I realize it's gotten big enough that I'm anxious about merging without at least a basic stability check.

@olore
Copy link
Contributor Author

olore commented Nov 14, 2018

@zkat yes I will see what I can do. Back when I started this, I was looking for any tests in this area and came up empty. I'll give it another look. Thanks.

@mikemimik
Copy link
Contributor

@olore we're going to address this in npm@7.x. Our initial thought is to gut that dep out of the cli. We're really focused on performance in npm@7.x, and this seems like some great low hanging fruit!

@darcyclarke darcyclarke added the semver:major backwards-incompatible breaking changes label Mar 3, 2020
@darcyclarke
Copy link
Contributor

@olore sorry for the very long wait here for any updates. We're going to review how we approach this prompt in the next ~month & so I'm going to closing this PR for not but will be reference your work in a net-new issue: #1592

@olore
Copy link
Contributor Author

olore commented Jul 31, 2020

@darcyclarke it's all good! Thanks for the link to the new issue

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Release 7.x work is associated with a specific npm 7 release semver:major backwards-incompatible breaking changes semver:patch semver patch level for changes
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants