Sort shorthand vs. longhand declarations — breaking change behind config

kylorhall-atlassian commented 3 months ago

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

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

changeset-bot[bot] commented 3 months ago

🦋 Changeset detected

Latest commit: 4ed15ebec15830eb3b67069a9f8f54986f211594

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[bot] commented 3 months ago

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

Name	Link
Latest commit	4ed15ebec15830eb3b67069a9f8f54986f211594
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.

dddlr commented 1 month ago

More todos

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

dddlr commented 1 month ago

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

JakeLane commented 1 month ago

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 commented 1 month ago

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 :)

atlassian-labs / compiled