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)