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

chore(package): use cssnano 4 and PostCSS 6 #739

Closed
wants to merge 1 commit into from
Closed

chore(package): use cssnano 4 and PostCSS 6 #739

wants to merge 1 commit into from

Conversation

ai
Copy link
Contributor

@ai ai commented Jul 4, 2018

What kind of change does this PR introduce?

Dependencies update

Did you add tests for your changes?

It is better to keep old tests for dependencies update

If relevant, did you update the README?

Not relevant

Summary

CSSnano 3 uses very old Autoprefixer with old Browerslist. At result, it block any new Browserslist queries (like not dead) from being wildly usage (since old Browserslist will throw “unknown query” error). This is why updating CSSNano is critical for the ecosystem, since Browerslist is used in Autoprefixer and Babel 7.

Does this PR introduce a breaking change?

All options should work. But in another hand, CSSNano 4 has a lot of changes inside.

Other information

I disabled cssDeclarationSorter, because I found a bug there. Also, this feature was not part of the previous css-loader. I removed all other disabled plugins (like zindex) because they are already disabled in default preset.

/cc @michael-ciniawsky @evilebottnawi

@@ -192,11 +192,11 @@ module.exports = function processCss(inputSource, inputMap, options, callback) {
if(minimize) {
var cssnano = require("cssnano");
var minimizeOptions = assign({}, query.minimize);
["zindex", "normalizeUrl", "discardUnused", "mergeIdents", "reduceIdents", "autoprefixer"].forEach(function(name) {
["cssDeclarationSorter", "normalizeUrl"].forEach(function(name) {
Copy link
Member

Choose a reason for hiding this comment

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

cssDeclarationSorter should be safe

Copy link
Contributor Author

Choose a reason for hiding this comment

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

We can enable it back when cssnano/cssnano#535 was fixed. Anyway, it is safer to start from CSSNano 3 optimization and after some tests, enable more optimizations.

@alexander-akait
Copy link
Member

alexander-akait commented Jul 4, 2018

@ai css-loader is a complete failure in webpack ecosystem, need find time to remove css modules to separate loader, or find way to use their using postcss-loader. Unfortunately, we are unlikely to be able to update here something before do this, because each update break something. Also we have a lot of problems with perf and bugs using old css modules plugins (need update all deps).

@ai
Copy link
Contributor Author

ai commented Jul 4, 2018

@evilebottnawi I just need to update Browserslist for webpack users. What can we do to update it?

@ai
Copy link
Contributor Author

ai commented Jul 4, 2018

Should we also remove Node.js 4 support in css-loader? CSSNano 4 doesn’t support it.

It will require the major release. But anyway everyone dropped Node.js 4 support right now.

@alexander-akait
Copy link
Member

@ai It is a lot of work 😞

  1. Remove css modules from css-loader (it should be separate loader or docs how to integrate this with postcss-loader).
  2. Remove minimize (i.e. cssnano) in favor plugin (https://github.com/intervolga/optimize-cssnano-plugin, need update cssnano also)
  3. Set importLoaders: infinity by default.
  4. Update all deps.
  5. Refactor code and update webpack-default.
  6. Major release.

But we can do this with 1-2 majors release. We already try to upgrade any related to postcss and it is break loader due postcss modules plugins (they are very buggy).

@ai ai changed the title Use cssnano 4 and PostCSS 6 chore(package): use cssnano 4 and PostCSS 6 Jul 4, 2018
@alexander-akait
Copy link
Member

It will require the major release. But anyway everyone dropped Node.js 4 support right now.

Unfortunately no

@ai
Copy link
Contributor Author

ai commented Jul 4, 2018

But we can do this with 1-2 majors release.

Yeap, let’s update cssnano and PostCSS in next major release. And then let’s do all your tasks in next major releases.

We already try to upgrade any related to postcss and it is break loader due postcss modules plugins (they are very buggy).

How I can repeat these issues. Browserslist update is very critical for me. I will fix postcss modules

@ai
Copy link
Contributor Author

ai commented Jul 4, 2018

Unfortunately no

Or we can return Node.js 4 support to cssnano 4. What do you think about it?

I sent PR for dropping Node.js 4 just because I already did it (before your comment). I am still OK with not dropping Node.js 4 (but, to be honest, I think it is better to do it, anyway everyone dropped it) #740

@alexander-akait
Copy link
Member

@ai we should drop nodejs@4,

  1. We should update to webpack-default (it is standard for webpack contrib) (here already nodejs@4 was dropped)
  2. Remove root option
  3. Remove minimize option
  4. Update all deps
  5. Release major

We can't release merge this PRs until we do it, started PR #639

@ai
Copy link
Contributor Author

ai commented Jul 4, 2018

@evilebottnawi great tasks. Why do we need to do them only in the single major release?

What do you think about fixing few things fast and release them?

@alexander-akait
Copy link
Member

css-loader as i said above is big problem, need full refactor and release, right now he was blocked for everything until the above is done (#739 (comment))

@alexander-akait
Copy link
Member

alexander-akait commented Jul 4, 2018

@ai because one thing broke other thing, we already try do it

@ai
Copy link
Contributor Author

ai commented Jul 4, 2018

@ai because one thing broke other thing, we already try do it

Can you give me a list of thing, which will be broken by PostCSS and cssnano update?

I really need Browserslist update. Babel 7 will promote it, but node x.x.x queries will not work, because old Browserslist in css-loader will throw a error.

I understand that you have a big plan and don’t want to release small steps instead of fixing them all. But could you please consider again (maybe after few hours)? Node.js 4 support dropping of will not break anything. All css modules incompatibility with PostCSS 6 can be fixed soon. What other problems we could have?

@alexander-akait
Copy link
Member

alexander-akait commented Jul 4, 2018

@ai it is not my solution what css-loader is blocked, it is solution webpack developers. All related to css modules will be changed and broken after update (we already try this).

Fast way is drop css modules (add docs how they can be using through postcss-loader or other loader/tool), update deps and do major release.

@ai
Copy link
Contributor Author

ai commented Jul 4, 2018

@evilebottnawi OK, what you will do on my place if you need to update Browserslist ASAP?

Maybe we should continue talking in webpack Slack if other webpack developers are related?

@alexander-akait
Copy link
Member

@ai you can talk about this here https://webpack.slack.com/messages/C1LUX2DS9/, but looks more unavailable or css outside their competence, i try to solve this problem about 6 months ago.

I do not think that we can solve this ASAP, sorry

/cc @sokra

@ai
Copy link
Contributor Author

ai commented Jul 4, 2018

i try to solve this problem about 6 months ago.

Did I understand correctly, that problem is with incompatibility between CSS Module plugins in PostCSS 6?

How can I get the list of errors?

@alexander-akait
Copy link
Member

@ai between postcss and other postcss css modules plugins, try to update deps and run tests

@ai
Copy link
Contributor Author

ai commented Jul 4, 2018

@evilebottnawi I did run tests and everything was fine (maybe I forgot to update something?)

@alexander-akait
Copy link
Member

@ai why just remove minimize option and release new major?

@ai
Copy link
Contributor Author

ai commented Jul 4, 2018

why just remove minimize option and release new major?

I like this plan =^_^=

@alexander-akait
Copy link
Member

@ai send PR with remove this option and we can release major release without pain

@ai ai mentioned this pull request Jul 4, 2018
@ai
Copy link
Contributor Author

ai commented Jul 4, 2018

Done #741

@ai
Copy link
Contributor Author

ai commented Jul 6, 2018

Close for #742

@ai ai closed this Jul 6, 2018
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

2 participants