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

feat: add status in quiet log level #2235

Merged
merged 11 commits into from
Sep 12, 2019

Conversation

rishabh3112
Copy link
Member

@rishabh3112 rishabh3112 commented Aug 31, 2019

  • This is a bugfix
  • This is a feature
  • This is a code refactor
  • This is a test update
  • This is a docs update
  • This is a metadata update

For Bugs and Features; did you add new tests?

Motivation / Use-Case

fixes #2233

Breaking Changes

May break some tests

Additional Info

@codecov
Copy link

codecov bot commented Aug 31, 2019

Codecov Report

Merging #2235 into master will increase coverage by 0.01%.
The diff coverage is 100%.

Impacted file tree graph

@@            Coverage Diff            @@
##           master   #2235      +/-   ##
=========================================
+ Coverage   93.88%   93.9%   +0.01%     
=========================================
  Files          34      34              
  Lines        1276    1279       +3     
  Branches      366     367       +1     
=========================================
+ Hits         1198    1201       +3     
  Misses         71      71              
  Partials        7       7
Impacted Files Coverage Δ
lib/utils/status.js 91.3% <100%> (+1.3%) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 3748c3f...f802b18. Read the comment docs.

@knagaitsev
Copy link
Collaborator

Looks like it should work, but needs tests.

@rishabh3112
Copy link
Member Author

Added tests for quiet flag.

@alexander-akait
Copy link
Member

/cc @hiroppy @Loonride

@alexander-akait
Copy link
Member

/cc @hiroppy @Loonride need review

Copy link
Member

@hiroppy hiroppy left a comment

Choose a reason for hiding this comment

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

I'll fix this problem in the next version without webpack-log.

@knagaitsev
Copy link
Collaborator

@evilebottnawi @hiroppy Not vital but do you want this in the test? #2235 (comment)

I can add this

@alexander-akait
Copy link
Member

@Loonride let's do it, maybe we can do this after merge?

@knagaitsev
Copy link
Collaborator

@Loonride let's do it, maybe we can do this after merge?

Yes

@rishabh3112
Copy link
Member Author

rishabh3112 commented Sep 12, 2019

Sorry guys, I am having my university exams this month.
Will push up tests as soon as I got time 😓

@knagaitsev
Copy link
Collaborator

Sorry guys, I am having my exams this month.
Will pish up tests as soon as I got time

No worries, we want to do release soon so I can do the small test change.

const colors = require('./colors');
const runOpen = require('./runOpen');

// TODO: don't emit logs when webpack-dev-server is used via Node.js API
function status(uri, options, log, useColor) {
if (options.quiet === true) {

Choose a reason for hiding this comment

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

does this mean that there is now no way to silence these extra logs when using in node api?

Copy link
Member

Choose a reason for hiding this comment

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

yes, this log should be outputted always, in future we will use webpack logger and you can filter this output

Choose a reason for hiding this comment

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

that would be great. this seems like a regression IMO. we now have an extra set of console output. if possible when the webpack logger option is available would be great to link that to this issue.

Copy link
Member

Choose a reason for hiding this comment

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

You can use custom logger as workaround

Choose a reason for hiding this comment

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

link to documentation? not sure what you mean.

Copy link

@endiliey endiliey Oct 15, 2019

Choose a reason for hiding this comment

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

@evilebottnawi

I am facing this as well. I use webpackbar and this extra console log noise actually make it weird because webpackbar tries to clear screen when showing the fancy bar

After this PR
sad

Before
good

Can we add an option to hide this output ? I'm sure many packages that use webpackbar is affected by this unknowingly :(

cc my team @yangshun

Update: We have adjusted to wds so whether to hide the output/not no longer matter to us 😂

Choose a reason for hiding this comment

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

pretty pretty please? this output decreases dev experience IMO.

Copy link
Member

Choose a reason for hiding this comment

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

Feel free to send a PR with new option

Copy link
Contributor

Choose a reason for hiding this comment

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

This reverts #1486 and I believe it to now be a breaking change.

this log should be outputted always

Sorry, strongly disagree. What's the point of the option if it doesn't do what it says? I understand the custom logger is just a documentation issue right now, but it still points to quiet (and noInfo) being pointless/improperly named

Copy link
Contributor

Choose a reason for hiding this comment

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

You actually can't fix this with a custom logger, because this setup noise is printed by utils/status.js directly, which is where this log override is happening. There's no intermediary step

@kelly-tock
Copy link

it seems like not a lot of people want this change. how can we get it back to be what it was?

@aprilandjan
Copy link

It is really frustrating finding out that currently these no way to silence these extra logs when using in node api 😭

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Option quiet does not show initial startup information
9 participants