Skip to content

Commit

Permalink
Fix @compiled/babel-plugin to not silently skip cssMap() extracti…
Browse files Browse the repository at this point in the history
…on in a few common edge-cases. (#1773)

* Fix `@compiled/babel-plugin` to not require `cssMap()` to be called prior to use.

Example: this code failed before for no reason other than the fact that our `state.cssMap` was generated _after_ `JSXElement` and `JSXOpeningElement` were ran.

```tsx
import { cssMap } from "@compiled/react";
export default () => <div css={styles.root} />;
const styles = cssMap({ root: { padding: 8 } });
```

* Attempt to resolve 'cssMap' calls when we hit a MemberExpression that does not have this defined.

* Throw an error when compiling a `cssMap` object where we expect a `css` or nested `cssMap` object. (#1774)

* Throw an error when compiling a `cssMap` object where we expect a `css` or nested `cssMap` object.

Fixes #1642

Example of code that silently fails today, using `styles` directly:
```tsx
import { cssMap } from '@compiled/react';
const styles = cssMap({ root: { padding: 8 } });
export default () => <div css={styles} />;
```

What we expect to see instead, using `styles.root` instead:
```tsx
import { cssMap } from '@compiled/react';
const styles = cssMap({ root: { padding: 8 } });
export default () => <div css={styles.root} />;
```

* Handle the scenario where we use `css={styles}` before `cssMap()` is called

 Also, just update the tests which muddy `cssMap` vs. `css` scope — technically a missed scenario, but one that we shouldn't hit as this throws an error if compiled in one go.
  • Loading branch information
kylorhall-atlassian authored Dec 31, 2024
1 parent b000220 commit 34e5339
Show file tree
Hide file tree
Showing 8 changed files with 155 additions and 12 deletions.
13 changes: 13 additions & 0 deletions .changeset/dirty-carrots-sit.md
Original file line number Diff line number Diff line change
@@ -0,0 +1,13 @@
---
'@compiled/babel-plugin': minor
---

Fix `@compiled/babel-plugin` to not require `cssMap()` to be called prior to use.

Example, this failed before for no reason other than the fact that our `state.cssMap` was generated _after_ `JSXElement` and `JSXOpeningElement` were ran.

```tsx
import { cssMap } from '@compiled/react';
export default () => <div css={styles.root} />;
const styles = cssMap({ root: { padding: 8 } });
```
21 changes: 21 additions & 0 deletions .changeset/silent-geese-wonder.md
Original file line number Diff line number Diff line change
@@ -0,0 +1,21 @@
---
'@compiled/babel-plugin': minor
---

Throw an error when compiling a `cssMap` object where we expect a `css` or nested `cssMap` object.

Example of code that silently fails today, using `styles` directly:

```tsx
import { cssMap } from '@compiled/react';
const styles = cssMap({ root: { padding: 8 } });
export default () => <div css={styles} />;
```

What we expect to see instead, using `styles.root` instead:

```tsx
import { cssMap } from '@compiled/react';
const styles = cssMap({ root: { padding: 8 } });
export default () => <div css={styles.root} />;
```
4 changes: 3 additions & 1 deletion packages/babel-plugin/src/babel-plugin.ts
Original file line number Diff line number Diff line change
Expand Up @@ -76,6 +76,7 @@ export default declare<State>((api) => {

this.sheets = {};
this.cssMap = {};
this.ignoreMemberExpressions = {};
let cache: Cache;

if (this.opts.cache === true) {
Expand Down Expand Up @@ -239,7 +240,6 @@ export default declare<State>((api) => {
},
ImportDeclaration(path, state) {
const userLandModule = path.node.source.value;

const isCompiledModule = this.importSources.some((compiledModuleOrigin) => {
if (compiledModuleOrigin === userLandModule) {
return true;
Expand Down Expand Up @@ -282,6 +282,7 @@ export default declare<State>((api) => {
const apiArray = state.compiledImports[apiName] || [];
apiArray.push(specifier.node.local.name);
state.compiledImports[apiName] = apiArray;

specifier.remove();
}
});
Expand Down Expand Up @@ -311,6 +312,7 @@ Reasons this might happen:
path.parentPath
);
}

if (isCompiledCSSMapCallExpression(path.node, state)) {
visitCssMapPath(path, { context: 'root', state, parentPath: path });
return;
Expand Down
43 changes: 43 additions & 0 deletions packages/babel-plugin/src/css-map/__tests__/index.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -36,6 +36,25 @@ describe('css map basic functionality', () => {
]);
});

it('should transform css map even when the styles are defined below the component', () => {
const actual = transform(`
import { cssMap } from '@compiled/react';
const Component = () => <div>
<span css={styles.danger} />
<span css={styles.success} />
</div>
const styles = cssMap(${styles});
`);

expect(actual).toIncludeMultiple([
'const styles={danger:"_syaz5scu _bfhk5scu",success:"_syazbf54 _bfhkbf54"};',
'<span className={ax([styles.danger])}/>',
'<span className={ax([styles.success])}/>',
]);
});

it('should transform css map even with an empty object', () => {
const actual = transform(`
import { css, cssMap } from '@compiled/react';
Expand Down Expand Up @@ -88,6 +107,30 @@ describe('css map basic functionality', () => {
]);
});

it('should error out if the root cssMap object is being directly called', () => {
expect(() => {
transform(`
import { cssMap } from '@compiled/react';
const styles = cssMap({ root: { color: 'red' } });
// Eg. we expect 'styles.root' here instead of 'styles'
<div css={styles} />
`);
}).toThrow(ErrorMessages.USE_VARIANT_OF_CSS_MAP);

expect(() => {
transform(`
import { cssMap } from '@compiled/react';
// Eg. we expect 'styles.root' here instead of 'styles'
<div css={styles} />
const styles = cssMap({ root: { color: 'red' } });
`);
}).toThrow(ErrorMessages.USE_VARIANT_OF_CSS_MAP);
});

it('should error out if variants are not defined at the top-most scope of the module.', () => {
expect(() => {
transform(`
Expand Down
5 changes: 5 additions & 0 deletions packages/babel-plugin/src/types.ts
Original file line number Diff line number Diff line change
Expand Up @@ -194,6 +194,11 @@ export interface State extends PluginPass {
*/
cssMap: Record<string, string[]>;

/**
* Holdings a record of member expression names to ignore
*/
ignoreMemberExpressions: Record<string, true>;

/**
* A custom resolver used to statically evaluate import declarations
*/
Expand Down
68 changes: 63 additions & 5 deletions packages/babel-plugin/src/utils/css-builders.ts
Original file line number Diff line number Diff line change
@@ -1,15 +1,19 @@
import generate from '@babel/generator';
import type { NodePath } from '@babel/traverse';
import * as t from '@babel/types';
import { addUnitIfNeeded, cssAffixInterpolation } from '@compiled/css';
import { hash, kebabCase } from '@compiled/utils';

import { visitCssMapPath } from '../css-map';
import type { Metadata } from '../types';

import { buildCodeFrameError } from './ast';
import { CONDITIONAL_PATHS } from './constants';
import { createErrorMessage, ErrorMessages } from './css-map';
import { evaluateExpression } from './evaluate-expression';
import {
isCompiledCSSCallExpression,
isCompiledCSSMapCallExpression,
isCompiledCSSTaggedTemplateExpression,
isCompiledKeyframesCallExpression,
isCompiledKeyframesTaggedTemplateExpression,
Expand Down Expand Up @@ -665,6 +669,45 @@ const extractObjectExpression = (node: t.ObjectExpression, meta: Metadata): CSSO
return { css: mergeSubsequentUnconditionalCssItems(css), variables };
};

/**
* If we don't yet have a `meta.state.cssMap[node.name]` built yet, try to build and cache it, eg. in this scenario:
* ```tsx
* const Component = () => <div css={styles.root} />
* const styles = cssMap({ root: { padding: 0 } });
* ```
*
* If we don't find this is a `cssMap()` call, we put it into `ignoreMemberExpressions` to ignore on future runs.
*
* @returns {Boolean} Whether the cache was generated
*/
const generateCacheForCSSMap = (node: t.Identifier, meta: Metadata): void => {
if (meta.state.cssMap[node.name] || meta.state.ignoreMemberExpressions[node.name]) {
return;
}

const resolved = resolveBinding(node.name, meta, evaluateExpression);
if (resolved && isCompiledCSSMapCallExpression(resolved.node, meta.state)) {
let resolvedCallPath = resolved.path.get('init');
if (Array.isArray(resolvedCallPath)) {
resolvedCallPath = resolvedCallPath[0];
}

if (t.isCallExpression(resolvedCallPath.node)) {
// This visits the cssMap path and caches the styles
visitCssMapPath(resolvedCallPath as NodePath<t.CallExpression>, {
context: 'root',
state: meta.state,
parentPath: resolved.path,
});
}
}

if (!meta.state.cssMap[node.name]) {
// If this cannot be found, it's likely not a `cssMap` identifier and we shouldn't parse it again on future runs…
meta.state.ignoreMemberExpressions[node.name] = true;
}
};

/**
* Extracts CSS data from a member expression node (eg. `styles.primary`)
*
Expand All @@ -688,11 +731,16 @@ function extractMemberExpression(
fallbackToEvaluate = true
): CSSOutput | undefined {
const bindingIdentifier = findBindingIdentifier(node);
if (bindingIdentifier && meta.state.cssMap[bindingIdentifier.name]) {
return {
css: [{ type: 'map', expression: node, name: bindingIdentifier.name, css: '' }],
variables: [],
};

if (bindingIdentifier) {
// In some cases, the `state.cssMap` is not warmed yet, so run it:
generateCacheForCSSMap(bindingIdentifier, meta);
if (meta.state.cssMap[bindingIdentifier.name]) {
return {
css: [{ type: 'map', expression: node, name: bindingIdentifier.name, css: '' }],
variables: [],
};
}
}

if (fallbackToEvaluate) {
Expand Down Expand Up @@ -956,6 +1004,16 @@ export const buildCss = (node: t.Expression | t.Expression[], meta: Metadata): C
);
}

// In some cases, the `state.cssMap` is not warmed yet, so run it:
generateCacheForCSSMap(node, meta);
if (meta.state.cssMap[node.name]) {
throw buildCodeFrameError(
createErrorMessage(ErrorMessages.USE_VARIANT_OF_CSS_MAP),
node,
meta.parentPath
);
}

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

assertNoImportedCssVariables(node, meta, resolvedBinding, result);
Expand Down
1 change: 1 addition & 0 deletions packages/babel-plugin/src/utils/css-map.ts
Original file line number Diff line number Diff line change
Expand Up @@ -86,6 +86,7 @@ export enum ErrorMessages {
STATIC_PROPERTY_KEY = 'Property key may only be a static string.',
SELECTOR_BLOCK_WRONG_PLACE = '`selector` key was defined in the wrong place.',
USE_SELECTORS_WITH_AMPERSAND = 'This selector is applied to the parent element, and so you need to specify the ampersand symbol (&) directly before it. For example, `:hover` should be written as `&:hover`.',
USE_VARIANT_OF_CSS_MAP = 'You must use the variant of a CSS Map object (eg. `styles.root`), not the root object itself, eg. `styles`.',
}

export const createErrorMessage = (message: string): string => {
Expand Down
12 changes: 6 additions & 6 deletions packages/react/src/create-strict-api/__tests__/generics.test.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -19,7 +19,7 @@ describe('createStrictAPI()', () => {
});

it('should mark all styles as optional in cssMap()', () => {
const styles = cssMap({
const stylesMap = cssMap({
nested: {
'&:hover': {},
'&:active': {},
Expand All @@ -28,7 +28,7 @@ describe('createStrictAPI()', () => {
},
});

const { getByTestId } = render(<div css={styles.nested} data-testid="div" />);
const { getByTestId } = render(<div css={stylesMap.nested} data-testid="div" />);

expect(getByTestId('div')).toBeDefined();
});
Expand Down Expand Up @@ -96,7 +96,7 @@ describe('createStrictAPI()', () => {
});

it('should violate types for cssMap()', () => {
const styles = cssMap({
const stylesMap = cssMap({
primary: {
// @ts-expect-error — Type '""' is not assignable to type ...
color: '',
Expand Down Expand Up @@ -129,7 +129,7 @@ describe('createStrictAPI()', () => {
},
});

const { getByTestId } = render(<div css={styles.primary} data-testid="div" />);
const { getByTestId } = render(<div css={stylesMap.primary} data-testid="div" />);

expect(getByTestId('div')).toBeDefined();
});
Expand Down Expand Up @@ -224,7 +224,7 @@ describe('createStrictAPI()', () => {
});

it('should pass type check for cssMap()', () => {
const styles = cssMap({
const stylesMap = cssMap({
primary: {
color: 'var(--ds-text)',
backgroundColor: 'var(--ds-bold)',
Expand Down Expand Up @@ -258,7 +258,7 @@ describe('createStrictAPI()', () => {
},
});

const { getByTestId } = render(<div css={styles.primary} data-testid="div" />);
const { getByTestId } = render(<div css={stylesMap.primary} data-testid="div" />);

expect(getByTestId('div')).toHaveCompiledCss('color', 'var(--ds-text)');
});
Expand Down

0 comments on commit 34e5339

Please sign in to comment.