Sort shorthand vs. longhand declarations — breaking change behind config #1700

kylorhall-atlassian · 2024-07-22T03:52:24Z

What is this change?

Adds sorting of shorthand vs. longhand to ensure longhand deterministically will override shorthand.

⚠️ This could result in regressions as this will no longer work, so it's behind config, but this may mean this change should never go through (if we added border to this sorting):

const styles = css({ borderTop: '1px solid blue' });
const noBorderStyles = css({ border: 'none' });

export default ({ noBorder }) => <div
  css={[styles, noBorder && noBorderStyles]}
/>

Why are we making this change?

This is because quite commonly this code may be applied in either direction meaning sometimes font-weight: bold is applied and sometimes it's not, depending on the order that the stylesheet was generated in.

const styles = css({ font: '12px / 24px' });
const boldStyles = css({ fontWeight: 'bold' });

export default ({ isBold }) => <div
  css={[styles, isBold && boldStyles]}
/>

Example in a Storybook:

How are we making this change?

Where we already sort atomic styles, we now sort shorthand vs. longhand in that same area.

PR checklist

Validate this works consistently as it's unclear if this sortAtomicStyleSheet iterates over all declarations or just one by one…
Added an applicable test suite
Expand the list of shorthand properties, right now this is just outline and font, there are many more!
Investigate if there's further tests to update
Update the documentation in website/ with this new config

changeset-bot · 2024-07-22T03:52:27Z

🦋 Changeset detected

Latest commit: 4ed15eb

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

This PR includes changesets to release 19 packages

Name	Type
@compiled/babel-plugin-strip-runtime	Minor
@compiled/parcel-optimizer	Minor
@compiled/webpack-loader	Minor
@compiled/react	Minor
@compiled/utils	Minor
@compiled/ssr-app	Minor
@compiled/css	Minor
@compiled/webpack-app	Patch
@compiled/parcel-transformer	Patch
@compiled/parcel-config	Patch
@compiled/parcel-optimizer-test-app	Patch
@compiled/parcel-transformer-test-app	Patch
@compiled/parcel-transformer-test-compress-class-name-app	Patch
@compiled/parcel-transformer-test-custom-resolve-app	Patch
@compiled/parcel-transformer-test-custom-resolver-app	Patch
@compiled/parcel-transformer-test-extract-app	Patch
@compiled/babel-plugin	Minor
@compiled/codemods	Patch
@compiled/eslint-plugin	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-07-22T03:52:38Z

✅ Deploy Preview for compiled-css-in-js ready!

Name	Link
🔨 Latest commit	`4ed15eb`
🔍 Latest deploy log	https://app.netlify.com/sites/compiled-css-in-js/deploys/66f604772c06d90008203475
😎 Deploy Preview	https://deploy-preview-1700--compiled-css-in-js.netlify.app
📱 Preview on mobile	Toggle QR Code... Use your smartphone camera to open QR code link.

To edit notification comments on pull requests, go to your Netlify site configuration.

kylorhall-atlassian · 2024-08-30T07:42:20Z

packages/react/src/runtime/__tests__/style-ssr.test.tsx

        ]}
      </Style>
    );

+    // WARNING: This is wrong, the `focus` border properties are different than without focus, borders would be indeterministic.


If this is required, all "bucket sorting" needs to be drastically rewritten to be nested.

Right now it's flat:

.abc { border: none } .abc:link { border: none } .abc:visited { border: none } /* … */ .abc:active { border: none } @media (/* … */) { .abc { border: none } }

But this doesn't support @media (…) being sorted internally or .abc:focus { … } being sorted against each other.

This would need deeper nested bucketing:

.abc { border: none } .abc{ border-top: none } .abc:link { border: none } .abc:link { border-top: none } @media (/* … */) { .abc { border: none } .abc { border-top: none } .abc:active { border: none } .abc:active { border-top: none } }

And I'm not exactly sure if that's even feasible for @media queries either…

packages/react/src/runtime/shorthand.ts

kylorhall-atlassian · 2024-09-23T23:48:00Z

packages/utils/src/shorthand.ts

