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: stats.hasWarnings() should respect ignoreWarnings #17690

Merged
merged 3 commits into from Apr 12, 2024

Conversation

nanianlisao
Copy link
Contributor

@nanianlisao nanianlisao commented Sep 18, 2023

Summary

Actually, I am not sure if this is a bug or if it was done intentionally. But I think hasWarnings in stats should respect ignoreWarnings

Especially in the compiler.hooks.done hooks, stats obtained is directly processed via hasWarnings to handle warnings.

🤖 Generated by Copilot at 345939a

Refactor Stats.hasWarnings to use Compilation.getWarnings. This is part of a larger effort to optimize and enhance the stats output.

Details

stats.hasWarnings() should respect ignoreWarnings

🤖 Generated by Copilot at 345939a

  • Refactor warnings handling logic in Compilation class (0,10,10,1,F0L3R

@linux-foundation-easycla
Copy link

linux-foundation-easycla bot commented Sep 18, 2023

CLA Signed

The committers listed above are authorized under a signed CLA.

@webpack-bot
Copy link
Contributor

For maintainers only:

  • This needs to be documented (issue in webpack/webpack.js.org will be filed when merged)
  • This needs to be backported to webpack 4 (issue will be created when merged)

@nanianlisao
Copy link
Contributor Author

@alexander-akait
Hi,
I've noticed that my pull request hasn't been reviewed yet. If you could find some time to review it, I'd really appreciate it.

Thanks a lot!

Copy link
Member

@alexander-akait alexander-akait left a comment

Choose a reason for hiding this comment

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

I am fine with it, but I am afraid it can be a breaking change, because some tools can rely on this behaviour... I think we need a small dicussion here @webpack/core-support @webpack/cli-team

@snitin315
Copy link
Member

@alexander-akait Yeah, this could be a potential breaking change. I believe we should consider this change for webpack v6

@vankop
Copy link
Member

vankop commented Apr 11, 2024

I think this is valid, ignoreWarnings should be respected.

@alexander-akait
Copy link
Member

alexander-akait commented Apr 11, 2024

@vankop I think we need a test cases to prevent a regression in future

@alexander-akait
Copy link
Member

Feel free to send it and we can merge it

@webpack-bot
Copy link
Contributor

@nanianlisao Thanks for your update.

I labeled the Pull Request so reviewers will review it again.

@vankop Please review the new changes.

@nanianlisao
Copy link
Contributor Author

Great, could you please take a look again. @alexander-akait

@alexander-akait alexander-akait merged commit 0f7dedd into webpack:main Apr 12, 2024
56 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
Status: Need's Release
Development

Successfully merging this pull request may close these issues.

None yet

5 participants