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

Idea: use FixStyleOnlyEntriesPlugin by default #33

Closed
mattheu opened this issue Aug 21, 2019 · 6 comments
Closed

Idea: use FixStyleOnlyEntriesPlugin by default #33

mattheu opened this issue Aug 21, 2019 · 6 comments

Comments

@mattheu
Copy link
Member

mattheu commented Aug 21, 2019

I have run into some issues where both a CSS file and the JS file generated for this were both loaded on production. The JS file is not necessary.

  • I was setting up a site using the HM asset loader plugin.
  • I had a SCSS file as an entry point.

To work around this, I used the webpack-fix-style-only-entries plugin during the production build to clean up these JS files that are only generated for a SCSS entry point. I did this on production

In my opinion, we should always use this alongside MiniCssExtractPlugin during production builds. It requires no configuration and I didn't find another way to avoid loading both files when using asset-loader without this.

Let me know your thoughts - happy to whip up a PR.

@mattheu mattheu changed the title Idea: use FixStyleOnlyEntriesPlugin Idea: use FixStyleOnlyEntriesPlugin by default Aug 21, 2019
@mattheu
Copy link
Member Author

mattheu commented Aug 21, 2019

Sorry - didn't read the docs and I see this is bundled.

However, I'm not going to close the ticket just yet - as the issue did trip me up. Is there any reason not to just include this by default?

@kadamwhite
Copy link
Contributor

My argument against adding it by default is that I, as a JS dev, expect Webpack to do this silly thing and generate an unused JS file; if it doesn't, I feel it might trip me up. But that raises the question about who the target audience of this tool should be, because it is a true statement that most people coming to Webpack are confused by the way it handles no-JS situations.

Maybe ping @rmccue @tfrommen et al for a consensus opinion, I could go either way on this.

@mattheu
Copy link
Member Author

mattheu commented Aug 21, 2019

Perhaps this is an asset loader issue instead? My real problem was that I couldn't use it to load just a single CSS file - which is something that I think should be easier than it is. Maybe that needs to be smarter about loading only the CSS file if there are both JS and CSS files with the same name?

@kadamwhite
Copy link
Contributor

A CSS and a JS file with the same name is a valid outcome, if you're extracting styles from a bundle which contains both (the "normal case" from a JS community standpoint, these days).

I think I'd be provisionally in agreement to add this; it's definitely something we'd want to change on this end, not the asset-loader end, I believe.

@tfrommen
Copy link
Contributor

tfrommen commented Aug 21, 2019

I'm fine with either. However, I too did use SCSS-only entries, where I don't need the JS file for production, and also don't care much about it being there.

But if the file ends up being loaded, we should change that, in my opinion. And this is something that the asset loader should (not) do. I mean, if I (auto)enqueue/register a style, this should only load a single file: the CSS file, if it exists, or the JS file (assuming this is development, or testing or whatever). Why would we want the JS file to load? (Again, I did not check if this is actually the case.)

@kadamwhite
Copy link
Contributor

OK, then let's include plugins.fixStyleOnlyEntries() by default in our production configuration. We should add it to that preset's default plugins array, and update documentation if we list out what plugins are included anywhere.

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

No branches or pull requests

3 participants