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

Autoprefixer comments are stripped out when using sass-loader #638

Closed
wimhendrikx opened this issue Sep 13, 2019 · 5 comments
Closed

Autoprefixer comments are stripped out when using sass-loader #638

wimhendrikx opened this issue Sep 13, 2019 · 5 comments
Labels
Bug Bug Fix

Comments

@wimhendrikx
Copy link
Contributor

wimhendrikx commented Sep 13, 2019

With version 7.3.0 of sass-loader, compression is also done during the production build process. Apparently, this happens before autoprefixing kicks in (postcss-loader), which causes the necessary grid comments (/* autoprefixer grid: on */) to be removed, thus preventing Grid autoprefixing for IE during production.

I have found 2 possible fixes:

  1. Adding a '!' in the comment => /*! autoprefixer grid: on */
  2. Disable compression in the sass-loader enableSassLoader(function (options) { options.outputStyle = 'expanded'; })

Personally, the 2nd fix seems to be the best solution, but maybe this can be provided by default when compression is already done by postcss-loader?

@weaverryan weaverryan added the Bug Bug Fix label Sep 16, 2019
@weaverryan
Copy link
Member

Indeed - because CSS minification is handled with mini-css-extract-plugin, I can't think of any reason for us to want Sass to do this for us. So I also vote for option (2) - specifically, making the outputStyle nested by default (because I think this is the default setting in dev?). @wimhendrikx would you be able to create a PR for this? The relevant file should be https://github.com/symfony/webpack-encore/blob/master/lib/loaders/sass.js

wimhendrikx added a commit to wimhendrikx/webpack-encore that referenced this issue Sep 16, 2019
@Lyrkan
Copy link
Collaborator

Lyrkan commented Sep 16, 2019

Indeed - because CSS minification is handled with mini-css-extract-plugin, I can't think of any reason for us to want Sass to do this for us

People can opt-out from using the mini-css-extract-plugin, so that's a (small) breaking change. Not a big issue though since they would still be able to change options.outputStyle when calling Encore.enableSassLoader().

specifically, making the outputStyle nested by default (because I think this is the default setting in dev?)

That point may cause a bigger issue though, because Dart Sass does not seem to support nested: https://sass-lang.com/documentation/js-api#outputstyle

  • "expanded" (the default for Dart Sass) writes each selector and declaration on its own line.
  • "compressed" removes as many extra characters as possible, and writes the entire stylesheet on a single line.
  • "nested" (the default for Node Sass, not supported by Dart Sass) indents CSS rules to match the nesting of the Sass source.
  • compact (not supported by Dart Sass) puts each CSS rule on its own single line.

We'll have to be careful about setting options that are not directly handled by the loader since they are passed to the chosen implementation which may or may not support them.

wimhendrikx added a commit to wimhendrikx/webpack-encore that referenced this issue Sep 16, 2019
wimhendrikx added a commit to wimhendrikx/webpack-encore that referenced this issue Sep 16, 2019
wimhendrikx added a commit to wimhendrikx/webpack-encore that referenced this issue Sep 16, 2019
wimhendrikx added a commit to wimhendrikx/webpack-encore that referenced this issue Sep 16, 2019
@wimhendrikx
Copy link
Contributor Author

I've created a PR #639. Any remarks are welcome!

weaverryan added a commit that referenced this issue Oct 7, 2019
…rikx)

This PR was squashed before being merged into the master branch (closes #639).

Discussion
----------

bug #638 sass-loader should not do css minification

Fixes a BC break from the sass-loader minifying the CSS which is already done with mini-css-extract-plugin.

I chose to use outputStyle 'expanded' so it's also compatible with Dart Sass.

Commits
-------

f55fb90 Adapt test for new sass-loader config
ae5fea1 bug #638 sass-loader should not do css minification
@garak
Copy link

garak commented Nov 27, 2019

Any reason for this issue not being closed?

@Lyrkan
Copy link
Collaborator

Lyrkan commented Nov 27, 2019

@garak I think we simply forgot to close it since most of the time it's done automatically thanks to the fix/close/... keywords.

It should be OK with the latest version of Encore, if that's not the case feel free to reopen.

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

No branches or pull requests

4 participants