Fix shorthand sorting buggy #1730

dddlr · 2024-10-22T05:14:24Z

What is this change?

Fix the shorthand property sorting being buggy when not using runtime mode (e.g. when using stylesheet extraction).

Why are we making this change?

@kylorhall-atlassian and I noticed that sortShorthand wasn't actually sorting the properties as it should.

It turns out that a lot of the time in the sorting algorithm, the two properties being compared are

not related to each other (e.g. padding is related to paddingLeft, but padding is not related to borderColor), and
one of the properties is a longhand property that doesn't exist in shorthandBuckets

As a result, there are cases where the stylesheet would not be sorted at all, simply because the wrong nodes were being compared and so the sortNodes function returns 0 every time.

How are we making this change?

Fix and simplify the sortNodes function:

Remove the logic of checking whether one property is a shorthand of another. It is safe to assume that this logic evaluates to false most of the time anyway.
In aShorthandBucket and bShorthandBucket, set the default value to Infinity. This way, if one property is a shorthand property and the other property is not a shorthand property, we guarantee that the shorthand property will come in front. For example, margin will come before paddingRight (even though they're not related).

PR checklist

Don't delete me!

I have...

Updated or added applicable tests
Verify the changes to existing tests are correct
Updated the documentation in website/
Verify that this PR fixes the lack of shorthand sorting in Jira
Address this PR comment from Kylor: Fix shorthand sorting buggy #1730 (comment)
Add changeset

changeset-bot · 2024-10-22T05:14:28Z

🦋 Changeset detected

Latest commit: 8b6edba

The changes in this PR will be included in the next version bump.

This PR includes changesets to release 5 packages

Name	Type
@compiled/css	Minor
@compiled/babel-plugin-strip-runtime	Patch
@compiled/babel-plugin	Patch
@compiled/parcel-optimizer	Patch
@compiled/webpack-loader	Patch

Not sure what this means? Click here to learn what changesets are.

Click here if you're a maintainer who wants to add another changeset to this PR

netlify · 2024-10-22T05:14:50Z

✅ Deploy Preview for compiled-css-in-js canceled.

Name	Link
🔨 Latest commit	`8b6edba`
🔍 Latest deploy log	https://app.netlify.com/sites/compiled-css-in-js/deploys/67183ef7ea580e000836a4d3

dddlr · 2024-10-22T05:15:33Z

packages/css/src/plugins/__tests__/sort-shorthand-declarations.test.ts

      .d { border: none; }
      .f { border-block-start-color: transparent; }"
    `);
  });

+  it('sorts a subset of atomic classes taken from a real stylesheet', () => {


this is a small subset of the jira stylesheet that i found was not being sorted correctly

Got it, some other thoughts if they're not tested in here should be border > border-top > border-top-color right?

Input:

._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 } @media (…) { ._abcd1234 { border-top-color: red } } @media (…) { ._abcd1234 { border-top: 1px solid } } @media (…) { ._abcd1234 { border: none } }

Output:

._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 } @media (…) { ._abcd1234 { border: none } } @media (…) { ._abcd1234 { border-top: 1px solid } } @media (…) { ._abcd1234 { border-top-color: red } }

The main question I'd have is do we still maintain this pseudo and media query sorting, the rest seems fine to me…

dddlr · 2024-10-22T05:17:22Z

packages/css/src/plugins/sort-shorthand-declarations.ts

@@ -37,31 +37,19 @@ const sortNodes = (a: ChildNode, b: ChildNode): number => {

  if (!aDecl?.prop || !bDecl?.prop) return 0;

-  const aShorthand = shorthandFor[aDecl.prop as ShorthandProperties];


remove the previous logic that checked whether the property was a shorthand property of the other (e.g. padding and paddingTop)

the likelihood that we manage to sort two nodes such that a === padding && b in [paddingTop, paddingLeft, paddingBottom, paddingRight] is pretty low, so it's much easier just to lean on the shorthand bucket logic later in the function

dddlr · 2024-10-22T05:19:41Z

packages/css/src/plugins/sort-shorthand-declarations.ts

-  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;


how the bucket system works is that every shorthand property is assigned a number

e.g. padding is 1, padding-block and padding-inline is 2

padding-{left, top, right, bottom} is assigned a number and so defaults to Infinity

when sorting, we ensure that properties with lower numbers come before properties with higher numbers

this also has the side effect of making the behaviour almost identical to runtime mode

Yeah, this changed because mine originally was like "A is a shorthand of B", now it's more like "A is shorthand 5", "B is shorthand 3" and this change LGTM.

This doesn't sort padding-block-start vs. padding-left, but that's still fine, those are "siblings" and that's what we want. I can't think of any scenario where this isn't what we want…maybe like border-block-color and border-top are sorted as siblings now and there could be some odd conflicts…? But I'd say if we have issues with that we have like sort-logical-properties or something—that's separate to this and should rarely come up (but likely does exist).

This doesn't sort padding-block-start vs. padding-left, but that's still fine, those are "siblings" and that's what we want…maybe like border-block-color and border-top are sorted as siblings now and there could be some odd conflicts…?

I can add it to packages/utils/src/shorthand.ts if you want :) just let me know

No, let's not, I don't know what that should even be. It's fine as-is, and if we need to add this later we have a good path forward!

kylorhall-atlassian

Just one comment on an additional test I think, LGTM otherwise.

kylorhall-atlassian · 2024-10-22T05:19:05Z

packages/css/src/plugins/sort-shorthand-declarations.ts

Yeah, I see. I think my original intent was to simply sort padding above paddingLeft for example, and ignore everything else…and then it grew a lot since then.

Now it's more like bucketing, but still sorted, which makes sense to me. Will suggest a few scenarios to test is all.

kylorhall-atlassian · 2024-10-22T05:21:53Z

packages/css/src/plugins/__tests__/sort-shorthand-declarations.test.ts

      .d { border: none; }
      .f { border-block-start-color: transparent; }"
    `);
  });

