Refactor postcss-merge-longhand

[sigveio]

This is a follow up from #1164

In relation to that issue, I had a look at postcss-merge-longhand

At first, I thought parts of it seemed a bit complex/difficult to follow.

Which doesn't have to be because it is; I also lack experience with the ecosystem.

So as an exercise I decided to throw together a 'proof-of-concept' for merging font longhand as a new plugin. I wanted to see if my thoughts about how to tackle it were sensible before being too biased by how postcss-merge-longhand does things - and was eager to get some practise with the postcss API.

My thinking was that this would also help me understand postcss-merge-longhand better - and how (or whether) a font related mechanism would fit into that existing framework. Especially since font has quite different considerations than properties relating to corner or edges of a box, which seemed to be the main focus of postcss-merge-longhand currently.

And it was a good learning experience, which helped tie things together for me

---

Just to write down some thoughts from that for later;

(Both for myself to remember, and for transparency / inviting others to discuss)

Similarities

The cornerstone of postcss-merge-longhand's approach is to walk through the declarations within a given rule, "explode" any existing shorthand declarations into corresponding longhand temporarily, de-duplicate, and "merge" it all back together.

The solution I ended up with also shared this trait. It seems like the logical way to approach it - and from that, I do understand the original grouping of these plugins together into one. The shared "boxbase" for margin and padding is clever.

Differences

An area where the approach in my experiment differed slightly is that... instead of having the "explode" and "merge" steps act on the tree independently from each other... the font plugin built up an internal registry of both the 'shorthand exploded into longhand' and existing relevant longhand declarations for a given rule in the "explode" step -- and only did the actual changes (deleting, cloning, inserting) to the tree in the "merge" step. To allow checks to look at all the relevant declarations within a rule as a whole before changing the tree.

But I also understand why the opposite originally made sense for box-related properties, since it allowed for a convenient way of expanding from shorthand -> shorthand for each box-side -> individual longhands for each box-side in iterations. The concerns there are also somewhat different.

Importance of initial values when exploding shorthand

One takeaway I learned, is that when "exploding" an existing shorthand... it's important to be aware that any omitted values within that shorthand should be set to their respective initial values. And those have precedence over any regular corresponding longhand that comes before it. So if you just "explode" without creating new declarations for those omitted values, you'll be in trouble when you merge.

This would be super important to keep in mind when adding support for things like font and background.

It appears the existing plugins within postcss-merge-longhand take this into account, so that seems good. 👍

PS: This consideration causes output to be longer than the input in some cases. More on that later.

Related issues with inherit

I also noted that due to the behaviour with omitted values being replaced by initial values, omitting them does not allow inheritance. Therefore inherit in place for an individual value within a shorthand is invalid - as pointed out in several of the bug reports related to postcss-merge-longhand

From reading those reports it also appears that there is a logic problem somewhere in the borders plugin related to either the order declarations are re-inserted into the tree... or how deduplication/reducing is done later. Which for example causes the unexpected behaviour in #864 where the inherit is dropped instead of given precedence. And my gut feeling says this is probably also related to the behaviour seen in #1044 - because that should not happen either if the order of operations in "exploding" and "merging" was right.

Potential for sub-optimal optimisation

If the bug above is identified and fixed, an "explosion" and "merging" of this:

border: 3px solid transparent;
border-color: inherit;
border-bottom-color: transparent;

Should lead to this:

border: 3px solid transparent;
border-top-color: inherit;
border-right-color: inherit;
border-left-color: inherit;

Which is semantically correct but not an optimisation - the result is ~30% longer than the original.

And even less optimal for the example in #1044 - which would be ~150% longer when correctly "exploded" and "merged".

This problem was also mentioned by @ludofischer in #1100

So perhaps the slightly different approach I touched upon with "not changing the tree until after the whole rule has been processed" could have some benefit in that regard. Then we could sum up the character length of the declarations before and after, and skip if there is no gain. If done right, I don't think that would be overly expensive to do in terms of performance.

What could be simplified?

One thing that sticks out, and has also been suggested by others, is that there appears to be potential for simplifying the borders plugin within postcss-merge-longhand. I think I would boil it down to the basics without all the special handling for custom props/vars - and focus on getting that as solid / bug free and readable/understandable/maintainable as possible.

The rest can always be re-built on top later if needed.

Does it make sense to split postcss-merge-longhand?

I think my findings so far is that it makes sense to keep things related to edges and corners of a box together - especially margin and padding can be handled by the same logic. And borders also benefits from re-using some code there.

But to aid in future maintainability and keep with the cssnano spirit of "many focused optimisations", I think I would do longhand merging for font and background (which have very different considerations) as separate packages. And also split out columns, because it's enough different from "box" properties to live on its own.

So perhaps;

• postcss-merge-longhand -> postcss-merge-longhand-box (refactor/simplify/fix bugs)
  • padding
  • margin
  • borders
• postcss-merge-longhand-cols (split out)
  • columns
