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

Roadmap for version 4. #88

Closed
ben-eb opened this issue Nov 11, 2015 · 28 comments
Closed

Roadmap for version 4. #88

ben-eb opened this issue Nov 11, 2015 · 28 comments

Comments

@ben-eb
Copy link
Collaborator

ben-eb commented Nov 11, 2015

Just a note; v4 is not in the works just now, due to time constraints. However I would like to get some community feedback on this set of major changes, which I think are really important for this project's health going forward.

/cc @ai @MoOx @TrySound @sindresorhus @yisibl @benfrain @davidtheclark @jonathantneal


Plugin architecture

Performance, though much improved by the 1.5x speed up in PostCSS remains one of our major sticking points. Due to the way this module is built, each sub module wastes time by parsing and stringifying values each time they are run. We can cut this down to hopefully a single parse/stringify step for each node rather than for each plugin, but to do this we need to change our architecture.

I propose that a new architecture should export only one PostCSS plugin, as our current setup means that for each value transform (https://github.com/TrySound/postcss-value-parser) we perform, we have to:

  • Parse a node's value into memory.
  • Transform it.
  • Stringify it ready for the next transform...

This is due to each plugin being totally focused in its own PostCSS world. Each transform can work independently, which is a nice feature but not totally relevant given that most people would rather include a call to cssnano in their PostCSS instance, rather than bundle each plugin.

In addition, each plugin utilises PostCSS' each or eachInside methods at least once; if we can try to group these calls together that will also improve performance. Previous refactors involved reducing the usage of these methods per module which improved performance, so doing this on a global level would be beneficial.

I still believe in the plugin pattern being the best way going forward however, so v4 should have a different architecture that exposes hooks that visitors can exploit. For example there could be a hook for each time that a color value [rgba?|hsla?|hex|keyword] is detected; then in cssnano we have a system more like PostCSS itself:

  • Parse node values into memory.
  • Transform, transform, transform...
  • Stringify.

We then have a plugin list like the following:

  • postcss-colormin => cssnano-minify-colors
  • postcss-zindex => cssnano-reduce-zindexes
  • etc...

All current modules that I maintain and bundle with cssnano will be deprecated in favour of this new architecture. For modules that I don't maintain, such as autoprefixer, they will continue to be bundled with cssnano but will run first. That way we keep current functionality the same.

As I already found out when refactoring postcss-discard-duplicates, stringifying PostCSS trees are really expensive and you should always try and cache/reduce these calls whenever possible. We should operate on arrays instead.

Preset configurations

One of the things that I really like about Babel 6 is its customisability; you can specify a preset for ES2015 and it will download only the relevant modules. I think we should follow this pattern and at the same time look to making v4 run safe by default. Transforms such as postcss-zindex are not intuitive default options - we assume too much from a user by turning it on by default. So in v4, we will expose a core module that is safe by default, and then presets for cssnano's more aggressive behaviours.

Perhaps something like the following:

  • cssnano (main module, loads only safe transforms)
  • cssnano-preset-advanced (loads transforms like postcss-zindex, postcss-discard-unused)
  • cssnano-preset-experimental (loads experimental optimisation paths that may not be safe yet, such as CSS custom variable name reduction)
  • cssnano-preset-all (loads the whole lot)

sourcemap option removal

options.sourcemap should be removed as it is just sugar for the PostCSS map option. By dropping this option we can remove this method, making our codebase that much leaner:

https://github.com/ben-eb/cssnano/blob/a66892373552966d06210421d3f257279b704dab/index.js#L128-L132

Other changes

See the 4.0 milestone for related issues: https://github.com/ben-eb/cssnano/milestones/4.0


I'll add more details as and when I think of them. Any feedback would be greatly appreciated. 😄

@ben-eb ben-eb added this to the 4.0 milestone Nov 11, 2015
@MoOx
Copy link
Contributor

MoOx commented Nov 11, 2015

Grouping each* can be a thing, but at the level of cssnano only. Like you said, it's the power of postcss after all. And some plugins even make 2 passes (like postcss-custom-properties).

@sindresorhus
Copy link
Contributor

Babel 6 also have some good plugin optimizations by combining many plugins into one step. Might be worth looking into for inspiration.

@ben-eb
Copy link
Collaborator Author

ben-eb commented Nov 11, 2015

@MoOx Yep, some plugins will need two or more passes still. I am thinking that transforms like postcss-colormin, postcss-convert-values, postcss-svgo, & postcss-normalize-url (and others) that map over values and transform e.g. black to #000 and 1.0px to 1px are going to benefit the most from this.

@sindresorhus Definitely. I think Babel's monorepo is a great pattern for testing/release cycles too. 👍

@TrySound
Copy link
Collaborator

@ben-eb I tried that monorepo today. It was hard. And I haven't success on testing it. ))

@benfrain
Copy link

I have nothing smart to offer in terms of architecture. I would just like to add that I think a safe default would be the better default.
We were stung in production by z-index rebase and autoprefixer running (we already had autoprefixer in our chain and didn't realise nano was running it again with its own default config).

@TrySound
Copy link
Collaborator

@benfrain But autoprefixer in cssnano is removed from chain by postcss-filter-plugins. @ben-eb Doesn't it work?

@benfrain
Copy link

Maybe that was introduced after the version we are running. I can check version number if it helps?

On 12 Nov 2015, at 08:08, Bogdan Chadkin notifications@github.com wrote:

@benfrain But autoprefixer in cssnano is removed from chain by postcss-filter-plugins. @ben-eb Doesn't it work?


Reply to this email directly or view it on GitHub.

@ben-eb
Copy link
Collaborator Author

ben-eb commented Nov 12, 2015

We introduced that functionality since v2.2.0. 5745135

@benfrain
Copy link

Just to confirm: yes, we are currently running gulp-cssnano 1.1.0 ;)

@davidtheclark
Copy link

I'll be really curious to see how an event-hooks architecture affects performance. If it's a significant gain, might be worth looking into for stylelint.

@addyosmani
Copy link

I was a fan of Babel 6's move to being a highly pluggable 'platform' and this mode would benefit from their approach to presets. safe transforms as a default that just gives you the minimal amount needed to get you up and running sounds like a good bet. The above breakdown for cssnano-preset-advanced, cssnano-preset-experimental etc. also seems fairly sane. I use cssnano in some of my projects and wouldn't have a hard time at all determining what those groupings relate to so 👍

Babel 6 also have some good plugin optimizations by combining many plugins into one step. Might be worth looking into for inspiration.

👍

@MoOx
Copy link
Contributor

MoOx commented Jan 8, 2016

+10 to @addyosmani comment 👌🏼

@MoOx
Copy link
Contributor

MoOx commented Jan 8, 2016

Maybe more postcss-preset-cssnano-* (long but make even more sense). Or just postcss-cssnano-*.

@ben-eb
Copy link
Collaborator Author

ben-eb commented Jan 8, 2016

@addyosmani Thanks for the feedback! 👍

@MoOx At the moment I am thinking of playing around in a cssnano-* namespace as I feel that we can improve performance in our own sandbox, and look to bringing those changes back to PostCSS. Right now I have a performance problem that I hope to solve by merging the more expensive parts of each plugin (parsing, stringifying, iterating), without the concession that each has to be an independent transform that can plug into PostCSS.

We have tried the independent transform approach up until this point and it is slow. So I would like to see if we can do better with visitors and a common parsing/stringifying step for values/node trees. 😄

@MoOx
Copy link
Contributor

MoOx commented Jan 8, 2016

Fair enough.

@Garbee
Copy link

Garbee commented Jan 8, 2016

I think one nice thing to introduce is the idea of the configuration being a project-file. Like .cssnanorc, which could also be included within the package.json config to keep the number of files per-project down. Babel's configuration system looks for the dotfile first and then checks package.json for a specific key. It would be nice to replicate the same setup.

The main benefit here is, speed to check out a projects ruleset. If you know it is one of these places then it is faster to find and view then digging through any given projects build pipeline.

EDIT: Whoops, just realized this is issue #91 for V4. Sorry for the needless comment.

@devinus
Copy link

devinus commented Feb 6, 2016

The performance right now is the only thing keeping us from using cssnano as a big part of our pipeline.

@ben-eb
Copy link
Collaborator Author

ben-eb commented Feb 6, 2016

If you have some large CSS that creates a performance problem, it would be nice to share so that I can have a look at areas in where we can improve performance. 😄

@devinus
Copy link

devinus commented Feb 6, 2016

It's not that it's large, but we need to minify millions of CSS documents as fast as possible. Right now we're using cleancss, but investigating csswring so we don't introduce more than one CSS parser. Obviously cssnano would be rad, but it's about 1.5x-2x slower than the alternatives.

@felixfbecker
Copy link

Why is autoprefixer even bundled with cssnano? I think of cssnano as a CSS minifier, and vendor prefixing doesn't have anything to do with minifying... It does the opposite if you think about it. I would prefer to use autoprefixer directly in Gulp, and update it seperately. Write programs that do one thing and do it well

@TrySound
Copy link
Collaborator

@felixfbecker autoprefixer here only removes unnecessary prefixes. You can disable it in your build. And why you use split autoprefixer in gulp plugin? Parse once in one postcss pipe via gulp-postcss.

@jlukic
Copy link

jlukic commented May 3, 2016

I just have to say after spending 3 hours today trying to figure out why all the z-indexes in my site were breaking. I was very surprised to find this was a performance tweak that was enabled by default.

Very excited to see safe will be on by default in 4.

@Ptico
Copy link

Ptico commented Jun 15, 2016

@ben-eb if you are looking for large and slow css example, take a look at this one http://semantic-ui.com/introduction/getting-started.html

@ben-eb
Copy link
Collaborator Author

ben-eb commented Jun 1, 2017

I've published an alpha build that is much less ambitious than the original goals I've outlined here, especially when it comes to performance & reducing parsing steps. In all honesty I think I'm reaching PostCSS' limits already, and while I did try to do a build that would combine all three parsers into a Frankenstein parser, it was too complex to make something useful out of, and I probably would have been better off writing another CSS parser from scratch. That's too time-consuming. 😄

But yet, we still have presets which was one of my main goals, and a bunch more things. Check it out at https://github.com/ben-eb/cssnano/releases/tag/v4.0.0-rc.0

@towry
Copy link

towry commented Oct 9, 2017

why change zindex by default?????????????

@cdeutsch
Copy link

Please don't change the z-index by default. That's just madness and a huge recipe for bugs.

@ai
Copy link
Member

ai commented May 26, 2018

Agree. z-index was the most confused rule. We can keep the rule, just disable it by default.

@alexander-akait
Copy link
Member

alexander-akait commented May 28, 2018

zindex disabled by default in cssnano

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

No branches or pull requests