Skip to content

Commit

Permalink
Update sorting logic for a lot more shorthands and attempt runtime so…
Browse files Browse the repository at this point in the history
…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…
  • Loading branch information
kylorhall-atlassian committed Aug 30, 2024
1 parent 59c6f2c commit bc029db
Show file tree
Hide file tree
Showing 11 changed files with 596 additions and 36 deletions.
Original file line number Diff line number Diff line change
Expand Up @@ -5,7 +5,7 @@ import { sortAtomicStyleSheet } from '../sort-atomic-style-sheet';

const transform = (css: string, enabled = true) => {
const result = postcss([
sortAtomicStyleSheet({ sortAtRulesEnabled: false, sortShorthandEnabled: enabled }),
sortAtomicStyleSheet({ sortAtRulesEnabled: true, sortShorthandEnabled: enabled }),
]).process(css, {
from: undefined,
});
Expand Down Expand Up @@ -184,6 +184,164 @@ describe('sort shorthand vs. longhand declarations', () => {
`);
});

it("sorts 4 layers deep of shorthands from 'all' to 'border-block-start'", () => {
const actual = transform(outdent`
@media all {
.f {
display: block;
}
.e {
border-block-start-color: transparent;
}
.d {
border-block-start: none;
}
.c {
border-top: none;
}
.b {
border: none;
}
.a {
all: unset;
}
}
.f:focus {
display: block;
}
.e:hover {
border-block-start-color: transparent;
}
.d:active {
border-block-start: none;
}
.c[data-foo='bar'] {
border-top: none;
}
.b[disabled] {
border: none;
}
.a > .external {
all: unset;
}
.f {
display: block;
}
.e {
border-block-start-color: transparent;
}
.d {
border-block-start: none;
}
.c {
border-top: none;
}
.b {
border: none;
}
.a {
all: unset;
}
`);

expect(actual).toMatchInlineSnapshot(`
"
.a {
all: unset;
}
.a > .external {
all: unset;
}
.b[disabled] {
border: none;
}
.c[data-foo='bar'] {
border-top: none;
}
.f {
display: block;
}
.b {
border: none;
}
.c {
border-top: none;
}
.d {
border-block-start: none;
}
.d:active {
border-block-start: none;
}
.e {
border-block-start-color: transparent;
}
.f:focus {
display: block;
}
.e:hover {
border-block-start-color: transparent;
}@media all {
.a {
all: unset;
}
.f {
display: block;
}
.b {
border: none;
}
.c {
border-top: none;
}
.d {
border-block-start: none;
}
.e {
border-block-start-color: transparent;
}
}"
`);
});

it('sorts non-atomic classes inline, but only singular declaration rules against each other', () => {
const actual = transform(outdent`
.e { border-top: none; }
.a {
border-block-start: 1px solid;
border-top: red;
all: reset;
border-block-start-color: transparent;
border: 2px dashed;
}
.f { border-block-start-color: transparent; }
.d { border: none; }
.c { all: unset; }
.b { all: unset; }
`);

// WARNING: This may be wrong as `.a { … }` is not sorted as we expect, it _should_ be 'abcdef' not 'eabcdf'.
// Is this a real scenario—multiple variables in a singular class?
expect(actual).toMatchInlineSnapshot(`
".e { border-top: none; }
.a {
all: reset;
border: 2px dashed;
border-top: red;
border-block-start: 1px solid;
border-block-start-color: transparent;
}
.b { all: unset; }
.c { all: unset; }
.d { border: none; }
.f { border-block-start-color: transparent; }"
`);
});

describe('when disabled', () => {
it('does nothing when crossover detected', () => {
const actual = transformWithoutSorting(outdent`
Expand Down
76 changes: 47 additions & 29 deletions packages/css/src/plugins/sort-shorthand-declarations.ts
Original file line number Diff line number Diff line change
@@ -1,18 +1,49 @@
import type { ChildNode } from 'postcss';

// TODO: Would need a full list from https://developer.mozilla.org/en-US/docs/Web/CSS/Shorthand_properties
// We _kind_ of have some of this in `expand-shorthands`, but only partially.
const shorthandFor: { [key: string]: string[] } = {
font: [
'font-style',
'font-variant',
'font-weight',
'font-stretch',
'font-size',
'line-height',
'font-family',
],
outline: ['outline-color', 'outline-style', 'outline-width'],
import { shorthandFor } 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 => {

Check failure on line 6 in packages/css/src/plugins/sort-shorthand-declarations.ts

View workflow job for this annotation

GitHub Actions / build

Not all code paths return a value.

Check failure on line 6 in packages/css/src/plugins/sort-shorthand-declarations.ts

View workflow job for this annotation

GitHub Actions / build

Not all code paths return a value.
if (nodeIsDeclaration(node)) {
return node;
}

if ('nodes' in node) {
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;
}
};

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;

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

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

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

return 0;
};

export const sortShorthandDeclarations = (nodes: ChildNode[]): void => {
Expand All @@ -25,18 +56,5 @@ export const sortShorthandDeclarations = (nodes: ChildNode[]): void => {
}
});

nodes.sort((a, b) => {
if (a.type !== 'decl' || b.type !== 'decl') return 0;
if (!a.prop || !b.prop) return 0;

if (shorthandFor[a.prop]?.includes(b.prop)) {
return -1;
}

if (shorthandFor[b.prop]?.includes(a.prop)) {
return 1;
}

return 0;
});
nodes.sort(sortNodes);
};
1 change: 1 addition & 0 deletions packages/react/package.json
Original file line number Diff line number Diff line change
Expand Up @@ -72,6 +72,7 @@
"jsx-dev-runtime"
],
"dependencies": {
"@compiled/utils": "^0.11.1",
"csstype": "^3.1.2"
},
"devDependencies": {
Expand Down
9 changes: 8 additions & 1 deletion packages/react/src/runtime/__tests__/style-ssr.test.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -43,12 +43,19 @@ describe('<Style />', () => {
`._g1234567:visited{ color: grey; }`,
`._h1234567:focus-visible{ color: white; }`,
`._i1234567:focus-within{ color: black; }`,
`._j1234567{ border: none; }`,
`._j1234567{ border-top: 1px solid transparent; }`,
`._j1234567{ border-top-color: red; }`,
`._j1234567:focus{ border-top-color: transparent; }`,
`._j1234567:focus{ border-color: blue; }`,
`._j1234567:focus{ border: 1px solid; }`,
]}
</Style>
);

