Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add CSS Map #1492

Closed
wants to merge 13 commits into from
2 changes: 2 additions & 0 deletions examples/webpack/src/app.jsx
Original file line number Diff line number Diff line change
Expand Up @@ -4,6 +4,7 @@ import { Suspense, lazy } from 'react';

import { primary } from './common/constants';
import Annotated from './ui/annotated';
import CSSMap from './ui/css-map';
import {
CustomFileExtensionStyled,
customFileExtensionCss,
Expand All @@ -23,5 +24,6 @@ export const App = () => (
<CustomFileExtensionStyled>Custom File Extension Styled</CustomFileExtensionStyled>
<div css={customFileExtensionCss}>Custom File Extension CSS</div>
<Annotated />
<CSSMap variant="danger">CSS Map</CSSMap>
liamqma marked this conversation as resolved.
Show resolved Hide resolved
</>
);
12 changes: 12 additions & 0 deletions examples/webpack/src/ui/css-map.jsx
Original file line number Diff line number Diff line change
@@ -0,0 +1,12 @@
import { css, cssMap } from '@compiled/react';

const styles = cssMap({
danger: {
color: 'red',
},
success: {
color: 'green',
},
});

export default ({ variant, children }) => <div css={css(styles[variant])}>{children}</div>;
6 changes: 4 additions & 2 deletions packages/babel-plugin/src/babel-plugin.ts
Original file line number Diff line number Diff line change
Expand Up @@ -20,6 +20,7 @@ import {
isCompiledKeyframesTaggedTemplateExpression,
isCompiledStyledCallExpression,
isCompiledStyledTaggedTemplateExpression,
isCompiledCSSMapCallExpression,
} from './utils/is-compiled';
import { normalizePropsUsage } from './utils/normalize-props-usage';

Expand Down Expand Up @@ -150,7 +151,7 @@ export default declare<State>((api) => {
return;
}

(['styled', 'ClassNames', 'css', 'keyframes'] as const).forEach((apiName) => {
(['styled', 'ClassNames', 'css', 'keyframes', 'cssMap'] as const).forEach((apiName) => {
if (
state.compiledImports &&
t.isIdentifier(specifier.node?.imported) &&
Expand Down Expand Up @@ -185,7 +186,8 @@ export default declare<State>((api) => {
isCompiledCSSTaggedTemplateExpression(path.node, state) ||
isCompiledKeyframesTaggedTemplateExpression(path.node, state) ||
isCompiledCSSCallExpression(path.node, state) ||
isCompiledKeyframesCallExpression(path.node, state);
isCompiledKeyframesCallExpression(path.node, state) ||
isCompiledCSSMapCallExpression(path.node, state);

if (isCompiledUtil) {
state.pathsToCleanup.push({ path, action: 'replace' });
Expand Down
142 changes: 142 additions & 0 deletions packages/babel-plugin/src/css-prop/__tests__/css-map.test.ts
Copy link
Collaborator

Choose a reason for hiding this comment

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

Nit: moving css-map.test.ts to packages/babel-plugin/src/utils/__tests__/ perhaps

(unless there's a reason to have it in the css-prop directory instead)

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I think it makes sense to be in packages/babel-plugin/src/css-prop, because they aren't unit tests against the functions of css-map.ts. These tests rely on many things from css-prop.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Ah no worries 👍

Original file line number Diff line number Diff line change
@@ -0,0 +1,142 @@
import type { TransformOptions } from '../../test-utils';
import { transform as transformCode } from '../../test-utils';

describe('css map behaviour', () => {
beforeAll(() => {
process.env.AUTOPREFIXER = 'off';
});

afterAll(() => {
delete process.env.AUTOPREFIXER;
});

const transform = (code: string, opts: TransformOptions = {}) =>
transformCode(code, { pretty: false, ...opts });

const styles = `
import { css, cssMap } from '@compiled/react';

const styles = cssMap({
danger: {
color: 'red',
backgroundColor: 'red'
},
success: {
color: 'green',
backgroundColor: 'green'
}
});
`;

const defaultErrorMessage = 'Using a CSS Map in this manner is incorrect.';
const nestedErrorMessage = 'You cannot access a nested CSS Map';

describe('valid syntax', () => {
it.only('should evulate css map when variant is a runtime variable', () => {
liamqma marked this conversation as resolved.
Show resolved Hide resolved
const actual = transform(`
${styles}

<div css={css(styles[variant])} />;
`);

expect(actual).toInclude(
'<div className={ax([variant==="danger"&&"_syaz5scu _bfhk5scu",variant==="success"&&"_syazbf54 _bfhkbf54"])}/>'
);
});

it('should evulate css map when variant is statically defined', () => {
liamqma marked this conversation as resolved.
Show resolved Hide resolved
const actual = transform(`
${styles}

<div css={css(styles.success)} />;
<div css={css(styles['danger'])} />;
`);

expect(actual).toInclude(
'<div className={ax(["success"==="danger"&&"_syaz5scu _bfhk5scu","success"==="success"&&"_syazbf54 _bfhkbf54"])}/>'
Copy link
Collaborator

Choose a reason for hiding this comment

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

Something weird is happening here. "success"==="danger" can be statically evaluated to false. Where does this comparison even come from though? I've got a feeling we're doing more work than needed with the css() call here, risks having spooky action.

If I do this,

<div css={css(styles.a)} />
<div css={css(styles.b)} />
<div css={css(styles.c)} />
<div css={css(styles.d)} />
<div css={css(styles.e)} />

I shouldn't end up with each case considering all of a,b,c,d and e. I was explicit in each

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Yea, you are right.
But I think the selected variant is runtime variable at most of cases. Statically access to a variant is "by-product". Users probably should use

const a = {
   color: red
}
<div css={css(a)} />

instead of

const styles = {
   a: {
       color: red
   }
}
<div css={css(styles.a)} />

Also, bundler drops the dead code ('apple' === 'orange') && '.aaaabbbb'

);
expect(actual).toInclude(
'<div className={ax([\'danger\'==="danger"&&"_syaz5scu _bfhk5scu",\'danger\'==="success"&&"_syazbf54 _bfhkbf54"])}/>'
Copy link
Collaborator

Choose a reason for hiding this comment

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

Quoting inconsistency here

);
});

it('should combine CSS Map with other styles', () => {
const actual = transform(
`
${styles}

<div css={css([styles[variant], { color: 'blue' }])} />;
`,
{ pretty: true }
);

console.log(actual);

expect(actual).toInclude(
'<div className={ax([variant==="danger"&&"_syaz5scu _bfhk5scu",variant==="success"&&"_syazbf54 _bfhkbf54","_syaz13q2"])}/>'
liamqma marked this conversation as resolved.
Show resolved Hide resolved
);
});
});

describe('invalid syntax', () => {
it('does not support TemplateLiteral as object property', () => {
expect(() => {
transform(`
${styles}

<div css={css(styles[\`danger\`])} />;
`);
}).toThrow(defaultErrorMessage);
});

it('does not support Expression as object property', () => {
expect(() => {
transform(`
${styles}

<div css={css(styles['dang' + 'er'])} />;
`);
}).toThrow(defaultErrorMessage);
});

it('does not support BinaryExpression as object property', () => {
expect(() => {
transform(`
${styles}

<div css={css(styles['dang' + 'er'])} />;
`);
}).toThrow(defaultErrorMessage);
});

it('does not support MemberExpression as object property', () => {
expect(() => {
transform(`
${styles}

<div css={css(styles[props.variant])} />;
`);
}).toThrow(defaultErrorMessage);
});

it('does not support CallExpression as object property', () => {
expect(() => {
transform(`
${styles}

<div css={css(styles()[variant])} />;
`);
}).toThrow(defaultErrorMessage);
});

it('does not support nesting', () => {
expect(() => {
transform(`
${styles}

<div css={css(styles.danger.veryDanger)} />;
`);
}).toThrow(nestedErrorMessage);
});
});
});
1 change: 1 addition & 0 deletions packages/babel-plugin/src/types.ts
Original file line number Diff line number Diff line change
Expand Up @@ -98,6 +98,7 @@ export interface State extends PluginPass {
css?: string;
keyframes?: string;
styled?: string;
cssMap?: string;
};

importedCompiledImports?: {
Expand Down
192 changes: 192 additions & 0 deletions packages/babel-plugin/src/utils/css-map.ts
Original file line number Diff line number Diff line change
@@ -0,0 +1,192 @@
import * as t from '@babel/types';

import type { Metadata } from '../types';

import { buildCodeFrameError } from './ast';
import { createResultPair } from './create-result-pair';
import { evaluateIdentifier } from './traverse-expression/traverse-member-expression/traverse-access-path/resolve-expression/identifier';
import type { EvaluateExpression } from './types';

const createErrorMessage = (message?: string): string => {
return `
${
message || 'Using a CSS Map in this manner is incorrect.'
} To correctly implement a CSS Map, follow the syntax below:

\`\`\`
import { css, cssMap } from '@compiled/react';
const borderStyleMap = cssMap({
none: { borderStyle: 'none' },
solid: { borderStyle: 'solid' },
});
const Component = ({ borderStyle }) => <div css={css(borderStyleMap[borderStyle])} />
\`\`\`
`;
};

/**
* Retrieves the leftmost identity from a given expression.
*
* For example:
* Given a member expression "colors.primary.500", the function will return "colors".
*
* @param expression The expression to be evaluated.
* @returns {string} The leftmost identity in the expression.
*/
const findBindingIdentifier = (
expression: t.Expression | t.V8IntrinsicIdentifier
): t.Identifier | undefined => {
if (t.isIdentifier(expression)) {
return expression;
} else if (t.isCallExpression(expression)) {
return findBindingIdentifier(expression.callee);
} else if (t.isMemberExpression(expression)) {
return findBindingIdentifier(expression.object);
}

return undefined;
};

/**
* Retrieves the CSS Map related information from a given expression.
*
* @param expression The expression to be evaluated.
* @param meta {Metadata} Useful metadata that can be used during the transformation
* @param evaluateExpression {EvaluateExpression} Function that evaluates an expression
*/
const getCSSmap = (
liamqma marked this conversation as resolved.
Show resolved Hide resolved
expression: t.Expression,
meta: Metadata,
evaluateExpression: EvaluateExpression
):
| {
Copy link
Collaborator

Choose a reason for hiding this comment

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

Nit: not sure what this extra pipe character does here

Copy link
Collaborator

Choose a reason for hiding this comment

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

I wonder why the eslint plugin doesn't catch this 🤔

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

believe it or not. The extra pipe is enforced by prettier. 🤷‍♂️

Copy link
Collaborator

Choose a reason for hiding this comment

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

TIL

value: t.ObjectExpression;
meta: Metadata;
property: t.Identifier | t.StringLiteral;
computed: boolean;
}
| undefined => {
// Bail out early if cssMap callExpression doesn't exist in the file
if (!meta.state.compiledImports?.cssMap) return undefined;

// We only care about member expressions. e.g. variants[variant]
if (!t.isMemberExpression(expression)) return undefined;

const bindingIdentifier = findBindingIdentifier(expression.object);

if (!bindingIdentifier) return undefined;

// Evaluate the binding identifier to get the value of the CSS Map
const { value, meta: updatedMeta } = evaluateIdentifier(
bindingIdentifier,
meta,
evaluateExpression
);

// Ensure cssMap is used in a correct format.
if (
t.isCallExpression(value) &&
t.isIdentifier(value.callee) &&
value.callee.name === meta.state.compiledImports?.cssMap &&
value.arguments.length > 0 &&
liamqma marked this conversation as resolved.
Show resolved Hide resolved
t.isObjectExpression(value.arguments[0])
) {
// It's CSS Map! We now need to check if the use of the CSS Map is correct.
if (t.isCallExpression(expression.object)) {
throw buildCodeFrameError(createErrorMessage(), expression, updatedMeta.parentPath);
liamqma marked this conversation as resolved.
Show resolved Hide resolved
}

if (t.isMemberExpression(expression.object)) {
throw buildCodeFrameError(
createErrorMessage('You cannot access a nested CSS Map.'),
expression,
updatedMeta.parentPath
);
}

if (!t.isIdentifier(expression.property) && !t.isStringLiteral(expression.property)) {
throw buildCodeFrameError(createErrorMessage(), expression, updatedMeta.parentPath);
}

return {
value: value.arguments[0],
property: expression.property,
computed: expression.computed,
meta: updatedMeta,
};
}

// It's not a CSS Map, let other code handle it
return undefined;
};

export const isCSSMap = (
expression: t.Expression,
meta: Metadata,
evaluateExpression: EvaluateExpression
): boolean => {
return getCSSmap(expression, meta, evaluateExpression) !== undefined;
};

/**
* Transform expression that uses a CSS Map into an array of logical expressions.
liamqma marked this conversation as resolved.
Show resolved Hide resolved
* For example:
* ```js
* const borderStyleMap = cssMap({
* none: { borderStyle: 'none' },
* solid: { borderStyle: 'solid' },
* });
* const Component = ({ borderStyle }) => <div css={css(borderStyleMap[borderStyle])} />
* ```
* gets transformed into:
* ```js
* const Component = ({ borderStyle }) => <div css={css([
* borderStyle === 'none' && { borderStyle: 'none' },
* borderStyle === 'solid' && { borderStyle: 'solid'}
* ])} />
Copy link
Collaborator

Choose a reason for hiding this comment

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

This looks like it's outputting an intermediate form. It's preferable for us to consolidate things into a 1-pass compilation rather than having multiple passes, because this is lossy and prevents us from understanding the original context (which is useful for optimisations like dropping the "success"==="danger" case. What's inhibiting us from doing this in the same pass as the css-prop -> className transform?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

um...good point. Let me look into it more.

Copy link
Collaborator Author

@liamqma liamqma Aug 21, 2023

Choose a reason for hiding this comment

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

consolidate things into a 1-pass compilation rather than having multiple passes

It's been an architectural thing.

  • Compiled firstly evaluates user input, and compiles it to expression. e.g. foo ? 'color: red': 'color: green'
  • At later stage, then compiles it to classNames and sheets

I think we can change this two-pass compilation into 1-pass by using local transform, as mentioned from another comment. I will make sure that I will spike this 1-pass compilation before PR is merged.

* ```
* Throw an error if a valid CSS Map is not provided.
*
* @param expression Expression we want to evulate.
* @param meta {Metadata} Useful metadata that can be used during the transformation
*/
export const evaluateCSSMap = (
expression: t.Expression,
meta: Metadata,
evaluateExpression: EvaluateExpression
): ReturnType<typeof createResultPair> => {
const result = getCSSmap(expression, meta, evaluateExpression);

// It should never happen because `isCSSMap` should have been checked already.
if (!result) throw buildCodeFrameError(createErrorMessage(), expression, meta.parentPath);
liamqma marked this conversation as resolved.
Show resolved Hide resolved

const { value: objectExpression, property: objectProperty, computed, meta: updatedMeta } = result;

return createResultPair(
t.arrayExpression(
objectExpression.properties.map((property) => {
if (
!t.isObjectProperty(property) ||
!t.isIdentifier(property.key) ||
!t.isExpression(property.value)
)
throw buildCodeFrameError(createErrorMessage(), expression, updatedMeta.parentPath);

return t.logicalExpression(
'&&',
t.binaryExpression(
'===',
t.isStringLiteral(objectProperty)
Copy link
Collaborator

Choose a reason for hiding this comment

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

I'm having a bit of trouble understanding this nested ternary expression - could it be made more readable somehow?

e.g. use of parentheses

A ? (B ? C : D) : E

(A ? B : C) ? D : E

Copy link
Collaborator

Choose a reason for hiding this comment

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

Or perhaps by extracting this ternary expression into a separate variable:

const someVariable = A ? B : (C ? D : E);
// ...
return ...
    t.binaryExpression(
        '===',
        someVariable,
        t.stringLiteral(property.key.name)
    )
    ...

? objectProperty
: computed
? objectProperty
: t.stringLiteral(objectProperty.name),
t.stringLiteral(property.key.name)
),
property.value
);
})
),
updatedMeta
);
};
Loading
Loading