Skip to content

Commit

Permalink
Error on indirect dynamic styles and attempt to statically concatenat…
Browse files Browse the repository at this point in the history 
…e where feasible (#1795)

* Fixes two scenarios found in #1789
- The static syntax of `"8px ".concat("8px")` which is auto-generated by certain Babel targets generates `props.style` outputs when not necessary—we can simplify this to `"8px 8px"` ourselves.
- Mixing "indirect selectors" (eg. `~`, `+`, `|`, `||`) that do not inherit CSS variables with dynamic styles which generates CSS variables and `props.style` will never work in Compiled, this now throws an error.

* Add additional commentary, logic, type protection, and minor cleanup.

* Update website

* Remove snapshot
  • Loading branch information
@kylorhall-atlassian
kylorhall-atlassian authored Feb 17, 2025
1 parent 87a3ad5 commit 3d10a6d
Show file tree
Hide file tree
Showing 5 changed files with 279 additions and 22 deletions.
8 changes: 8 additions & 0 deletions .changeset/fresh-monkeys-laugh.md
View file
Original file line number Diff line number Diff line change
@@ -0,0 +1,8 @@
---
'@compiled/babel-plugin': minor
---

Fixes two scenarios found in https://github.com/atlassian-labs/compiled/issues/1789

- The static syntax of `"8px ".concat("8px")` which is auto-generated by certain Babel targets generates `props.style` outputs when not necessary—we can simplify this to `"8px 8px"` ourselves.
- Mixing "indirect selectors" (eg. `~`, `+`, `|`, `||`) that do not inherit CSS variables with dynamic styles which generates CSS variables and `props.style` will never work in Compiled, this now throws an error.
105 changes: 105 additions & 0 deletions packages/babel-plugin/src/__tests__/index.test.ts
View file
Original file line number Diff line number Diff line change
Expand Up @@ -406,4 +406,109 @@ describe('babel plugin', () => {

expect(actual).toInclude('import { ac, ix, CC, CS } from "@compiled/react/runtime"');
});

it('should not generate dynamic styles with a static concat call', () => {
const code = `
import { css } from '@compiled/react';
const styles = css({
border: "1px solid ".concat('var(--ds-red)'),
margin: "8px ".concat(0),
padding: "8px ".concat('var(--ds-space-050)', ' ', 0),
});
export default () => <div css={styles}><label /></div>;
`;

const actual = transform(code);

// Does not have any `props.style` or CSS variables generated and instead has static values, as expected:
expect(actual).not.toMatch(/:\s*var\(--_/);
expect(actual).not.toMatch(/style=\{\{/);
expect(actual).toIncludeMultiple([
'._1yt4ogpx{padding:8px var(--ds-space-050) 0}',
'._18u0idpf{margin-left:0}',
'._otyrftgi{margin-bottom:8px}',
'._19it6gvt{border:1px solid var(--ds-red)}',
]);
});

describe('indirect selectors with dynamic styles', () => {
it.each([
'"div + button":{color:"var(--_abcd1234)"}',
'"div~label":{color:"var(--_abcd1234)"}',
'"[data-id~=test]~span":{color:"var(--_abcd1234)"}',
'"colgroup||td":{color:"var(--_abcd1234)"}',
'"namespace|a":{color:"var(--_abcd1234)"}',
])('should throw an error for scenario=%p', (scenario) => {
const code = `
import { css } from '@compiled/react';
export default (props) => {
const styles = css({
${scenario}
});
return <div css={styles}><label /></div>
};
`;

expect(() => transform(code)).toThrow(
'unknown file: Found a mix of an indirect selector and a dynamic variable which is unsupported with Compiled. See: https://compiledcssinjs.com/docs/limitations#mixing-dynamic-styles-and-indirect-selectors'
);
});

it('should throw an error with a more complex scenario', () => {
const code = `
import { css } from '@compiled/react';
import { padding, background } from '../utils';
export default (props) => {
const styles = css({
'& + label': {
padding: "8px ".concat(padding),
background,
},
'& div': {
padding: \`8px \${padding}\`,
},
padding: "8px",
});
return <div css={styles}><label /></div>
};
`;

expect(() => transform(code)).toThrow(
'unknown file: Found a mix of an indirect selector and a dynamic variable which is unsupported with Compiled. See: https://compiledcssinjs.com/docs/limitations#mixing-dynamic-styles-and-indirect-selectors'
);
});

it.each([
'"& > label":{color:"var(--_abcd1234)"}',
'"& > label":{color:"red"}',
'"div + button":{color:"red"}',
'"div~label":{color:"red"}',
'"[data-id~=test]~span":{color:"red"}',
'"colgroup||td":{color:"red"}',
'"namespace|a":{color:"red"}',
'"& label":{color:"red"}',
'"& label":{color:"var(--_abcd1234)"}',
'"div [data-id~=test]":{color:"red"}',
'"div [data-id~=test]":{color:"var(--_abcd1234)"}',
'"& + label": { padding: "8px", background: "blue" }, "& div": { padding: "8px var(--_1xumd0e)" }, padding: "8px"',
])('should not error for safe scenario=%p', (scenario) => {
const code = `
import { css } from '@compiled/react';
export default (props) => {
const styles = css({${scenario}});
return <div css={styles}><label /></div>
};
`;

expect(() => transform(code)).not.toThrow();
});
});
});
96 changes: 78 additions & 18 deletions packages/babel-plugin/src/utils/css-builders.ts
View file
Original file line number Diff line number Diff line change
Expand Up @@ -25,7 +25,11 @@ import {
optimizeConditionalStatement,
recomposeTemplateLiteral,
} from './manipulate-template-literal';
import { objectPropertyToString } from './object-property-to-string';
import {
objectPropertyToString,
expressionToString,
canBeStaticallyConcatenated,
} from './object-property-to-string';
import { resolveBinding } from './resolve-binding';
import type {
CSSOutput,
Expand Down Expand Up @@ -370,7 +374,7 @@ const extractConditionalExpression = (node: t.ConditionalExpression, meta: Metad
isCompiledCSSTaggedTemplateExpression(pathNode, meta.state) ||
isCompiledCSSCallExpression(pathNode, meta.state)
) {
cssOutput = buildCss(pathNode, meta);
cssOutput = buildCssInternal(pathNode, meta);
} else if (t.isIdentifier(pathNode)) {
const resolved = resolveBinding(pathNode.name, meta, evaluateExpression);

Expand All @@ -380,7 +384,7 @@ const extractConditionalExpression = (node: t.ConditionalExpression, meta: Metad
(isCompiledCSSTaggedTemplateExpression(resolved.node, resolved.meta.state) ||
isCompiledCSSCallExpression(resolved.node, resolved.meta.state))
) {
cssOutput = buildCss(resolved.node, resolved.meta);
cssOutput = buildCssInternal(resolved.node, resolved.meta);
assertNoImportedCssVariables(pathNode, meta, resolved, cssOutput);
}
} else if (t.isConditionalExpression(pathNode)) {
Expand Down Expand Up @@ -436,7 +440,7 @@ const extractLogicalExpression = (node: t.ArrowFunctionExpression, meta: Metadat

if (t.isExpression(node.body)) {
const { value: propValue, meta: updatedMeta } = evaluateExpression(node.body, meta);
const result = buildCss(propValue, updatedMeta);
const result = buildCssInternal(propValue, updatedMeta);

callbackIfFileIncluded(meta, updatedMeta);

Expand Down Expand Up @@ -465,7 +469,7 @@ const extractKeyframes = (
const selector = `@keyframes ${name}`;
const { css, variables } = toCSSRule(
selector,
buildCss(
buildCssInternal(
t.isCallExpression(expression) ? (expression.arguments as t.Expression[]) : expression.quasi,
{ ...meta, context: 'keyframes', keyframe: name }
)
Expand Down Expand Up @@ -521,6 +525,20 @@ const extractObjectExpression = (node: t.ObjectExpression, meta: Metadata): CSSO
return;
}

if (t.isCallExpression(propValue) && canBeStaticallyConcatenated(propValue)) {
// We're concatenating the string expression on our own, eg.: `color: 'red '.concat('blue')` => `color: 'red blue'`
const value = expressionToString(propValue, updatedMeta);

css.push({
type: 'unconditional',
css: `${isCustomPropertyName(key) ? key : kebabCase(key)}: ${
key === 'content' ? normalizeContentValue(value) : value
};`,
});

return;
}

if (t.isNumericLiteral(propValue)) {
// We've found a numeric literal like: `fontSize: 12`
css.push({
Expand All @@ -541,7 +559,7 @@ const extractObjectExpression = (node: t.ObjectExpression, meta: Metadata): CSSO
if (t.isObjectExpression(propValue) || t.isLogicalExpression(propValue)) {
// We've found either an object like `{}` or a logical expression `isPrimary && {}`.
// We can handle both the same way as they end up resulting in a CSS rule.
const result = toCSSRule(key, buildCss(propValue, updatedMeta));
const result = toCSSRule(key, buildCssInternal(propValue, updatedMeta));
css.push(...result.css);
variables.push(...result.variables);

Expand Down Expand Up @@ -655,7 +673,7 @@ const extractObjectExpression = (node: t.ObjectExpression, meta: Metadata): CSSO
}

const { value: propValue, meta: updatedMeta } = evaluateExpression(prop.argument, meta);
const result = buildCss(propValue, updatedMeta);
const result = buildCssInternal(propValue, updatedMeta);

callbackIfFileIncluded(meta, updatedMeta);

Expand Down Expand Up @@ -745,7 +763,7 @@ function extractMemberExpression(

if (fallbackToEvaluate) {
const { value, meta: updatedMeta } = evaluateExpression(node, meta);
return buildCss(value, updatedMeta);
return buildCssInternal(value, updatedMeta);
}

return undefined;
Expand Down Expand Up @@ -826,7 +844,7 @@ const extractTemplateLiteral = (node: t.TemplateLiteral, meta: Metadata): CSSOut
? nestedTemplateLiteralMeta
: updatedMeta;

const result = buildCss(interpolation, buildCssMeta);
const result = buildCssInternal(interpolation, buildCssMeta);

if (result.css.length) {
// Add previous accumulative CSS first before CSS from expressions
Expand Down Expand Up @@ -890,7 +908,7 @@ const extractTemplateLiteral = (node: t.TemplateLiteral, meta: Metadata): CSSOut
if (t.isArrowFunctionExpression(prop)) {
if (t.isLogicalExpression(prop.body)) {
const { value: propValue, meta: updatedMeta } = evaluateExpression(prop.body, meta);
const result = buildCss(propValue, updatedMeta);
const result = buildCssInternal(propValue, updatedMeta);

callbackIfFileIncluded(meta, updatedMeta);

Expand Down Expand Up @@ -928,7 +946,7 @@ const extractArray = (node: t.ArrayExpression | t.Expression[], meta: Metadata)

const result = t.isConditionalExpression(element)
? extractConditionalExpression(element, meta)
: buildCss(element, meta);
: buildCssInternal(element, meta);

css.push(...result.css);
variables.push(...result.variables);
Expand All @@ -941,12 +959,15 @@ const extractArray = (node: t.ArrayExpression | t.Expression[], meta: Metadata)
};

/**
* Will return a CSS string and CSS variables array from an input node.
* Internal functionality to return a CSS string and CSS variables array from an input node.
*
* @param node Node we're interested in extracting CSS from.
* @param meta {Metadata} Useful metadata that can be used during the transformation
*/
export const buildCss = (node: t.Expression | t.Expression[], meta: Metadata): CSSOutput => {
export const buildCssInternal = (
node: t.Expression | t.Expression[],
meta: Metadata
): CSSOutput => {
if (Array.isArray(node)) {
return extractArray(node, meta);
}
Expand All @@ -956,7 +977,7 @@ export const buildCss = (node: t.Expression | t.Expression[], meta: Metadata): C
}

if (t.isTSAsExpression(node)) {
return buildCss(node.expression, meta);
return buildCssInternal(node.expression, meta);
}

if (t.isTemplateLiteral(node)) {
Expand Down Expand Up @@ -1016,7 +1037,7 @@ export const buildCss = (node: t.Expression | t.Expression[], meta: Metadata): C
);
}

const result = buildCss(resolvedBinding.node, resolvedBinding.meta);
const result = buildCssInternal(resolvedBinding.node, resolvedBinding.meta);

assertNoImportedCssVariables(node, meta, resolvedBinding, result);

Expand All @@ -1029,7 +1050,7 @@ export const buildCss = (node: t.Expression | t.Expression[], meta: Metadata): C

if (t.isLogicalExpression(node)) {
const expression = node.left;
const result = buildCss(node.right, meta);
const result = buildCssInternal(node.right, meta);
const css = result.css.map((item) => {
if (item.type === 'logical') {
return {
Expand Down Expand Up @@ -1062,11 +1083,11 @@ export const buildCss = (node: t.Expression | t.Expression[], meta: Metadata): C
}

if (isCompiledCSSTaggedTemplateExpression(node, meta.state)) {
return buildCss(node.quasi, meta);
return buildCssInternal(node.quasi, meta);
}

if (isCompiledCSSCallExpression(node, meta.state)) {
return buildCss(node.arguments[0] as t.ObjectExpression, meta);
return buildCssInternal(node.arguments[0] as t.ObjectExpression, meta);
}

const areCompiledAPIsEnabled =
Expand All @@ -1082,3 +1103,42 @@ export const buildCss = (node: t.Expression | t.Expression[], meta: Metadata): C
meta.parentPath
);
};

/**
* See "indirect selector" tests in `packages/babel-plugin/src/__tests__/index.test.ts`
* Basically, I want to select anything that's indirect and has a dynamic variable in it.
*
* Think `&>div{color:var(--_color)}` or `&~div{color:var(--_color)}`,
* but not `&:hover{color:var(--_color)}` or `[data-id~="test"]{color:var(--_color)}` (which also has `~` in it)
*
* This isn't perfectly conclusive, but relatively high confidence.
*/
const invalidDynamicIndirectSelectorRegex = /(\+|~|\||\|\|)[^=\{]+\{[^\}]+var\(--_/;
/**
* Will return a CSS string and CSS variables array from an input node.
*
* This includes some top-level error handling for invalid CSS combinations.
*
* @param node Node we're interested in extracting CSS from.
* @param meta {Metadata} Useful metadata that can be used during the transformation
*/
export const buildCss = (node: t.Expression | t.Expression[], meta: Metadata): CSSOutput => {
const output = buildCssInternal(node, meta);

// Check for invalid dynamic selectors
if (
output.css.some(
(item) =>
(item.type === 'unconditional' || item.type === 'conditional') &&
invalidDynamicIndirectSelectorRegex.test(getItemCss(item))
)
) {
throw buildCodeFrameError(
'Found a mix of an indirect selector and a dynamic variable which is unsupported with Compiled. See: https://compiledcssinjs.com/docs/limitations#mixing-dynamic-styles-and-indirect-selectors',
null,
meta.parentPath
);
}

return output;
};
Loading

0 comments on commit 3d10a6d

Please sign in to comment.