[v7] Update output of npm install command #1017
Conversation
Still a lot to do, and this has a lot of very rought edges.
- Stop sending an HTTP Referer header to the registry - Install global packages properly - Save added packages in the appropriate dep type in package.json - Dedupe npm-registry-fetch and pacote to top level
This is a defense for all cases where we might be trying to open a web url based on either a response from the server, or some form of user input. As far as we can tell, they're all being validated, but defense in depth is always a good idea.
That field has been moved under the 'search' object.
PR-URL: #968 Credit: @isaacs and @mikemimik Close: #968
mikemimik
added
Enhancement
mikemimik
force-pushed
the
mikemimik/feature/npm-install
branch
from
319f189 to
616fa0c
Compare
mikemimik
force-pushed
the
mikemimik/feature/npm-install
branch
from
616fa0c to
e32956f
Compare
| const arbCallOpts = { ...npm.flatOptions, add: args } | ||
|
|
||
| try { | ||
| const start = Date.now() |
There was a problem hiding this comment.
I'd love opinions on this start/stop logic. I don't love it, but I was trying to keep the output from the previous version of the command.
There was a problem hiding this comment.
Yeah, Date.now() is a bit blunt, but probably fine enough accuracy for our purposes. It'd of course be more precise to use process.hrtime(), and get nanosecond precision, but since big installs are sometimes several minutes long, we probably don't need that much precision.
- functionality to get diff and explicitRequets from arborist instace requires @npmcli/[email protected] - create printResult function - output install action time (fairly naively) - added functionality to create and print archy dep tree
mikemimik
force-pushed
the
mikemimik/feature/npm-install
branch
from
e32956f to
1e0ee77
Compare
| } | ||
| } | ||
|
|
||
| function printResults (timing, tree, arb) { |
There was a problem hiding this comment.
I think it probably would be more useful to silly-log a representation of the diff rather than just the top-level deps.
Maybe just iterate through the nodes in the diff and report what we're doing at each path (adding, removing, or replacing) and the version/integrity of what's getting put there?
|
|
||
| // Collect information about command action taken | ||
| const pkgCount = arb.diff.children.length | ||
| const contribCount = pkgCount |
There was a problem hiding this comment.
I think this should be a count of contributors, not a count of packages. Ie, unique maintainers or something?
| const contribCount = pkgCount | ||
| const added = `added ${pkgCount}` | ||
|
|
||
| const from = `from ${contribCount} contributor${contribCount ? 's' : ''}` |
There was a problem hiding this comment.
The pluralizing logic is a little off here. It'll add the s for any number other than zero?
| const added = `added ${pkgCount}` | ||
|
|
||
| const from = `from ${contribCount} contributor${contribCount ? 's' : ''}` | ||
| const audited = `audited ${0} package${'s'}` |
There was a problem hiding this comment.
Pluralizing logic even more weird here ;)
There was a problem hiding this comment.
Oh, I see, this is a placeholder until we have quick audits back in place.
I'd rather stub a function to return 0 or whatever, and have the correct logic here, or omit the audited bit entirely until it's correct, just so it's not a bug lying in wait.
isaacs
force-pushed
the
release/v7.0.0-beta
branch
from
cd6840b to
e57a991
Compare
|
Closed by #1245 |
What / Why
We previous added arborist and used it to install dependencies. This pull-request contains changes to how that command now outputs to the terminal for the user.
Arborist Functionality
There is a pull-request on npm/arborist#52 which adds some additional functionality, exposing the diff tree, that the cli is now going to leverage to get change actions from commands run with arborist.
References
npm installcommand #995