Add new subcommand `npm doctor` #14582

watilde · 2016-11-10T08:39:27Z

From here #6756 (comment)

this screenshot was updated with current progress at 4th December 2016

ToDo:

Add new subcommand
Write a test
Replace getLatestNodejsVersion with a fixturable way
Update docs

legodude17 · 2016-11-10T17:25:25Z

Does this add any new deps?

legodude17 · 2016-11-10T17:27:40Z

lib/doctor.js

Wouldn't it be 'npm@' + npmV?

othiym23 · 2016-11-14T22:10:26Z

@watilde I owe you a response! I'll try to get to it today! The summary is this looks fantastic, but we need to talk a lil more about the functionality everybody wants.

legodude17 · 2016-11-14T22:24:23Z

Current comments:

Any ideas on a more exhaustive test then, "It prints things out with 5 lines"? I was thinking maybe make sure it prints a table and that the columns match.
What is the best way to get the latest node version? I think that yours is the best way.

watilde · 2016-11-15T06:19:37Z

@legodude17 Yes, that's a good point. Before that, I'm thinking about a way to make the test having a fixture in test.

@othiym23 Sure thing! Let's continue the discussion at #6756

legodude17 · 2016-11-20T20:00:40Z

@watilde is this still in-progress?

watilde · 2016-11-21T13:53:22Z

@legodude17 It's fixed technically for the original plan, but the code potentially will be changed depending on what kind of request comes from the issue thread. Then the status doesn't matter anymore :)

legodude17 · 2016-11-23T23:12:55Z

One suggestion from #6756 is to offer to fix it like Would you like me to run npm cache clean for you?. I think that would be great, but maybe in a separate PR.

othiym23

@watilde, my apologies for the lengthy delay. It's been a bruiser of a month.

Here, in no particular order, are my thoughts:

I'm not in love with the idea of prescribing that users nuke their caches every time they run npm doctor. It makes installs (potentially much) slower until the cache rewarms, and in cases in which the user is on a flaky internet connection, can actually make installs even flakier. If at all possible, I'd like to move to advising people to only npm cache clean individual broken dependencies.

It's probably out of scope for this initial spike, but something that validates that the cache is in good shape (and maybe went ahead and cleaned up any shasum mismatches automatically) would be a super useful alternative.
Something that validates that $HOME/.npm and all its children is owned and writable by $USER would be very helpful for diagnostic purposes.
Similarly, something that checks whether join(npm.config.globalPrefix, 'lib', 'node_modules') is writable and tells the user whether they need to use sudo to run global installs would be very helpful.
It would be very helpful to verify that Git is installed and available, especially for Windows users.
This is another thing that doesn't have to be added yet, but something that builds a very simple canned native module with node-gyp would be extremely helpful for Windows users unsure whether they have their toolchain correctly installed.
Finally, and longest-term, it would be fantastic to have some way for npm to verify its own integrity, which probably requires that some kind of install manifest be generated for it. While it would be useful to have this functionality for npm packages in general, it's especially important for npm, because of the Node.js installer's bad habit of just copying the new npm right over the top of the old one, which causes a bunch of the stranger module not found bugs that show up on the issue tracker. Being able to catch this automatically would be hugely useful.

This is basically my wishlist, though. What you've got is a fantastic start, and when you get the above comments addressed, I'll ship what you've got so far. Thank you so much for putting this together!

othiym23 · 2016-11-28T19:44:24Z

lib/doctor.js

You're going to want to use a semver range comparator here, because something weird will happen as soon as Node 10 is released. 👍 on the check for LTS, though – that's exactly what I was looking for.

othiym23 · 2016-11-28T19:45:35Z

doc/cli/npm-doctor.md

This should be expanded to explain all of the questions and answers, as well as the team's consensus about rationales for the recommendations.

othiym23 · 2016-11-28T19:47:30Z

doc/cli/npm-doctor.md

