-
Notifications
You must be signed in to change notification settings - Fork 12k
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
fix(@ngtools/webpack): log ngcc info messages #14320
Conversation
Below is how the logging looks like with this PR. First build:
Next builds:
|
@@ -104,6 +104,7 @@ export class NgccProcessor { | |||
} | |||
|
|||
class NgccLogger implements Logger { | |||
private alreadyProcessMsg = 'The target entry-point has already been processed'; |
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.
If ngcc
change its logging messages our filter will break.
@petebacondarwin @gkalpak do you have a suggestion on a better way to log only when something is actually happening?
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.
How about we just agree to change the info
messages that you do not wish to see to debug
messages? Then you don't need to filter at all?
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 I think it would also be fair for CLI to output a message before ngcc is triggered to say something like "Compiling Angular packages with ngcc".
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.
It's hard to go down that route because the CLI doesn't know if ngcc will try to compile a package or skip it. If we had such message it would come up on every build, several times.
info(..._args: string[]) { | ||
const msg = _args.join(' '); | ||
if (!msg.includes(this.alreadyProcessMsg)) { | ||
process.stderr.write(`\n${msg}\n`); |
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.
Why not use the standard output?
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.
Because it's a progress-like message. The webpack ProgressPlugin uses stderr for progress reporting, and so does Architect CLI.
I remember a discussion with @clydin about this too where I was pretty convinced progress-like messages should go there.
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.
Thanks for the clarification
info(..._args: string[]) { | ||
const msg = _args.join(' '); | ||
if (!msg.includes(this.alreadyProcessMsg)) { | ||
process.stderr.write(`\n${msg}\n`); |
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.
Maybe we should only print the compiling
messages.
We should skip all others, such as skipping target already compiled and target entrypoint already compiled.
Something along:
If (msg.startsWith(‘Compiling’)) {
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.
@petebacondarwin should ngcc also log Skipping
messages as debug?
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.
If you like 🤗
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'm good with that approach then. @alan-agius4 WDYT?
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.
Please send a PR :-)
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.
Sounds good to me
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.
PR up at angular/angular#30232, PTAL @petebacondarwin. I'll then change this PR to log all info messages instead.
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.
Approved
This issue has been automatically locked due to inactivity. Read more about our automatic conversation locking policy. This action has been performed automatically by a bot. |
Fix #14194
This PR by itself will address #14194, but needs angular/angular#30232 in order to not be spammy.