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 orderWarnings flag #366

Conversation

hedgepigdaniel
Copy link

The flag defaults to true, which retains the existing behaviour of
warnings when there is conflicting import order between multiple CSS
files. When set to false, these warnings are not generated.

This PR contains a:

  • bugfix
  • new feature
  • code refactor
  • test update
  • typo fix
  • metadata update

Motivation / Use-Case

As has been pointed out in #250:

  • Sorting all imports (not just in files but in tree traversal import order is difficult to do in a large project, and impossible if there are async chunks with CSS.
  • Consistent use of scoping or naming conventions in CSS makes the order of CSS import irrelevant, and this is a much more manageable strategy for a large project as @jsg2021 said in New version 0.4.2 capture lot of warnings #250 (comment)
  • Using stats.warningFilter as recommended by @sokra in New version 0.4.2 capture lot of warnings #250 (comment) or webpack-filter-warnings-plugin does not work consistently. It's much better to avoid generating the warnings at their source (optionally) rather than to partially filter them later with regexes. For example, I use webpack-error-on-warnings-plugin to enforce that there are no warnings in a build. This is incompatible with filtering warnings, because it the error must happen in the before-emit hook so that it prevents emission of assets when there are warnings. The warnings filtering happens in the done hook or in the toJson method, which is too late.

The flag defaults to true, which retains the existing behaviour of
warnings when there is conflicting import order between multiple CSS
files. When set to false, these warnings are not generated.
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.

We don't need option for this, warning in this case is good, other developers should know what order is invalid

@alexander-akait
Copy link
Member

stats.warningFilter should work please provide minimum reproducible test repo where it is doesn't work as expected

@hedgepigdaniel
Copy link
Author

For example, I use webpack-error-on-warnings-plugin to enforce that there are no warnings in a build. This is incompatible with filtering warnings, because it the error must happen in the before-emit hook so that it prevents emission of assets when there are warnings. The warnings filtering happens in the done hook or in the toJson method, which is too late.

I can make a minimum repo of this problem but I don't think it will help. stats.filterWarnings only filters the warnings that are printed - it doesn't filter the warnings in the compilation object, which is what my plugin uses. stats.warningFilter works as intended, it just doesn't solve my problem. It should be clear why if you look at the code for webpack-error-on-warnings-plugin. Why put the warnings into the compilation in the first place if you are going to filter them out later?

I think its good for a project to disallow warnings (e.g. in production, the master branch, etc). That way warnings are useful in development. If there's heaps of warnings all the time, nobody reads them or notices new ones and they are not useful. Sometimes that means making a compromise - some warnings are too hard to fix and the fix doesn't provide enough benefit. It's better to hide them than to have heaps of warnings that nobody reads.

All this change does is provide an (optional, non-default) way of turning the warning off in a simpler and cleaner way than filtering strings with regexes.

@alexander-akait
Copy link
Member

No it is bad DX. You may miss a problem when you actually have to solve it.

@hedgepigdaniel
Copy link
Author

Then why are you recommending to use stats.warningFilter? That would still make people not see the warning right?

#250 (comment):

For example, in my projects each component's styles are self contained so order is only important within each component. (each component has a very unique class name root and all selectors are scoped to them)

In projects that do that, it doesn't matter what order CSS is imported, and the warning is wasting their time.

Having the warning on by default is fine, I can see that it is a potential problem people should be aware of. But not letting them turn it off in a clean way seems counter productive.

@pnarielwala-tc
Copy link

#362 (comment)

Should this PR be updated with ignoreOption: true? Would really love this, thanks!

@alexander-akait
Copy link
Member

PR welcome

@mjhenkes mjhenkes mentioned this pull request Jul 10, 2019
6 tasks
@hedgepigdaniel
Copy link
Author

Closing in favour of #422

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.

None yet

3 participants