• postcss-merge-longhand-font (add later)
  • font
• postcss-merge-longhand-bg (add later)
  • background

This would also make it easier to slowly introduce the new features as optional/experimental additions. And later add them to presets when they are deemed stable.

[ludofischer]

I also believe the current 'explode and mergeimplementation is fundamentally incorrect. Do you believe that correctly expanding values toinitialand takinginherit` into account before merging back into shorthand would give a correct solution?

As a curiosity, some researchers found that merging CSS rules is an NP-hard problem https://anthonywlin.github.io/papers/toplas19.pdf. But I

But to aid in future maintainability and keep with the cssnano spirit of "many focused optimisations", I think I would do longhand merging for font and background (which have very different considerations) as separate packages.

@alexander-akait and I were considering merging plugins to allow turning all minifications on without installing extra packages and improve performance See #1098 for more context. I think we could improve maintanability by organizing optimizations with modules, without necessarily placing each one in different PostCSS plugins. For example, grouping trasformations that act on the same parts of the tree (for instance all selector transforms together)

[sigveio]

In theory, yes I believe so - but there might be things I'm not considering due to the codebase and postcss ecosystem still being somewhat fresh to me. I started doing the same experiment for borders as I did with font, discarding the old solution and writing my own from scratch to explore my interpretation of how to solve the problem and gain more insight. It seems to be doing the exploding quite reliably for all the variations of the properties and with less and cleaner code. I've got to the point of having a de-duplicated and correctly ordered final list of longhands that needs to be merged, and so far it looks promising. I think I can answer your question better once I get past that point, and have the basic essence of it down.

Currently I'm having a little pause from it to work on improving minification of gradients, which I think I will aim to finish up before I circle back to postcss-merge-longhand. :-)

Interesting about the research; thanks for sharing 🤓

I will follow up separately in #1098 in the not so distant future regarding thoughts on structure.

PS: I deleted my last two replies here in an effort to keep things a bit more focused. So this reply is mostly a re-post of the more relevant part. I think I need to get myself a blog to quench this writing need I seem to have. 😆

✌️

[ludofischer]

PS: I deleted my last two replies here in an effort to keep things a bit more focused. So this reply is mostly a re-post of the more relevant part.

I remember there was a part about performance which seems to have disappeared. This flamegraph shows most of the time is spent inside postcss and css-value-parser (ignore the zlib part).
I find that this result is not incredible: someone also found that changing a regex in postcss-selector-parser generated a 63% speedup: postcss/postcss-selector-parser#239
The PostCSS creator also indicates parsing is the big bottleneck: https://twitter.com/sitnikcode/status/1385209163131858944
Looking at that flamegraph, speeding up css-value-parser might have more impact than speeding up cssnano itself! That's why I suspect the reason cssnano is twice as slow as the other css minizers is that it traverses the CSS file over and over again.

[sigveio]

Ups, sorry, answering this one managed to slip my mind.

To also continue the somewhat related talk in #1181:

Working on these optimizations is indeed a lot of work, I think you have to undertake it more for fun than for anything else. From the various tests I've found on the web, switching to brotli compression from gzip gives a size decrease that's much bigger than the difference between the output of any two css minifiers. So a generic text compression algorithm beats techniques that use CSS domain knowledge.

Absolutely; I'm tackling this because I enjoy the opportunity to learn and hone my skills. This kind of deep domain knowledge might seem a bit obscure, but tends to come in handy sooner or later. And in the pursuit of correctness, it should give me an increased chance of considering all the different angles (and write meaningful unit tests) when I refactor.

And in my opinion, correctness should be the primary quality gate for a minimiser. Long before size or performance. Because without, it's not an optimisation at all... and thus not worth doing.

I noticed that Angular is in the process of replacing cssnano with the Golang-based esbuild, which boasts pretty impressive performance numbers. And despite doing less with the CSS, I suspect it produced similar sized output in their tests due to the compression difference you mention.

Probably we could never compete against Golang on speed with JS... but there is definitely still a place for us in the rest of the node ecosystem if we work on maintaining quality. So that's where I personally would choose to direct focus for now.

The vast majority of our codebase is only covered by functional tests - which are actually really complex to maintain, understand, and proof over time. I think if we gradually work on refactoring with the aim of also adding unit test coverage... it would bring a lot of advantages across the board. Because, besides squashing both known and unknown bugs, the way you have to structure the code - and the insight you gain while doing that refactoring - would undoubtedly bring improvements in both performance and maintainability as a side effect. Especially in an environment where the spec you are working against is a moving target.

An example performance wise is postcss-merge-longhand which constantly throws the ball back to postcss and triggers at the very least re-parsing of a section of the tree many many times. Avoiding that kind of stuff would help, I think.

Having the code paired with tests would make it much easier for new developers to come into the project as well, and pave the way for being able to confidently make bigger changes/updates further down the line. I would even go as far as saying it should be a prerequisite for even considering something like #1098.