+    'border-block-end-style',
+    'border-block-end-width',
+  ],
+  'border-color': [


Example: Should this sort above border-top as well, not just the children? I think this one is still required.

@dddlr I'm not decided enough on what "perfect" looks like for these, so I think it's fine to have some bugs for very rarely used properties…let's circle back to this and call it a gap unless it's obvious.

…in a way, these are "siblings", but to me it feels like there's an explicit order? MDN doesn't really provide anything and I think there's no explicit browser setting—it's a first-defined-basis:

https://developer.mozilla.org/en-US/docs/Web/CSS/border-width

https://developer.mozilla.org/en-US/docs/Web/CSS/border-top

But if this makes sense, in theory I think for all of border-color, border-style, border-width, this should be:

Suggested change

'border-color': [

'border-color': [

'border-top',

'border-block',

'border-inline',

'border-left',

'border-right',

'border-bottom',

ah the order of properties in shorthandFor no longer matters - i should probably just reset that to alphabetical order to be honest ...

shorthandBuckets is now used in packages/css/src/plugins/sort-shorthand-declarations.ts (previously was just in packages/react/src/runtime/sheet.ts) to enforce that border-{color,style,width} comes before border-{top,right,bottom,left}

example https://github.com/atlassian-labs/compiled/pull/1700/files#diff-3b4ae519f265680a755d14f6de289c4d7507a936e4911e95b9b3889fa1ac579bR187

let me know if this aligns with what you have in mind

Got it, the test LGTM, that's what this aShorthandBucket - bShorthandBucket does 👌

packages/utils/src/shorthand.ts

dddlr · 2024-09-25T01:35:27Z

More todos

Add changesets
Ensure sorting in runtime mode (extraction turned off) is adequately tested
Ensure sorting border* properties is deterministic when stylesheet extraction is turned on
~~Benchmark whether adding 14 extra buckets at runtime makes anything slower~~

…rting as well. Concerns: - Runtime sorting does not work properly as we'd also need per-bucket nested sorting where `@media, :focus, border` needs to be sorted. - `shorthandFor` needs 40 more shorthand properties defined…

…uents. This no longer sorts `border-top` vs. `border-block-start` as they are siblings, not a sibling of a constituent…

… by moving shorthandFor to @compiled/react/runtime

…properties are sorted deterministically

packages/react/src/runtime/shorthand.ts

packages/utils/src/shorthand.ts

dddlr · 2024-09-26T05:20:25Z

docs deploy preview

https://deploy-preview-1700--compiled-css-in-js.netlify.app/docs/shorthand

based on my best understanding of how it works, feel free to make any corrections

dddlr · 2024-09-26T05:22:47Z

packages/utils/src/source-maps.ts

this appeared to be unused everywhere and was causing me grief when building

liamqma · 2024-09-26T06:43:35Z

examples/webpack/webpack.config.js

@@ -11,6 +11,8 @@ const classNameCompressionMap = require('./class-name-compression-map.json');

 const extractCSS = process.env.EXTRACT_TO_CSS === 'true';

+console.log('Stylesheet extraction enabled?', extractCSS);


Is this console.log intentional?

oh yeah, i added it as a debugging measure but then decided to keep it as it seemed to be useful

happy to remove it if it's not valuable

kylorhall-atlassian

@dddlr This is my own PR so I can't request changes, but could use a sanity check in a few places and minor suggestions, nothing too blocking.

packages/react/src/runtime/__tests__/style.test.tsx

packages/react/src/runtime/sheet.ts

packages/react/src/runtime/shorthand.ts

kylorhall-atlassian · 2024-09-26T22:45:07Z

packages/utils/src/shorthand.ts

+    'border-block-end-style',
+    'border-block-end-width',
+  ],
+  'border-color': [


@dddlr I'm not decided enough on what "perfect" looks like for these, so I think it's fine to have some bugs for very rarely used properties…let's circle back to this and call it a gap unless it's obvious.

…in a way, these are "siblings", but to me it feels like there's an explicit order? MDN doesn't really provide anything and I think there's no explicit browser setting—it's a first-defined-basis:

https://developer.mozilla.org/en-US/docs/Web/CSS/border-width

https://developer.mozilla.org/en-US/docs/Web/CSS/border-top

But if this makes sense, in theory I think for all of border-color, border-style, border-width, this should be:

Suggested change

'border-color': [

'border-color': [

'border-top',

'border-block',

'border-inline',

'border-left',

'border-right',

'border-bottom',

packages/utils/src/shorthand.ts

kylorhall-atlassian · 2024-09-26T22:49:04Z

packages/utils/src/shorthand.ts

+  transition: 1,
+  'view-timeline': 1,
+} as const satisfies Record<ShorthandProperties, Depths>;
+// ^^ this type lets us enforce that the copy of shorthandBuckets


To question aloud, why do we even have this in this file?

…what if we imported this into the other file, doesn't that just work? I'd expect shorthandFor to be tree shaken, am I wrong?

It's fine, it feels maintainable, just looking at it again, I'm not sure why it's in both places…

ah i made two copies of shorthandBuckets because in @compiled/react/runtime, importing @compiled/utils bloats the bundle size and causes npx size-limit to fail yeah

re: tree shaking, can we assume that tree-shaking always runs in realistic Compiled usage? if so, i can see if i can update the size-limit config to account for tree-shaking maybe

I have no clue 😸

website/packages/docs/src/pages/pkg-babel-plugin-strip-runtime.mdx

website/packages/docs/src/pages/shorthand.mdx

JakeLane · 2024-09-27T01:11:29Z

Can we add linting for this area?

This change makes the behaviour deterministic but it's still confusing to developers. Given the example in the description:

const styles = css({ font: '12px / 24px' });
const boldStyles = css({ fontWeight: 'bold' });

export default ({ isBold }) => <div
  css={[styles, isBold && boldStyles]}
/>

The inverse could happen:

const styles = css({ font: '12px / 24px' });
const boldStyles = css({ fontWeight: 'bold' });

export default ({ isFont }) => <div
  css={[boldStyles, isFont && styles]}
/>

which would have deterministic behaviour opposite to what the developer intended.

Generally, we should make the linting sort the code with autofixers (and warn when not possible) so that the source represents the production code.

dddlr · 2024-09-27T01:31:27Z

Can we add linting for this area?

This change makes the behaviour deterministic but it's still confusing to developers. Given the example in the description:
const styles = css({ font: '12px / 24px' });
const boldStyles = css({ fontWeight: 'bold' });

export default ({ isBold }) => <div
  css={[styles, isBold && boldStyles]}
/>
The inverse could happen:
const styles = css({ font: '12px / 24px' });
const boldStyles = css({ fontWeight: 'bold' });

export default ({ isFont }) => <div
  css={[boldStyles, isFont && styles]}
/>
which would have deterministic behaviour opposite to what the developer intended.

Generally, we should make the linting sort the code with autofixers (and warn when not possible) so that the source represents the production code.

yeah sure

@zesmith2 is working on sorting shorthand and longhand properties within the same css/styled/etc function call, but we can add checking arrays as a follow-up task :)

kylorhall-atlassian requested review from JakeLane, pancaspe87, liamqma and dddlr as code owners July 22, 2024 03:52

kylorhall-atlassian force-pushed the sort-shorthand-longhand-properties branch 2 times, most recently from a3a841d to bc029db Compare August 30, 2024 07:40

kylorhall-atlassian commented Aug 30, 2024

View reviewed changes

dddlr force-pushed the sort-shorthand-longhand-properties branch from 89df158 to c9cae48 Compare September 23, 2024 03:51

kylorhall-atlassian commented Sep 23, 2024

View reviewed changes

packages/react/src/runtime/shorthand.ts Outdated Show resolved Hide resolved

dddlr changed the title ~~EXAMPLE: Sort shorthand vs. longhand declarations — breaking change behind config~~ Sort shorthand vs. longhand declarations — breaking change behind config Sep 23, 2024

kylorhall-atlassian commented Sep 23, 2024

View reviewed changes

dddlr marked this pull request as draft September 25, 2024 05:01

kylorhall-atlassian and others added 6 commits September 25, 2024 16:18

Sort shorthand vs. longhand declarations

3381ae7

Add all known shorthand properties, constituents, and sibling constit…

93f4077

…uents. This no longer sorts `border-top` vs. `border-block-start` as they are siblings, not a sibling of a constituent…

Remove unused buildSourceMap function as it was breaking builds

b710a35

Minimise ./packages/react/dist/browser/runtime/style.js size increase…

f60dc14

… by moving shorthandFor to @compiled/react/runtime

Make shorthand 'sorting' more rigorous, ensuring that border-related …

9ca3547

…properties are sorted deterministically

dddlr force-pushed the sort-shorthand-longhand-properties branch from c948dbd to 9ca3547 Compare September 25, 2024 06:19

Fix outdated bucket number

5c641ab

kylorhall-atlassian commented Sep 25, 2024

View reviewed changes

packages/react/src/runtime/shorthand.ts Outdated Show resolved Hide resolved

kylorhall-atlassian commented Sep 25, 2024

View reviewed changes

packages/utils/src/shorthand.ts Outdated Show resolved Hide resolved

dddlr added 3 commits September 26, 2024 13:51

Add tests, reduce bucket numbers

e97d4da

Add docs

4022ead

Strengthen typing (thanks Kylor)

bf3c5ee

dddlr marked this pull request as ready for review September 26, 2024 04:35

Add changeset

ccd70f7

Enable in Webpack example project, and fix docs

d86315e

dddlr reviewed Sep 26, 2024

View reviewed changes

packages/utils/src/source-maps.ts Outdated

Copy link

Collaborator

dddlr Sep 26, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this appeared to be unused everywhere and was causing me grief when building

liamqma reviewed Sep 26, 2024

View reviewed changes

liamqma previously approved these changes Sep 26, 2024

View reviewed changes

kylorhall-atlassian commented Sep 26, 2024

View reviewed changes

dddlr added 2 commits September 27, 2024 11:00

Address PR feedback

70cedfa

Reset ordering of longhand properties because it no longer matters

4ed15eb

dddlr dismissed liamqma’s stale review via 4ed15eb September 27, 2024 01:03

liamqma approved these changes Sep 29, 2024

View reviewed changes

JakeLane approved these changes Sep 29, 2024

View reviewed changes

dddlr merged commit 9a15e74 into master Sep 30, 2024
13 checks passed

dddlr deleted the sort-shorthand-longhand-properties branch September 30, 2024 00:53

dddlr mentioned this pull request Oct 25, 2024

Add shorthand-property-sorting to recommended ESLint rules #1736

Merged

2 tasks

dddlr mentioned this pull request Nov 28, 2024

[SPIKE] Throw error if shorthand and longhand properties are mixed, or if invalid CSS properties are used #1747

Draft

12 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Sort shorthand vs. longhand declarations — breaking change behind config #1700

Sort shorthand vs. longhand declarations — breaking change behind config #1700

kylorhall-atlassian commented Jul 22, 2024 •

edited by dddlr

Loading

changeset-bot bot commented Jul 22, 2024 •

edited

Loading

netlify bot commented Jul 22, 2024 •

edited

Loading

kylorhall-atlassian Aug 30, 2024 •

edited

Loading

kylorhall-atlassian Sep 23, 2024 •

edited

Loading

kylorhall-atlassian Sep 26, 2024

dddlr Sep 27, 2024

kylorhall-atlassian Sep 27, 2024

dddlr commented Sep 25, 2024 •

edited

Loading

dddlr commented Sep 26, 2024

dddlr Sep 26, 2024

liamqma Sep 26, 2024

dddlr Sep 27, 2024

kylorhall-atlassian left a comment

kylorhall-atlassian Sep 26, 2024

kylorhall-atlassian Sep 26, 2024

dddlr Sep 27, 2024

kylorhall-atlassian Sep 27, 2024

JakeLane commented Sep 27, 2024

dddlr commented Sep 27, 2024

-  'border-color': [
+  'border-color': [
+    'border-top',
+    'border-block',
+    'border-inline',
+    'border-left',
+    'border-right',
+    'border-bottom',

		@@ -11,6 +11,8 @@ const classNameCompressionMap = require('./class-name-compression-map.json');

		const extractCSS = process.env.EXTRACT_TO_CSS === 'true';

		console.log('Stylesheet extraction enabled?', extractCSS);

Sort shorthand vs. longhand declarations — breaking change behind config #1700

Sort shorthand vs. longhand declarations — breaking change behind config #1700

Conversation

kylorhall-atlassian commented Jul 22, 2024 • edited by dddlr Loading

What is this change?

Why are we making this change?

How are we making this change?

PR checklist

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

🦋 Changeset detected

netlify bot commented Jul 22, 2024 • edited Loading

✅ Deploy Preview for compiled-css-in-js ready!

kylorhall-atlassian Aug 30, 2024 • edited Loading

Choose a reason for hiding this comment

kylorhall-atlassian Sep 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

dddlr commented Sep 25, 2024 • edited Loading

dddlr commented Sep 26, 2024

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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

JakeLane commented Sep 27, 2024

dddlr commented Sep 27, 2024

kylorhall-atlassian commented Jul 22, 2024 •

edited by dddlr

Loading

changeset-bot bot commented Jul 22, 2024 •

edited

Loading

netlify bot commented Jul 22, 2024 •

edited

Loading

kylorhall-atlassian Aug 30, 2024 •

edited

Loading

kylorhall-atlassian Sep 23, 2024 •

edited

Loading

dddlr commented Sep 25, 2024 •

edited

Loading