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/parcel/src/app.jsx
Original file line number Diff line number Diff line change
Expand Up @@ -9,6 +9,7 @@ import '@compiled/react';

import { primary } from './constants';
import Annotated from './ui/annotated';
import CSSMap from './ui/css-map';
import {
CustomFileExtensionStyled,
customFileExtensionCss,
Expand All @@ -29,5 +30,6 @@ export const App = () => (
<React.Suspense fallback={<div>Loading...</div>}>
<AsyncComponent />
</React.Suspense>
<CSSMap variant="danger">CSS Map</CSSMap>
</>
);
12 changes: 12 additions & 0 deletions examples/parcel/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({
Copy link
Collaborator

Choose a reason for hiding this comment

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

nit: Let's simplify this to

import { css } from '@compiled/react'

const styles = css.create({
   ...
});

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Having cssMap simplifies types. What's the reason that css.create is preferred?

danger: {
color: 'red',
},
success: {
color: 'green',
},
});

export default ({ variant, children }) => <div css={css(styles[variant])}>{children}</div>;
Copy link
Collaborator

Choose a reason for hiding this comment

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

Just walking my way through this...

  1. Just using javascript to choose the style
const styles = css.create({
    danger: {
       color: 'red'
    },
    success: {
      color: 'green'
    }
});

const map = {
  danger: styles.danger,
  success: styles.success
}

export default ({variant, children}) => <div css={css(map[variant])} >{children}</div>

Can compile to

const styles = {
    danger: {
       color: '<hash_color_red>'
    },
    success: {
      color: '<hash_color_green>'
    }
});

const map = {
  danger: styles.danger, // == {color: <hash_color_red>}
  success: styles.success // == {color: <hash_color_green>}
}

export default ({variant, children}) => <div css={css(map[variant])} >{children}</div>
  1. Consumer passes in a style itself
export default ({ extendedStyle, children }) => <div css={css(extendedStyle)}>{children}</div>;

can compile to

export default ({ extendedStyle, children }) => <div css={css(extendedStyle)}>{children}</div>;

You're combining those two cases together... to form
3.

const styles = css.match({
    danger: {
       color: 'red'
    },
    success: {
      color: 'green'
    }
});
export default ({ variant, children }) => <div css={css(styles[variant])}>{children}</div>;

seems like a nice shortcut to me, but I'm unsure if we need it to be a different and special construct vs what we could make possible with 1

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Compiled essentially transforms one CSS rule into three pieces.

  1. classNames
  2. sheets
  3. CSS variables (if there are variables cannot be resolved statically). For simplicity, let's ignore it.

We can conditionally apply classNames to DOM element, but sheets cannot. Hence a special construct help identifying variants (will be useful for lints too), and piece sheets together.

const styles = cssMap({
    danger: {
       color: 'red'
    },
    success: {
      color: 'green'
    }
});
export default ({ variant }) => <div css={css(styles[variant])} />;

gets compiled to

<CS>".a {color: red}", ".b {color: green}"</CS> // sheets need to include all possible CSS outcome
...

I think the approach you mentioned from 1 is more leaning on local tranforms (which is roadmapped 🙂). If we have local transforms:

const foo = css({ 
    color: 'red' 
})

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

gets compiled to

const foo = {
  className: ".a",
  sheet: ".a{color:red}",
};

const bar = {
    danger: {
         className: ".a",
         sheet: ".a{color:red}",
    },
    success: {
         className: ".b",
         sheet: ".b{color:green}",
    }
}

We can easily use javascript to choose the style

<div css={css([foo, bar[variant]])} />

gets compiled to

<CS>{[foo, bar]}</CS>
<div className={ax(foo.className, bar[variant].className])} />

If I misunderstood the comment, let's have a quick zoom call to clarify it (maybe)?

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
199 changes: 199 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,199 @@
import type { TransformOptions } from '../../test-utils';
import { transform as transformCode } from '../../test-utils';
import { ErrorMessages } from '../../utils/css-map';

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'
}
});
`;

describe('valid syntax', () => {
it('should evaluate css map when variant is a runtime variable', () => {
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 evaluate css map when variant is statically defined', () => {
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' }])} />;
`
);

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(ErrorMessages.VARIANT_ACCESS);
});

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

<div css={css(styles[isDanger?'danger':'success'])} />;
`);
}).toThrow(ErrorMessages.VARIANT_ACCESS);

expect(() => {
transform(`
${styles}

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

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

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

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

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

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

<div css={css(styles.danger.veryDanger)} />;
`);
}).toThrow(ErrorMessages.NESTED_VARIANT);
});

it('should error out if cssMap does not receive any argument', () => {
expect(() => {
transform(`
import { css, cssMap } from '@compiled/react';

const styles = cssMap();

<div css={css(styles.danger)} />;
`);
}).toThrow(ErrorMessages.NUMBER_OF_ARGUMENT);
});

it('should error out if cssMap receives more than one argument', () => {
expect(() => {
transform(`
import { css, cssMap } from '@compiled/react';

const styles = cssMap({}, {});

<div css={css(styles.danger)} />;
`);
}).toThrow(ErrorMessages.NUMBER_OF_ARGUMENT);
});

it('should error out if cssMap does not receive an object', () => {
expect(() => {
transform(`
import { css, cssMap } from '@compiled/react';

const styles = cssMap('color: red');

<div css={css(styles.danger)} />;
`);
}).toThrow(ErrorMessages.ARGUMENT_TYPE);
});

it('should error out if spread element is used', () => {
expect(() => {
transform(`
import { css, cssMap } from '@compiled/react';

const styles = cssMap({
...base
});

<div css={css(styles.danger)} />;
`);
}).toThrow(ErrorMessages.STATIC_VARIANT_OBJECT);
});

it('should error out if css map object key is dynamic', () => {
expect(() => {
transform(`
import { css, cssMap } from '@compiled/react';

const styles = cssMap({
[variantName]: { color: 'red' }
});

<div css={css(styles.danger)} />;
`);
}).toThrow(ErrorMessages.STATIC_VARIANT_KEY);
});
});
});
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
Loading
Loading