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

fix: reduce spammy logs #2206

Merged
merged 8 commits into from Dec 8, 2020
Merged

fix: reduce spammy logs #2206

merged 8 commits into from Dec 8, 2020

Conversation

alexander-akait
Copy link
Member

What kind of change does this PR introduce?

bugfix

Did you add tests for your changes?

Existing

If relevant, did you update the documentation?

No

Summary

No need extra logs, please disable them by default, so I change info to log

Does this PR introduce a breaking change?

No

Other information

we should output minimum information, stats is enough by default, developers don't love it

@anshumanv
Copy link
Member

need rerun CI after merge - #2208

@anshumanv
Copy link
Member

lint fixed in master, we can skip rebase and merge on CI green

anshumanv
anshumanv previously approved these changes Dec 7, 2020
@codecov
Copy link

codecov bot commented Dec 8, 2020

Codecov Report

Merging #2206 (b3b203e) into master (53ed926) will increase coverage by 0.34%.
The diff coverage is 100.00%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #2206      +/-   ##
==========================================
+ Coverage   68.19%   68.54%   +0.34%     
==========================================
  Files          70       70              
  Lines        2352     2378      +26     
  Branches      523      535      +12     
==========================================
+ Hits         1604     1630      +26     
  Misses        748      748              
Impacted Files Coverage Δ
packages/webpack-cli/lib/plugins/CLIPlugin.js 100.00% <100.00%> (ø)
packages/info/src/index.ts 100.00% <0.00%> (ø)
packages/webpack-cli/lib/utils/cli-flags.js 100.00% <0.00%> (ø)
packages/webpack-cli/lib/utils/arg-parser.js 89.89% <0.00%> (+0.10%) ⬆️
packages/webpack-cli/lib/groups/runHelp.js 99.00% <0.00%> (+0.33%) ⬆️

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 5872d35...b3b203e. Read the comment docs.

@alexander-akait alexander-akait merged commit 9b3cc28 into master Dec 8, 2020
@alexander-akait alexander-akait deleted the fix-reduce-spammy-logs branch December 8, 2020 13:41
@codymikol
Copy link

How can this be turned back on? This actually obfuscated some issues I was experiencing.

@eamodio
Copy link

eamodio commented Dec 31, 2020

Yeah, I would like to know the same -- as this change breaks problem matcher support in VS Code.

@alexander-akait ?

@alexander-akait
Copy link
Member Author

You should not rely on logger output, it is not safe, but you can enable them using https://webpack.js.org/configuration/other-options/#infrastructurelogging (set level to log)

@eamodio
Copy link

eamodio commented Dec 31, 2020

@alexander-akait do you have a suggestion on what to rely on then?

@alexander-akait
Copy link
Member Author

alexander-akait commented Dec 31, 2020

stats output with --json flag or --watch-options-stdin

@alexander-akait
Copy link
Member Author

alexander-akait commented Dec 31, 2020

What is use case? Why you need these lines?

@eamodio
Copy link

eamodio commented Dec 31, 2020

@alexander-akait
Copy link
Member Author

@eamodio
Copy link

eamodio commented Dec 31, 2020

Will that still provide start and stop makers in watch mode?

Also the fact that these are now gone by default adds a lot of extra friction for users since they need a custom config for things to "just work"

@alexander-akait
Copy link
Member Author

alexander-akait commented Dec 31, 2020

Will that still provide start and stop makers in watch mode?

Yes, we just decrease log level

Also the fact that these are now gone by default adds a lot of extra friction for users since they need a custom config for things to "just work"

It is already works fine, I don't know why you use it in this form, most of developers uses scripts and npm run/yarn

Some developers say - please remove extra logs
Some developers say - we need these extra logs

Sorry, I can't solve this problem for everyone

@codymikol
Copy link

Will that still provide start and stop makers in watch mode?

Yes, we just decrease log level

Also the fact that these are now gone by default adds a lot of extra friction for users since they need a custom config for things to "just work"

It is already works fine, I don't know why you use it in this form, most of developers uses scripts and npm run/yarn

Some developers say - please remove extra logs
Some developers say - we need these extra logs

Sorry, I can't solve this problem for everyone

I'm totally fine with logs off by default, I was just ignorant as to how to enable them. Your help is much appreciated!

@eamodio
Copy link

eamodio commented Dec 31, 2020

It is already works fine, I don't know why you use it in this form, most of developers uses scripts and npm run/yarn

I thought I was pretty clear as to what we are/were using it for — allowing VS Code to provide a richer build experience when using built tasks. These tasks can just be (and often are) just package.json scripts run by npm or yarn, or they can have some extra sugar on top to provide more functionality. For example, many devs configure a default build task, and then when they press F5 to start debugging, the debugger waits until the build task completes successfully before launching. Also errors output from typescript/eslint/etc can be scraped and aggregated into the VS Code Problems view:

image

Also to be clear, I'm only arguing for a start/end marker when in watch, and IMO (even outside the problem matcher issue) not having an end marker at least, doesn't provide a user a nice clear indicator that the watch task has actually completed (especially when you have multiple webpack configs exported)

@alexander-akait
Copy link
Member Author

If you need some extra logs - PR welcome, we don't remove previous logs, just decrease level

@eamodio
Copy link

eamodio commented Dec 31, 2020

Also, while I can certainly appreciate the you can't please everyone sentiment (and believe me I get it), I am raising this for a community/ecosystem. I am a core dev on VS Code and the maintainer of the webpack typescript/eslint problem matchers.

Yes, I understand that the logs still exist, but we would still like to have the ability to integrate with webpack without a user having to manually alter their configuration -- which was the case before these changes.

@alexander-akait
Copy link
Member Author

@eamodio Can you open a new issue and describe how you run webpack and what you need, I think we can find solution

@eamodio
Copy link

eamodio commented Jan 23, 2021

@alexander-akait Sorry for the delay, opened it here: #2374

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

Successfully merging this pull request may close these issues.

None yet

5 participants