Replace post install module list with summary report #15914
Conversation
Changes Unknown when pulling c67f7a7 on iarna/post-install-report into ** on release-next**. |
I would prefer something like
|
Disk utilization is not a bad idea, but out of scope for this. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM. This is gonna be great
Ok. Now I can't decide which format I like better. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is much better then it used to be.
lib/install.js
Outdated
var lastAction = actions.pop() | ||
report += actions.join(', ') + ' and ' + lastAction | ||
} | ||
report += ' package' + (plural ? 's' : '') + '.' |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I would suggest removing the period.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
But... but... I like my complete sentences. =D
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't like complete sentences in logs. They look weird and takes 6 extra characters.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
six? I don't understand. There'd only be one…
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I meant in the code, not the log.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
¯_(ツ)_/¯
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good answer. Also, use more github emoji. =D
lib/install.js
Outdated
if (updated) actions.push('updated ' + updated) | ||
if (moved) actions.push('moved ' + moved) | ||
if (actions.length === 0) { | ||
return cb() |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I would think that if nothing happened it should print that.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Like
$ npm install
Nothing changed
$
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Slightly more complicated as we may be bubbling up an error.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
(But ideally I like that.)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I may land it as is, and then we can patch that in next. Depends on how long getting through the tests takes. =D
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ok, sounds like a good idea.
87bbc1d
to
47f77ad
Compare
6005571
to
1b36ac6
Compare
1b36ac6
to
27f9cb7
Compare
6f1b69a
to
2ae141b
Compare
Just thought of this, would a PR to include sizes in this report be accepted? (Maybe after this is merged) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Will it still be possible to get some more verbose output available when enabled via an environment variable (e.g. |
@legodude17 asked:
If it doesn't have a substantial impact in install speed, yes. @jacobq asked:
How about via |
@iarna That should be fine, thanks; I didn't realize those options were there. |
@iarna Good to know! I will look into it. |
Hmm, I am poking around the issue tracker - I am looking for that option for |
This is a PR for a feature in a future release. To my knowledge there is not good way to prevent |
ad02037
to
d39dc0e
Compare
npm find-dupes is now an alias for `npm dedupe --dry-run --parseable`
d39dc0e
to
89f9140
Compare
This is merged into |
Something we've been talking about for a while. With a project of any size the post-install module list is unwieldy. This give us something like:
Versus what we got previously:
Test failures are due to tests relying on specific
npm install
output and will require patching.