From 9b960009b604bac7e91404917bd87033126caf43 Mon Sep 17 00:00:00 2001
From: Grant Wong <2908767+dddlr@users.noreply.github.com>
Date: Wed, 23 Oct 2024 19:01:43 +1300
Subject: [PATCH] Fix shorthand sorting buggy (#1730)

* 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
---
 .changeset/witty-bags-complain.md             |   5 +
 .../sort-shorthand-declarations.test.ts       | 144 +++++++++++++++---
 .../src/plugins/sort-atomic-style-sheet.ts    |  12 +-
 .../plugins/sort-shorthand-declarations.ts    |  46 ++----
 packages/css/src/sort.ts                      |  12 +-
 5 files changed, 159 insertions(+), 60 deletions(-)
 create mode 100644 .changeset/witty-bags-complain.md

diff --git a/.changeset/witty-bags-complain.md b/.changeset/witty-bags-complain.md
new file mode 100644
index 000000000..be3c22357
--- /dev/null
+++ b/.changeset/witty-bags-complain.md
@@ -0,0 +1,5 @@
+---
+'@compiled/css': minor
+---
+
+Fix shorthand sorting not working most of the time, when stylesheet extraction is turned on.
diff --git a/packages/css/src/plugins/__tests__/sort-shorthand-declarations.test.ts b/packages/css/src/plugins/__tests__/sort-shorthand-declarations.test.ts
index 34f2356d3..01e050e6b 100644
--- a/packages/css/src/plugins/__tests__/sort-shorthand-declarations.test.ts
+++ b/packages/css/src/plugins/__tests__/sort-shorthand-declarations.test.ts
@@ -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;
@@ -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 {
@@ -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 {
@@ -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 {
@@ -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;
@@ -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] {
@@ -308,6 +309,12 @@ 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;
@@ -315,12 +322,6 @@ describe('sort shorthand vs. longhand declarations', () => {
       .f {
         display: block;
       }
-      .d {
-        border-top: none;
-      }
-      .c {
-        border-block-start: none;
-      }
       .e {
         border-block-start-color: transparent;
       }
@@ -328,18 +329,16 @@ describe('sort shorthand vs. longhand declarations', () => {
       .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;
         }
@@ -349,6 +348,9 @@ describe('sort shorthand vs. longhand declarations', () => {
         .c {
           border-block-start: none;
         }
+        .f {
+          display: block;
+        }
         .e {
           border-block-start-color: transparent;
         }
@@ -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`
diff --git a/packages/css/src/plugins/sort-atomic-style-sheet.ts b/packages/css/src/plugins/sort-atomic-style-sheet.ts
index 03a5e4e4d..57d93335a 100644
--- a/packages/css/src/plugins/sort-atomic-style-sheet.ts
+++ b/packages/css/src/plugins/sort-atomic-style-sheet.ts
@@ -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);
@@ -101,10 +109,6 @@ export const sortAtomicStyleSheet = (config: {
       }
 
       root.nodes = [...catchAll, ...rules, ...atRules.map((atRule) => atRule.node)];
-
-      if (sortShorthandEnabled) {
-        sortShorthandDeclarations(root.nodes);
-      }
     },
   };
 };
diff --git a/packages/css/src/plugins/sort-shorthand-declarations.ts b/packages/css/src/plugins/sort-shorthand-declarations.ts
index 5272a32d7..7da88adb5 100644
--- a/packages/css/src/plugins/sort-shorthand-declarations.ts
+++ b/packages/css/src/plugins/sort-shorthand-declarations.ts
@@ -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;
   }
@@ -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;
@@ -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 => {
diff --git a/packages/css/src/sort.ts b/packages/css/src/sort.ts
index 303f0467c..f2c05cb7f 100644
--- a/packages/css/src/sort.ts
+++ b/packages/css/src/sort.ts
@@ -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(),