It would be cool if npm doctor produced output in a format similar enough to Markdown that the output could just be cut and pasted directly into a new issue.

I think the best way to do that would be to template it, but I am not sure about what anyone's thoughts are about including a template engine with npm.

That's absolutly good idea, and I'd like to have the feature in npm report command which have been proposaling at #5214. I'm planning to use the ISSUE_TEMPLATE.md as a acutual template, but let's separete the reporting part from this diagnosis part to make the initial scope clear and simple.

othiym23 · 2016-11-28T19:48:11Z

lib/doctor.js

A lot of these aren't really questions, so I suggest something like Check, Value, and Recommendation.

Good point @othiym23.

True! I've updated :)

othiym23 · 2016-11-28T19:50:34Z

test/tap/doctor.js

Shouldn't this use the url initialized above?

That's right! I've updated.

othiym23 · 2016-11-28T19:55:11Z

test/tap/doctor.js

The test should verify that npm doctor is producing sensible output, and should also start up a mock registry that allows the test to control which version of npm is returned.

Yeah it should be since npm doctor ask the things to the registry through npm ping and fetchPackageMetadata. I've added npm-registry-mock into this test and verifying all of the output!

othiym23 · 2016-11-28T19:56:54Z

lib/doctor.js

I'd prefer this to pull d['dist-tags'].latest instead, so that npm doctor always recommends latest instead of next.

So cb(er, d['dist-tags'].latest)?

I've updated with npm@latest check.

othiym23 · 2016-11-28T19:57:57Z

lib/doctor.js

...but this is part of why we need to be matching against npm doctor actually returns in the tests, because it seems very possible to me that changes made to the configuration code could break this command without anyone noticing otherwise.

Added it to the test! Once the structure of default properties are changed, the test makes an error.

watilde · 2016-12-01T21:47:59Z

@othiym23 Hey Forrest, thanks for your thoughtful wish list! I'd like to separate examination processes and auto-fix features and let me reply to every wish-item of yours.

Agreed! This request is the rest of the things I'd like to put into this PR.
I've added into this PR already.
I've added into this PR already.
I've added into this PR already.
I'd like to have node-gyp doctor instead of putting into npm doctor. Let's take out this from the current initial scope.
This request may make this PR turning into a giant case of scope creep like you said. Could we also take out this from the current initial scope?

Additionally, I've added one more permission check for join(npm.config.localPrefix, 'lib', 'node_modules') since I was missing! As the initial plan to iterate from it, let's start with the following check-list as the smallest subset of information necessary to improve the supportability of npm.

npm ping
npm -v
node -v
npm config get registry
which git
Perms check on cached files
Perms check on join(npm.config.globalPrefix, 'lib', 'node_modules')
Perms check on join(npm.config.localPrefix, 'lib', 'node_modules')
Shasum check for cached packages

Let me ping you when they're ready.

legodude17 · 2016-12-02T01:31:03Z

Some random ideas for things to add:

Ensure the proxy is setup correctly (somehow)
Ensure git is present and works

Keep up the great work @watilde!

watilde · 2016-12-04T19:07:15Z

@othiym23 I've added some check items that I wrote down at #14582 (comment), and also written up the detailed documentation for the npm doctor command! Could I possibly request your review again?

(๑˃̵ᴗ˂̵)و let me cc to you as well @zkat

This command will diagnose user's environment and let the user know some recommended solutions if they potentially have any problems related to npm.

othiym23 · 2016-12-12T06:29:27Z

I think this will be ready in time to include in this week's release, which might end up being the last release of the CLI in 2016! I only have a few further comments:

