fix(merge-rules): avoid merging conflicting borders #1450
Conversation
| return true; | ||
| } | ||
| const nextConflictInIntersection = intersection | ||
| .slice(intersectIndex + 1) | ||
| .filter((d) => isConflictingProp(d.prop, decl.prop)); | ||
| if (!nextConflictInIntersection.length) { |
There was a problem hiding this comment.
Next condition ensures that nextConflictInIntersection.length !== 0 since already nextConflictInFirst.length !== 0, but in fact it even does not matter, as long as the number of conflicts is the same we can still hope the merge will work.
| // Conflict if rest-count mismatches | ||
| if (a.rest.length !== b.rest.length) { | ||
| return true; | ||
| } | ||
|
|
||
| /* Do not merge conflicting border properties */ | ||
| if (a.base === 'border') { |
There was a problem hiding this comment.
I find this function uses some non-obvious terminology. base is the first part of the property name excluding the vendor prefix. rest all the rest of the name.
I could not find an alternative to using a special case, CSS syntax is just not that regular. I think in fact conflict if count mismatches or if every part is the same is only true for the margin- and padding- properties.
I would rather rewrite the logic when we drop Node 12 and 14 and release 6.0, this is fix for people who will remain on 5.x. Do you think there's a more general method to merge the rules? I think you can likely do more than merging only groups of adjacent two rules, but you still need to make special cases for the properties with different names but can override each other |
|
'Evil' case: .a { place-content: center; justify-content: start; } .b { justify-content: start; place-content: center; } |
Codecov ReportBase: 97.61% // Head: 97.62% // Increases project coverage by
📣 This organization is not using Codecov’s GitHub App Integration. We recommend you install it so Codecov can continue to function properly for your repositories. Learn more Additional details and impacted files@@ Coverage Diff @@
## master #1450 +/- ##
=======================================
Coverage 97.61% 97.62%
=======================================
Files 122 122
Lines 9999 10010 +11
=======================================
+ Hits 9761 9772 +11
Misses 238 238
Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here. ☔ View full report at Codecov. |
ludofischer
force-pushed
the
merge-rules-refactor
branch
from
3b34c26
to
ea403e2
Compare
|
I've added a fix for the |
Yeah, I am fine with it after drop Node 12 and Node 14
I think we should |
Does this reorder logic exist some inside cssnano? What I did was detecting if one of the properties was place-*, then don't merge. ea403e2#diff-106bd8df0f811c5c47fcb5841936b1a954ea692da68c1b246384bec798328d5fR168 From looking at the list of all shorthands, I think |
I think if we move it https://github.com/cssnano/cssnano/blob/master/packages/cssnano-preset-default/src/index.js#L124 before merge-rules we will fix most of cases |
I see. What do you want to do with the current PR? Merge it for 5.x or just leave it out? |
|
@ludofischer Let's add TODO/create an issue about it to not lose context and we can merge as patch for fast fix |
* add function return type * explicitly check array size * We've already checked that nextConflictInFirst is not 0, so we only care if the number of conflicts in the intersection is different.
The previous logic assumed properties did not conflict if their names were not equal but contained the same number of dash-separated parts. This works for margin and padding but does not work for border, because for example border-left and border-color can conflict. This change prevents a valid optimization when the conflicting properties re-declared the same value (for example border-width: 10px and border: 10px solid blue), but that's the same as for other properties.
`place` conflicts with `justify` and `align`, even if they don't start with the same property.
ludofischer
force-pushed
the
merge-rules-refactor
branch
from
ea403e2
to
1e5e63a
Compare
|
I have create an issue for a better fix as you've suggested: #1466 Do you think we can merge this? Then I can make a new release with the other fixes that have been contributed recently. |
|
@ludofischer yeah, let's do it 👍 |
The previous logic assumed properties did not conflict if their
names were not equal but contained the same number of dash-separated parts.
This works for margin and padding but does not work for border,
because for example border-left and border-color can conflict.
This change prevents a valid optimization when the conflicting properties
re-declared the same value (for example border-width: 10px and
border: 10px solid blue), but that's the same as for other properties.
Related to #1000 and #701
I think a similar problem is going to happen for
place-itemsandalign-itemsorjustify-itemssince they conflict but escape the current conflict detection algorithm (it assumes no conflict if the names property names start with a different word).