Skip to content

CSS variables are wrongly optimized in production #436

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

Closed
javiereguiluz opened this issue Nov 8, 2018 · 5 comments
Closed

CSS variables are wrongly optimized in production #436

javiereguiluz opened this issue Nov 8, 2018 · 5 comments

Comments

@javiereguiluz
Copy link
Member

javiereguiluz commented Nov 8, 2018

In a CSS file I define several CSS variables, including these:

:root {
    --border-width: 1px;
    --border-style: solid;
    --border-color: #e3e7ee;
}

When using yarn encore dev everything is great ... but with yarn encore production those 3 variables are destroyed and instead, the following wrong CSS is generated:

:root {
    border:1px solid #e3e7ee
}

How can I tell Webpack Encore to not optimize my CSS variables? Thanks!

@Lyrkan
Copy link
Collaborator

Lyrkan commented Nov 8, 2018

Hi @javiereguiluz,

It looks like a bug in the version of cssnano used by the css-loader (see webpack-contrib/css-loader#708).

As a workaround you could try disabling minification in the css-loader and using a newer version of cssnano through postcss:

$ yarn add --dev postcss-loader cssnano
// webpack.config.js
const Encore = require('@symfony/webpack-encore');

Encore
  // (...)
  .configureCssLoader(options => { options.minimize = false; })
  .enablePostCssLoader()
;

module.exports = Encore.getWebpackConfig();
// postcss.config.js
module.exports = {
  plugins: {
    'cssnano': {},
  }
}

@jfcherng
Copy link

jfcherng commented Nov 9, 2018

css-loader has a stable release now! Maybe we should update it from ^0.28.10 to ^1.0.0.

@javiereguiluz
Copy link
Member Author

@Lyrkan thanks a lot for your help. I tried doing that but I found an issue. This is my entire webpack.config.js file:

var Encore = require('@symfony/webpack-encore');

Encore
    .setOutputPath('./src/Resources/public/')
    .setPublicPath('/bundles/easyadmin')
    .setManifestKeyPrefix('bundles/easyadmin')

    .cleanupOutputBeforeBuild()
    .enableSassLoader()
    .enableSourceMaps(false)
    .enableVersioning(false)
    .autoProvidejQuery()

    // needed to avoid this bug: https://github.com/symfony/webpack-encore/issues/436
    .configureCssLoader(options => { options.minimize = false; })
    .enablePostCssLoader()

    .addEntry('app', './assets/js/app.js')
    .addEntry('app-rtl', './assets/js/app-rtl.js')
    .addEntry('bootstrap-all', './assets/js/bootstrap-all.js')
;

module.exports = Encore.getWebpackConfig();

And I run yarn add --dev postcss-loader cssnano and created postcss.config.js too, but I see this error:

$ yarn encore production

  Running webpack ...

  Error: Encore.configureCssLoader is not a recognized property or method.

  - index.js:969 Object.get
    [easy-admin-bundle]/[@symfony]/webpack-encore/index.js:969:27

  - webpack.config.js:15 Object.<anonymous>
    /Users/javier/code/easy-admin-bundle/webpack.config.js:15:5

  - module.js:624 Module._compile
    module.js:624:30

  - module.js:635 Object.Module._extensions..js
    module.js:635:10

  - module.js:545 Module.load
    module.js:545:32

  - module.js:508 tryModuleLoad
    module.js:508:12

  - module.js:500 Function.Module._load
    module.js:500:3

  - module.js:568 Module.require
    module.js:568:17

  - module.js:11 require
    internal/module.js:11:18

  - convert-argv.js:97 requireConfig
    [easy-admin-bundle]/[webpack]/bin/convert-argv.js:97:18


error Command failed with exit code 1.
info Visit https://yarnpkg.com/en/docs/cli/run for documentation about this command.

@jfcherng
Copy link

jfcherng commented Nov 9, 2018

configureCssLoader() is introduced in encore 0.21.0.

@javiereguiluz
Copy link
Member Author

@jfcherng you are absolutely right! I upgraded to 0.21 and it worked. @Lyrkan you are a genius and you perfectly solved my issue. Thanks a lot!

Closing as "fixed".

javiereguiluz added a commit to EasyCorp/EasyAdminBundle that referenced this issue Aug 10, 2019
…2875) (liarco)

This PR was merged into the 2.0.x-dev branch.

Discussion
----------

Fixing bootstrap import (js) and old webpack config (fixes #2875)

### Disclaimer
This PR **should not be merged without accurate review** since webpack configuration changes might break JS/CSS code.

### What changes
This PR fixes #2875. After further investigation I discovered that changing babel configuration was not the only option to fix the issue.

Here is what I did:

- as @Tersoal correctly pointed out, the Edge error seems to be related to the way Bootstrap JS is imported inside `/assets/js/app.js`. Changing the import statement fixes this issue and works on **Edge** _(42.17134.1.0)_, **Chrome** _(75.0.3770.100)_, **Firefox** _(67.0.2)_, **Safari** _(12.1.1)_ and **Opera** _(62.0.3331.101)_. [Official bootstrap documentation](https://getbootstrap.com/docs/4.3/getting-started/webpack/) also suggests using "bare imports" when using webpack
- looking at the webpack config I found a reference to symfony/webpack-encore#436. This issue is solved in recent versions of `webpack-encore` and `nanocss`, so I simplified the configuration dropping custom `postcss` pass. I then inspected resulting CSS and custom properties are now optimized correctly

### Conclusions
These changes should fix the issue, but:

- a question for @Tersoal: you mentioned this fix in other issues (#2727, #2856) do you think that we should put this change under deep testing to avoid breaking the existing code? If so can you explain why so we can focus our tests and verify this all together?
- @javiereguiluz: do you think there is any reason not to update webpack-encore and keep the "nanocss hack"? If so I can revert those changes
- @maximumsnowy: can you please test this PR in your environment to confirm it's working?

Thank you for your time.

Commits
-------

c3fb60a Fixing bootstrap import (js) and old webpack config (fixes #2875)
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