// WARNING: This is wrong, the `focus` border properties are different than without focus, borders would be indeterministic.
expect(result.split('</style>').join('</style>\n')).toMatchInlineSnapshot(`
"<style data-cmpld="true">._c1234567{ display: block; }._d1234567:link{ color: green; }._g1234567:visited{ color: grey; }._i1234567:focus-within{ color: black; }._f1234567:focus{ color: pink; }._h1234567:focus-visible{ color: white; }._a1234567:hover{ color: red; }._b1234567:active{ color: blue; }@media (max-width: 800px){ ._e1234567{ color: yellow; } }</style>
"<style data-cmpld="true">._j1234567{ border: none; }._j1234567{ border-top: 1px solid transparent; }._c1234567{ display: block; }._j1234567{ border-top-color: red; }._d1234567:link{ color: green; }._g1234567:visited{ color: grey; }._i1234567:focus-within{ color: black; }._f1234567:focus{ color: pink; }._j1234567:focus{ border-top-color: transparent; }._j1234567:focus{ border-color: blue; }._j1234567:focus{ border: 1px solid; }._h1234567:focus-visible{ color: white; }._a1234567:hover{ color: red; }._b1234567:active{ color: blue; }@media (max-width: 800px){ ._e1234567{ color: yellow; } }</style>
"
`);
});
Expand Down
22 changes: 20 additions & 2 deletions packages/react/src/runtime/sheet.ts
Original file line number Diff line number Diff line change
@@ -1,3 +1,5 @@
import { getShorthandDepth } from '@compiled/utils';