+  it('sorts a subset of atomic classes taken from a real stylesheet', () => {


Got it, some other thoughts if they're not tested in here should be border > border-top > border-top-color right?

Input:

._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 } @media (…) { ._abcd1234 { border-top-color: red } } @media (…) { ._abcd1234 { border-top: 1px solid } } @media (…) { ._abcd1234 { border: none } }

Output:

._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 } @media (…) { ._abcd1234 { border: none } } @media (…) { ._abcd1234 { border-top: 1px solid } } @media (…) { ._abcd1234 { border-top-color: red } }

The main question I'd have is do we still maintain this pseudo and media query sorting, the rest seems fine to me…

…e declaration

dddlr · 2024-10-22T23:50:47Z

packages/css/src/plugins/sort-shorthand-declarations.ts

-
-    const declarations = node.nodes.map(findDeclaration).filter(Boolean) as Declaration[];
-
-    if (declarations.length === 1) {


I deleted this as it would cause

@media (max-width: 100px) { ._abcd1234 { border-top: 1px solid } }

to be sorted with

._abcd1234 { border: 1px solid }

which overrides the sorting from the sort-at-rules plugin and I don't think is intentional?

If I had to guess, I think I had issues understanding what sorting even looked at in the beginning, eg. there's a lot of different types of styles, I wasn't really sure what we were sorting at times 😅

a whole stylesheet with multiple classes, media, etc in it

an @media query with multiple classes in it

a single .class {property}

A multiple .class {property, webkitproperty} — though I don't know if I even solved for that.

…so yeah, treat any of the code (if I wrote this) around here as possibly wrong!

dddlr · 2024-10-23T00:02:19Z

packages/css/src/plugins/__tests__/sort-shorthand-declarations.test.ts

+      ._abcd1234 { border-top-color: red }
+      ._abcd1234:hover { border-top-color: red }
+      ._abcd1234:active { border-top-color: red }
+      @media (max-width: 150px) { ._abcd1234 { border: none } }


note that the media query sorting is also being run here

dddlr · 2024-10-23T00:11:13Z

packages/css/src/plugins/__tests__/sort-shorthand-declarations.test.ts

        font: 16px;
+        outline-width: 1px;


i don't think this matters, because styles are atomicised in Compiled anyway

kylorhall-atlassian

Love your work, I think this is great and tests cover all my concerns really.

kylorhall-atlassian · 2024-10-23T03:49:34Z

packages/css/src/plugins/sort-atomic-style-sheet.ts

-
-      if (sortShorthandEnabled) {
-        sortShorthandDeclarations(root.nodes);
-      }


Got it, so it was just my bad this was ordered like this.

it was my bad as well 😅 lots of subtle behaviour quirks and bugs i didn't spot even from working on it for hours

kylorhall-atlassian · 2024-10-23T03:51:46Z

packages/css/src/sort.ts

-  }: { sortAtRulesEnabled: boolean | undefined; sortShorthandEnabled: boolean | undefined }
+  }: { sortAtRulesEnabled: boolean | undefined; sortShorthandEnabled: boolean | undefined } = {


Given this is now a required prop, I actually expect this to fail in TS anywhere sort() is called…but yeah I guess we have to find them directly in code if we find them:

-sort(styles); +sort(styles, { sortAtRulesEnabled: true, sortShorthandEnabled: true });

Not the best configuration experience, but we'll survive for now.

typescript says it's optional 🤔

but yeah we would need to turn on sortShorthandEnabled manually everywhere in order to roll it out

(how would we make sure we don't forget? 🤔 should we default to true instead?)

kylorhall-atlassian · 2024-10-23T03:52:00Z

packages/css/src/plugins/__tests__/sort-shorthand-declarations.test.ts

+      ._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 }


Oh is this fixed now? 😮

yeah i fixed it in 18adcbe

whoops not that commit

fixed in this one 8b6edba

Fix shorthand sorting buggy

17e0310

dddlr requested review from JakeLane, pancaspe87, liamqma and zesmith2 as code owners October 22, 2024 05:14

dddlr commented Oct 22, 2024

View reviewed changes

kylorhall-atlassian reviewed Oct 22, 2024

View reviewed changes

Add changeset + fix build error

92755ab

liamqma previously approved these changes Oct 22, 2024

View reviewed changes

dddlr added 3 commits October 23, 2024 10:16

Change patch to minor

e60a63e

Fix bug where media queries are erroneously sorted if they contain on…

18adcbe

…e declaration

Make the sort config optional

60d50a5

dddlr dismissed liamqma’s stale review via 60d50a5 October 22, 2024 23:49

dddlr commented Oct 22, 2024

View reviewed changes

dddlr commented Oct 23, 2024

View reviewed changes

Change pseudo-selector sorting to take priority over shorthand sorting

8b6edba

dddlr commented Oct 23, 2024

View reviewed changes

kylorhall-atlassian approved these changes Oct 23, 2024

View reviewed changes

dddlr merged commit 9b96000 into master Oct 23, 2024
14 of 15 checks passed

dddlr deleted the bug/fix-shorthand-sorting-jank branch October 23, 2024 06:01

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix shorthand sorting buggy #1730

Fix shorthand sorting buggy #1730

dddlr commented Oct 22, 2024 •

edited

Loading

changeset-bot bot commented Oct 22, 2024 •

edited

Loading

netlify bot commented Oct 22, 2024 •

edited

Loading

dddlr Oct 22, 2024

kylorhall-atlassian Oct 22, 2024

dddlr Oct 22, 2024

dddlr Oct 22, 2024

kylorhall-atlassian Oct 22, 2024

dddlr Oct 22, 2024

kylorhall-atlassian Oct 22, 2024

kylorhall-atlassian left a comment

kylorhall-atlassian Oct 22, 2024

kylorhall-atlassian Oct 22, 2024

dddlr Oct 22, 2024 •

edited

Loading

kylorhall-atlassian Oct 23, 2024 •

edited

Loading

dddlr Oct 23, 2024

dddlr Oct 23, 2024

kylorhall-atlassian left a comment

kylorhall-atlassian Oct 23, 2024

dddlr Oct 23, 2024

kylorhall-atlassian Oct 23, 2024 •

edited

Loading

dddlr Oct 23, 2024

dddlr Oct 23, 2024 •

edited

Loading

kylorhall-atlassian Oct 23, 2024

dddlr Oct 23, 2024

dddlr Oct 23, 2024

dddlr Oct 23, 2024

		@@ -37,31 +37,19 @@ const sortNodes = (a: ChildNode, b: ChildNode): number => {

		if (!aDecl?.prop \|\| !bDecl?.prop) return 0;

		const aShorthand = shorthandFor[aDecl.prop as ShorthandProperties];


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

		if (declarations.length === 1) {

		}: { sortAtRulesEnabled: boolean \| undefined; sortShorthandEnabled: boolean \| undefined }
		}: { sortAtRulesEnabled: boolean \| undefined; sortShorthandEnabled: boolean \| undefined } = {

Fix shorthand sorting buggy #1730

Fix shorthand sorting buggy #1730

Conversation

dddlr commented Oct 22, 2024 • edited Loading

What is this change?

Why are we making this change?

How are we making this change?

PR checklist

changeset-bot bot commented Oct 22, 2024 • edited Loading

🦋 Changeset detected

netlify bot commented Oct 22, 2024 • edited Loading

✅ Deploy Preview for compiled-css-in-js canceled.

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kylorhall-atlassian left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dddlr Oct 22, 2024 • edited Loading

Choose a reason for hiding this comment

kylorhall-atlassian Oct 23, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kylorhall-atlassian left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kylorhall-atlassian Oct 23, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dddlr Oct 23, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dddlr commented Oct 22, 2024 •

edited

Loading

changeset-bot bot commented Oct 22, 2024 •

edited

Loading

netlify bot commented Oct 22, 2024 •

edited

Loading

dddlr Oct 22, 2024 •

edited

Loading

kylorhall-atlassian Oct 23, 2024 •

edited

Loading

kylorhall-atlassian Oct 23, 2024 •

edited

Loading

dddlr Oct 23, 2024 •

edited

Loading