Right now, the command seems to hang while it's running (I'm going to guess most of that is due to checking the shasums in the cache). If you add a log statement to each check at loglevel info and enable the progress bar, this will let users know that in fact something is happening.
Because validating the hashes can be very time-consuming if people have a lot of packages in their cache, it might make sense to log at level debug each shasum you're validating, especially because you're doing so synchronously. Because the completion routine returns the number of files to check, it should be pretty easy to set up a progress group that will track the process of validating the cache. (This is a nice to have, and if it turns out to be tricky, don't worry about it and somebody can add it in later.)
In the next day or two, I'll make a pass at expanding the docs you've written thus far, and make sure they'll tie in nicely with the the new TROUBLESHOOTING.md that @snopeks and @zkat are in the process of assembling.

This is really great! I can't wait to get it out into people's hands. Thank you so much for your patience and careful work, Daijiro!

iarna · 2016-12-14T00:27:28Z

lib/doctor.js

+  var nodeLTS = p[2].replace('v', '')
+  var whichGit = p[3] || 'not installed'
+  var readbleCaches = p[4] ? 'ok' : 'notOk'
+  var executableGlobalModules = p[4] ? 'ok' : 'notOk'


Shouldn't these from this line down be 5, 6 & 7?

iarna · 2016-12-14T00:31:24Z

lib/doctor/checksum-cached-files.js

+      if (!/.tgz$/.test(file)) return
+      var tgz = fs.readFileSync(file)
+      try {
+        var pkgJSON = fs.readFileSync(path.join(file, '../package/package.json'))


I think what's wanted here is: path.join(path.dirname(file), 'package/package.json')

Alternatively, I think: path.resolve(file, '../package/package.json')

Would have worked, but it's sufficiently weird that I prefer the former.

It also turns out that it's entirely ok for the package.json to be missing, in which case we ought to skip trying to verify the shasum.

iarna · 2016-12-14T00:36:17Z

lib/doctor/checksum-cached-files.js

+      var tgz = fs.readFileSync(file)
+      try {
+        var pkgJSON = fs.readFileSync(path.join(file, '../package/package.json'))
+        var shasum = JSON.parse(pkgJSON).dist.shasum


dist is added by the registry and won't be present for modules installed from other sources. _shasum should be available however.

iarna · 2016-12-14T01:10:47Z

lib/doctor/check-files-permission.js

+
+function checkFilesPermission (root, mask, cb) {
+  var accessible = true
+  if (process.platform === 'win32') return cb(null, !accessible)


This should probably be calling back with true shouldn't it? We don't want it to mark windows as always failing right?

iarna · 2016-12-14T01:26:17Z

lib/doctor/check-files-permission.js

+    files.some(function (f) {
+      if (!accessible) return true
+      var file = path.join(root, f)
+      fs.stat(file, function (e, stat) {


fs.stat is async and files.some is sync, they can't be used together.

iarna · 2016-12-14T01:28:54Z

lib/doctor/check-files-permission.js

+var getUid = require('uid-number')
+var npm = require('../npm.js')
+
+function checkFilesPermission (root, mask, cb) {


mask isn't being used like a umask which I think is the cause of an issue in how this function is being called…

iarna · 2016-12-14T01:30:09Z

lib/doctor.js

+    [getLatestNpmVersion],
+    [getLatestNodejsVersion, args['node-url']],
+    [which, 'git'],
+    [checkFilesPermission, npm.cache, 4],


filemode & 0400 would be true for files you have read access to. Should this be rewrite instead? (6)

iarna · 2016-12-14T01:31:11Z

lib/doctor.js

+    [getLatestNodejsVersion, args['node-url']],
+    [which, 'git'],
+    [checkFilesPermission, npm.cache, 4],
+    [checkFilesPermission, globalNodeModules, 1],


filemode & 0100 is true if you have execute access to. I'm assuming that this is supposed to be checking for read access? (4)

iarna · 2016-12-14T01:54:07Z

I put together a little patch that fixes the bugs that I encountered and reworks things to allow progress bars. You can see that here:

9fc0704

watilde · 2016-12-14T10:57:17Z

@iarna Thanks for your comments, and your patch seems so brilliant.. Let me do cherry-pick it into this PR, or you also can handle the commits to be suited for next-release.

@watilde

Credit: @watilde Reviewed-By: @othiym23 Reviewed-By: @iarna PR-URL: #14582 Fixes: #6756

@watilde

This command will diagnose user's environment and let the user know some recommended solutions if they potentially have any problems related to npm. Credit: @watilde Reviewed-By: @othiym23 Reviewed-By: @iarna PR-URL: #14582 Fixes: #6756

iarna · 2016-12-16T21:46:41Z

Thanks again for this! It was merged and released in 4.1.0.

legodude17 reviewed Nov 10, 2016

View reviewed changes

lib/doctor.js Outdated

Copy link

Contributor

legodude17 Nov 10, 2016

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Wouldn't it be 'npm@' + npmV?

zkat added in-progress needs-discussion labels Nov 14, 2016

othiym23 self-assigned this Nov 14, 2016

watilde force-pushed the feature/doctor-wombat branch from 1b49329 to 6333bc7 Compare November 19, 2016 06:50

watilde changed the title ~~[WIP] Add new subcommand npm doctor~~ Add new subcommand npm doctor Nov 20, 2016

othiym23 suggested changes Nov 28, 2016

View reviewed changes

zkat added needs-docs and removed needs-discussion labels Dec 1, 2016

util: put ansiTrim into util from outdated

ba5d73d

watilde force-pushed the feature/doctor-wombat branch from fe36ada to 86b0824 Compare December 4, 2016 16:47

doctor: add new subcommand

d8b1b45

This command will diagnose user's environment and let the user know some recommended solutions if they potentially have any problems related to npm.

watilde force-pushed the feature/doctor-wombat branch from c9002da to d8b1b45 Compare December 7, 2016 20:39

iarna added this to the next milestone Dec 12, 2016

iarna reviewed Dec 14, 2016

View reviewed changes

fixup: add progress bars, fix review bugs (to be squashed)

2954e71

iarna pushed a commit that referenced this pull request Dec 15, 2016

util: put ansiTrim into util from outdated

0209ee5

Credit: @watilde Reviewed-By: @othiym23 Reviewed-By: @iarna PR-URL: #14582 Fixes: #6756

reconbot mentioned this pull request Dec 16, 2016

npm doctor outline #6756

Closed

iarna closed this Dec 16, 2016

watilde deleted the feature/doctor-wombat branch December 16, 2016 23:53

zkat mentioned this pull request Jan 26, 2017

deps: upgrade npm to 4.1.2 nodejs/node#11020

Closed

4 tasks

Add new subcommand npm doctor #14582

Add new subcommand npm doctor #14582

Uh oh!

Conversation

watilde commented Nov 10, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

legodude17 commented Nov 10, 2016

Uh oh!

Choose a reason for hiding this comment

Uh oh!

othiym23 commented Nov 14, 2016

Uh oh!

legodude17 commented Nov 14, 2016

Uh oh!

watilde commented Nov 15, 2016

Uh oh!

legodude17 commented Nov 20, 2016

Uh oh!

watilde commented Nov 21, 2016

Uh oh!

legodude17 commented Nov 23, 2016

Uh oh!

othiym23 left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

watilde Dec 4, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

watilde commented Dec 1, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

legodude17 commented Dec 2, 2016

Uh oh!

watilde commented Dec 4, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

othiym23 commented Dec 12, 2016

Uh oh!

Choose a reason for hiding this comment

Uh oh!

iarna Dec 14, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Add new subcommand `npm doctor` #14582

Add new subcommand `npm doctor` #14582

watilde commented Nov 10, 2016 •

edited

Loading

watilde Dec 4, 2016 •

edited

Loading

watilde commented Dec 1, 2016 •

edited

Loading

watilde commented Dec 4, 2016 •

edited

Loading

iarna Dec 14, 2016 •

edited

Loading

iarna commented Dec 14, 2016 •

edited

Loading