Skip to content

Commit

Permalink
Fix shorthand sorting buggy (#1730)
Browse files Browse the repository at this point in the history 
* Fix shorthand sorting buggy

* Add changeset + fix build error

* Change patch to minor

* Fix bug where media queries are erroneously sorted if they contain one declaration

* Make the sort config optional

* Change pseudo-selector sorting to take priority over shorthand sorting
  • Loading branch information
@dddlr
dddlr authored Oct 23, 2024
1 parent be74f07 commit 9b96000
Show file tree
Hide file tree
Showing 5 changed files with 159 additions and 60 deletions.
5 changes: 5 additions & 0 deletions .changeset/witty-bags-complain.md
View file
Original file line number Diff line number Diff line change
@@ -0,0 +1,5 @@
---
'@compiled/css': minor
---

Fix shorthand sorting not working most of the time, when stylesheet extraction is turned on.
144 changes: 124 additions & 20 deletions packages/css/src/plugins/__tests__/sort-shorthand-declarations.test.ts
View file
Original file line number Diff line number Diff line change
Expand Up @@ -36,8 +36,8 @@ describe('sort shorthand vs. longhand declarations', () => {

expect(actual).toMatchInlineSnapshot(`
".a {
outline-width: 1px;
font: 16px;
outline-width: 1px;
}
.b {
font: 24px;
Expand Down Expand Up @@ -69,8 +69,8 @@ describe('sort shorthand vs. longhand declarations', () => {
expect(actual).toMatchInlineSnapshot(`
".a {
outline: none;
outline-width: 1px;
font: 16px normal;
outline-width: 1px;
font-weight: bold;
}
.b {
Expand Down Expand Up @@ -147,8 +147,8 @@ describe('sort shorthand vs. longhand declarations', () => {
.a {
outline: none;
outline-width: 1px;
font: 16px normal;
outline-width: 1px;
font-weight: bold;
}
.b {
Expand All @@ -162,14 +162,14 @@ describe('sort shorthand vs. longhand declarations', () => {
.a:focus {
outline: none;
outline-width: 1px;
font: 16px normal;
outline-width: 1px;
font-weight: bold;
}@media all {
.a {
outline: none;
outline-width: 1px;
font: 16px normal;
outline-width: 1px;
font-weight: bold;
}
.b {
Expand Down Expand Up @@ -210,6 +210,7 @@ describe('sort shorthand vs. longhand declarations', () => {

it('sorts a variety of different shorthand properties used together', () => {
const actual = transform(outdent`
@media all {
.f {
display: block;
Expand Down Expand Up @@ -284,10 +285,10 @@ describe('sort shorthand vs. longhand declarations', () => {

expect(actual).toMatchInlineSnapshot(`
"
.a {
.a > .external {
all: unset;
}
.a > .external {
.a {
all: unset;
}
.b[disabled] {
Expand All @@ -308,38 +309,36 @@ describe('sort shorthand vs. longhand declarations', () => {
.c[data-foo='bar'] {
border-top: none;
}
.d {
border-top: none;
}
.c {
border-block-start: none;
}
.j {
margin-bottom: 6px;
}
.f {
display: block;
}
.d {
border-top: none;
}
.c {
border-block-start: none;
}
.e {
border-block-start-color: transparent;
}
.f:focus {
display: block;
}
.e:hover {
border-block-start-color: transparent;
}
.d:active {
border-block-start: none;
}
.e:hover {
border-block-start-color: transparent;
}@media all {
@media all {
.a {
all: unset;
}
.f {
display: block;
}
.b {
border: none;
}
Expand All @@ -349,6 +348,9 @@ describe('sort shorthand vs. longhand declarations', () => {
.c {
border-block-start: none;
}
.f {
display: block;
}
.e {
border-block-start-color: transparent;
}
Expand Down Expand Up @@ -386,13 +388,115 @@ describe('sort shorthand vs. longhand declarations', () => {
border-block-start: 1px solid;
border-block-start-color: transparent;
}
.b { all: unset; }
.c { all: unset; }
.b { all: unset; }
.d { border: none; }
.f { border-block-start-color: transparent; }"
`);
});

it('sorts a stylesheet that is mainly longhand properties', () => {
const actual = transform(outdent`
._oqicidpf{padding-top:0}
._1rmjidpf{padding-right:0}
._cjbtidpf{padding-bottom:0}
._pnmbidpf{padding-left:0}
._glte7vkz{width:1pc}
._165t7vkz{height:1pc}
._ue5g1408{margin:0 var(--ds-space-800,4pc)}
._1yag1dzv{padding:var(--ds-space-100) var(--ds-space-150)}
._dbjg12x7{margin-top:var(--ds-space-075,6px)}
@media (min-width:1200px){
._jvpg11p5{display:grid}
._szna1wug{margin-top:auto}
._13on1wug{margin-right:auto}
._1f3k1wug{margin-bottom:auto}
._inid1wug{margin-left:auto}
._1oqj1epz{padding:var(--ds-space-1000,5pc)}
._12wp9ac1{max-width:1400px}
._jvpgglyw{display:none}
}
`);

expect(actual).toMatchInlineSnapshot(`
"
._ue5g1408{margin:0 var(--ds-space-800,4pc)}
._1yag1dzv{padding:var(--ds-space-100) var(--ds-space-150)}._oqicidpf{padding-top:0}
._1rmjidpf{padding-right:0}
._cjbtidpf{padding-bottom:0}
._pnmbidpf{padding-left:0}
._glte7vkz{width:1pc}
._165t7vkz{height:1pc}
._dbjg12x7{margin-top:var(--ds-space-075,6px)}
@media (min-width:1200px){
._1oqj1epz{padding:var(--ds-space-1000,5pc)}
._jvpg11p5{display:grid}
._szna1wug{margin-top:auto}
._13on1wug{margin-right:auto}
._1f3k1wug{margin-bottom:auto}
._inid1wug{margin-left:auto}
._12wp9ac1{max-width:1400px}
._jvpgglyw{display:none}
}"
`);
});

it('sorts border, border-top, border-top-color', () => {
const actual = transform(outdent`
._abcd1234 { border-top-color: red }
._abcd1234 { border-top: 1px solid }
._abcd1234 { border: none }
._abcd1234:hover { border-top-color: red }
._abcd1234:hover { border-top: 1px solid }
._abcd1234:hover { border: none }
._abcd1234:active { border-top-color: red }
._abcd1234:active { border-top: 1px solid }
._abcd1234:active { border: none }
@supports (border: none) {
._abcd1234 { border-top-color: red }
._abcd1234 { border-top: 1px solid }
._abcd1234 { border: none }
}
@media (max-width: 50px) { ._abcd1234 { border-top-color: red } }
@media (max-width: 100px) { ._abcd1234 { border-top: 1px solid } }
@media (max-width: 120px) {
._abcd1234 { border-top-color: red }
._abcd1234 { border-top: 1px solid }
._abcd1234 { border: none }
}
@media (max-width: 150px) { ._abcd1234 { border: none } }
`);

expect(actual).toMatchInlineSnapshot(`
"
._abcd1234 { border: none }
._abcd1234 { border-top: 1px solid }
._abcd1234 { border-top-color: red }
._abcd1234:hover { border: none }
._abcd1234:hover { border-top: 1px solid }
._abcd1234:hover { border-top-color: red }
._abcd1234:active { border: none }
._abcd1234:active { border-top: 1px solid }
._abcd1234:active { border-top-color: red }
@media (max-width: 150px) { ._abcd1234 { border: none } }
@media (max-width: 120px) {
._abcd1234 { border: none }
._abcd1234 { border-top: 1px solid }
._abcd1234 { border-top-color: red }
}
@media (max-width: 100px) { ._abcd1234 { border-top: 1px solid } }
@media (max-width: 50px) { ._abcd1234 { border-top-color: red } }
@supports (border: none) {
._abcd1234 { border: none }
._abcd1234 { border-top: 1px solid }
._abcd1234 { border-top-color: red }
}"
`);
});

describe('when disabled', () => {
it('does nothing when crossover detected', () => {
const actual = transformWithoutSorting(outdent`
Expand Down
12 changes: 8 additions & 4 deletions packages/css/src/plugins/sort-atomic-style-sheet.ts
View file
Original file line number Diff line number Diff line change
Expand Up @@ -87,6 +87,14 @@ export const sortAtomicStyleSheet = (config: {
}
});

if (sortShorthandEnabled) {
sortShorthandDeclarations(catchAll);
sortShorthandDeclarations(rules);
sortShorthandDeclarations(atRules.map((atRule) => atRule.node));
}

// Pseudo-selector and at-rule sorting takes priority over shorthand
// property sorting.
sortPseudoSelectors(rules);
if (sortAtRulesEnabled) {
atRules.sort(sortAtRules);
Expand All @@ -101,10 +109,6 @@ export const sortAtomicStyleSheet = (config: {
}

root.nodes = [...catchAll, ...rules, ...atRules.map((atRule) => atRule.node)];

if (sortShorthandEnabled) {
sortShorthandDeclarations(root.nodes);
}
},
};
};
Expand Down
46 changes: 12 additions & 34 deletions packages/css/src/plugins/sort-shorthand-declarations.ts
View file
Original file line number Diff line number Diff line change
@@ -1,9 +1,9 @@
import { shorthandBuckets, shorthandFor, type ShorthandProperties } from '@compiled/utils';
import { shorthandBuckets, type ShorthandProperties } from '@compiled/utils';
import type { ChildNode, Declaration } from 'postcss';

const nodeIsDeclaration = (node: ChildNode): node is Declaration => node.type === 'decl';

const findDeclaration = (node: ChildNode): Declaration | Declaration[] | undefined => {
const findDeclaration = (node: ChildNode): Declaration | undefined => {
if (nodeIsDeclaration(node)) {
return node;
}
Expand All @@ -12,14 +12,6 @@ const findDeclaration = (node: ChildNode): Declaration | Declaration[] | undefin
if (node.nodes.length === 1 && nodeIsDeclaration(node.nodes[0])) {
return node.nodes[0];
}

const declarations = node.nodes.map(findDeclaration).filter(Boolean) as Declaration[];

if (declarations.length === 1) {
return declarations[0];
}

return declarations;
}

return undefined;
Expand All @@ -29,39 +21,25 @@ const sortNodes = (a: ChildNode, b: ChildNode): number => {
const aDecl = findDeclaration(a);
const bDecl = findDeclaration(b);

// Don't worry about any array of declarations, this would be something like a group of
// AtRule versus a regular Rule.
//
// Those are sorted elsewhere…
if (Array.isArray(aDecl) || Array.isArray(bDecl)) return 0;

// This will probably happen because we have an AtRule being compared to a regular
// Rule. Don't try to sort this - the *contents* of the AtRule will be traversed and
// sorted by sortShorthandDeclarations, and the sort-at-rules plugin will sort AtRules
// so they come after regular rules.
if (!aDecl?.prop || !bDecl?.prop) return 0;

const aShorthand = shorthandFor[aDecl.prop as ShorthandProperties];
if (aShorthand === true || aShorthand?.includes(bDecl.prop)) {
return -1;
}

const bShorthand = shorthandFor[bDecl.prop as ShorthandProperties];
if (bShorthand === true || bShorthand?.includes(aDecl.prop)) {
return 1;
}

const aShorthandBucket = shorthandBuckets[aDecl.prop as ShorthandProperties];
const bShorthandBucket = shorthandBuckets[bDecl.prop as ShorthandProperties];
// Why default to Infinity? Because if the property is not a shorthand property,
// we want it to come after all of the other shorthand properties.
const aShorthandBucket = shorthandBuckets[aDecl.prop as ShorthandProperties] ?? Infinity;
const bShorthandBucket = shorthandBuckets[bDecl.prop as ShorthandProperties] ?? Infinity;

// Ensures a deterministic sorting of shorthand properties in the case where those
// shorthand properties overlap.
//
// For example, `border-top` and `border-color` are not shorthand properties of
// each other, BUT both properties are shorthand versions of `border-top-color`.
// If `border-top` is in bucket 12 and `border-color` is in bucket 6, we can ensure
// If `border-top` is in bucket 4 and `border-color` is in bucket 2, we can ensure
// that `border-color` always comes before `border-top`.
if (aShorthandBucket && bShorthandBucket) {
return aShorthandBucket - bShorthandBucket;
}

return 0;
return aShorthandBucket - bShorthandBucket;
};

export const sortShorthandDeclarations = (nodes: ChildNode[]): void => {
Expand Down
12 changes: 10 additions & 2 deletions packages/css/src/sort.ts
View file
Original file line number Diff line number Diff line change
Expand Up @@ -8,14 +8,22 @@ import { sortAtomicStyleSheet } from './plugins/sort-atomic-style-sheet';
* Sorts an atomic style sheet.
*
* @param stylesheet
* @returns
* @returns the sorted stylesheet
*/
export function sort(
stylesheet: string,
{
sortAtRulesEnabled,
sortShorthandEnabled,
}: { sortAtRulesEnabled: boolean | undefined; sortShorthandEnabled: boolean | undefined }
}: { sortAtRulesEnabled: boolean | undefined; sortShorthandEnabled: boolean | undefined } = {
// These default values should remain undefined so we don't override the default
// values set in packages/css/src/plugins/sort-atomic-style-sheet.ts
//
// Modify packages/css/src/plugins/sort-atomic-style-sheet.ts if you want to
// update the actual default values for sortAtRulesEnabled and sortShortEnabled.
sortAtRulesEnabled: undefined,
sortShorthandEnabled: undefined,
}
): string {
const result = postcss([
discardDuplicates(),
Expand Down

0 comments on commit 9b96000

Please sign in to comment.