import { isCacheDisabled } from './cache';
import type { Bucket, StyleSheetOpts } from './types';

Expand All @@ -6,6 +8,11 @@ import type { Bucket, StyleSheetOpts } from './types';
* If changes are needed make sure that it aligns with the definition in `sort-at-rule-pseudos.tsx`.
*/
export const styleBucketOrdering: Bucket[] = [
// shorthand properties
's-root',
's-1',
's-2',
's-3',
// catch-all
'',
// link
Expand Down Expand Up @@ -114,15 +121,26 @@ export const getStyleBucketName = (sheet: string): Bucket => {
return 'm';
}

const firstBracket = sheet.indexOf('{');

/**
* We assume that classname will always be 9 character long,
* using this the 10th character could be a pseudo declaration.
* using this the 10th characters could be a pseudo declaration.
*/
if (sheet.charCodeAt(10) === 58 /* ":" */) {
// We send through a subset of the string instead of the full pseudo name.
// For example `"focus-visible"` name would instead of `"us-visible"`.
// Return a mapped pseudo else the default catch all bucket.
return pseudosMap[sheet.slice(14, sheet.indexOf('{'))] || '';
const mapped = pseudosMap[sheet.slice(14, firstBracket)];
if (mapped) return mapped;
}

const property = sheet.slice(firstBracket + 1, sheet.indexOf(':', firstBracket)).trim();

const shorthandDepth = getShorthandDepth(property);
if (shorthandDepth) {
// NOTE: This doesn't actually work fully because there's various _layers_ of shorthands — up to 4 deep, I believe…
return `s-${shorthandDepth}` as const;
}

// Return default catch all bucket
Expand Down
5 changes: 5 additions & 0 deletions packages/react/src/runtime/types.ts
Original file line number Diff line number Diff line change
Expand Up @@ -11,6 +11,11 @@ export interface StyleSheetOpts {
* Buckets under which we will group our stylesheets
*/
export type Bucket =
// shorthand properties
| 's-root'
| 's-1'
| 's-2'
| 's-3'
// catch-all
| ''
// link
Expand Down
2 changes: 1 addition & 1 deletion packages/react/tsconfig.browser.json
Original file line number Diff line number Diff line change
Expand Up @@ -12,5 +12,5 @@
],
"tsBuildInfoFile": "tsconfig.browser.tsbuildinfo"
},
"references": [{ "path": "../benchmark" }, { "path": "../jest" }]
"references": [{ "path": "../benchmark" }, { "path": "../jest" }, { "path": "../utils" }]
}
2 changes: 1 addition & 1 deletion packages/react/tsconfig.cjs.json
Original file line number Diff line number Diff line change
Expand Up @@ -5,5 +5,5 @@
"outDir": "dist/cjs",
"tsBuildInfoFile": "tsconfig.cjs.tsbuildinfo"
},
"references": [{ "path": "../benchmark" }, { "path": "../jest" }]
"references": [{ "path": "../benchmark" }, { "path": "../jest" }, { "path": "../utils" }]
}
2 changes: 1 addition & 1 deletion packages/react/tsconfig.json
Original file line number Diff line number Diff line change
Expand Up @@ -4,5 +4,5 @@
"outDir": "dist/esm",
"tsBuildInfoFile": "tsconfig.esm.tsbuildinfo"
},
"references": [{ "path": "../benchmark" }, { "path": "../jest" }]
"references": [{ "path": "../benchmark" }, { "path": "../jest" }, { "path": "../utils" }]
}
1 change: 1 addition & 0 deletions packages/utils/src/index.ts
Original file line number Diff line number Diff line change
Expand Up @@ -8,3 +8,4 @@ export { createError } from './error';
export { preserveLeadingComments } from './preserve-leading-comments';
export { INCREASE_SPECIFICITY_SELECTOR } from './increase-specificity';
export { DEFAULT_PARSER_BABEL_PLUGINS } from './default-parser-babel-plugins';
export { shorthandFor, getShorthandDepth } from './shorthand';
Loading

0 comments on commit bc029db

Please sign in to comment.