Replies: 1 comment 3 replies
-
I also believe the current 'explode and merge As a curiosity, some researchers found that merging CSS rules is an NP-hard problem https://anthonywlin.github.io/papers/toplas19.pdf. But I
@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) |
Beta Was this translation helpful? Give feedback.
-
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 howpostcss-merge-longhand
does things - and was eager to get some practise with thepostcss
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 sincefont
has quite different considerations than properties relating to corner or edges of a box, which seemed to be the main focus ofpostcss-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 shorthandOne 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
andbackground
.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. Thereforeinherit
in place for an individual value within a shorthand is invalid - as pointed out in several of the bug reports related topostcss-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 theinherit
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:
Should lead to this:
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 withinpostcss-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
andbackground
(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)postcss-merge-longhand-cols
(split out)postcss-merge-longhand-font
(add later)postcss-merge-longhand-bg
(add later)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.
Beta Was this translation helpful? Give feedback.
All reactions