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

replace update-notifier #61

wants to merge 3 commits into from

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 as a code owner Aug 28, 2018
@zkat
Copy link
Contributor

@zkat 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 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 😃

@olore olore force-pushed the remove-update-notifier branch from 90db349 to 228abd0 Aug 29, 2018
@olore olore force-pushed the remove-update-notifier branch from 228abd0 to 9902590 Aug 31, 2018
@olore
Copy link
Contributor Author

@olore 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

@ewanharris ewanharris commented Sep 25, 2018

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 olore commented Sep 27, 2018

@ewanharris - yes the previous flag is still supported

@olore
Copy link
Contributor Author

@olore 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 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 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

@mikemimik mikemimik commented Nov 19, 2019

@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
Copy link
Member

@darcyclarke darcyclarke commented Jul 31, 2020

@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 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
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

5 participants