Add CSS Map #1492

liamqma · 2023-08-10T03:30:13Z

Add the support for cssMap

import { css, cssMap } from '@compiled/react';

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

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

atlassian-cla-bot · 2023-08-10T03:30:16Z

Thank you for your submission! Like many open source projects, we ask that you sign our CLA (Contributor License Agreement) before we can accept your contribution.
If your email is listed below, please ensure that you sign the CLA with the same email address.

The following users still need to sign our CLA:
❌AtlassianRubberDuck

_{Already signed the CLA? To re-check, try refreshing the page.}

changeset-bot · 2023-08-10T03:30:16Z

⚠️ No Changeset found

Latest commit: 3b6f848

Merging this PR will not cause a version bump for any packages. If these changes should not result in a new version, you're good to go. If these changes should result in a version bump, you need to add a changeset.

This PR includes no changesets

When changesets are added to this PR, you'll see the packages that this PR includes changesets for and the associated semver types

Click here to learn what changesets are, and how to add one.

Click here if you're a maintainer who wants to add a changeset to this PR

packages/babel-plugin/src/css-prop/__tests__/css-map.test.ts

packages/babel-plugin/src/utils/css-map.ts

packages/react/src/css-map/index.ts

Co-authored-by: Jake Lane <[email protected]>

examples/webpack/src/app.jsx

packages/babel-plugin/src/css-prop/__tests__/css-map.test.ts

packages/babel-plugin/src/utils/css-map.ts

dddlr · 2023-08-16T01:32:02Z

packages/babel-plugin/src/utils/css-map.ts

+          '&&',
+          t.binaryExpression(
+            '===',
+            t.isStringLiteral(objectProperty)


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

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) ) ...

packages/babel-plugin/src/utils/css-map.ts

tomgasson

A few thoughts

tomgasson · 2023-08-18T00:44:12Z

examples/parcel/src/ui/css-map.jsx

@@ -0,0 +1,12 @@
+import { css, cssMap } from '@compiled/react';
+
+const styles = cssMap({


nit: Let's simplify this to

import { css } from '@compiled/react' const styles = css.create({ ... });

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

tomgasson · 2023-08-18T00:51:59Z

packages/babel-plugin/src/css-prop/__tests__/css-map.test.ts

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


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

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'

tomgasson · 2023-08-18T00:52:24Z

packages/babel-plugin/src/css-prop/__tests__/css-map.test.ts

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


Quoting inconsistency here

tomgasson · 2023-08-18T01:08:38Z

packages/babel-plugin/src/utils/css-map.ts

+ * const Component = ({ borderStyle }) => <div css={css([
+ *   borderStyle === 'none' && { borderStyle: 'none' },
+ *   borderStyle === 'solid' && { borderStyle: 'solid'}
+ * ])} />


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?

um...good point. Let me look into it 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.

tomgasson · 2023-08-18T01:11:12Z

examples/parcel/src/ui/css-map.jsx

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


Just walking my way through this...

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>

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

Compiled essentially transforms one CSS rule into three pieces.

classNames

sheets

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)?

packages/babel-plugin/src/utils/css-map.ts

dddlr · 2023-08-18T03:51:37Z

packages/babel-plugin/src/css-prop/__tests__/css-map.test.ts

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)

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.

Ah no worries 👍

packages/babel-plugin/src/utils/css-map.ts

dddlr · 2023-08-18T04:39:13Z

packages/babel-plugin/src/utils/css-map.ts

+  meta: Metadata,
+  evaluateExpression: EvaluateExpression
+):
+  | {


Nit: not sure what this extra pipe character does here

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

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

packages/babel-plugin/src/utils/css-map.ts

dddlr

LGTM, just got some nitpicks

liamqma · 2023-08-31T01:54:35Z

An alternative implementation has been adopted.

liamqma requested review from JakeLane, pancaspe87 and dddlr as code owners August 10, 2023 03:30

liamqma force-pushed the css-map branch 2 times, most recently from cb0f1c8 to 334d839 Compare August 10, 2023 03:51

Add CSS Map

664672b

liamqma force-pushed the css-map branch from 334d839 to 664672b Compare August 10, 2023 06:12

JakeLane requested changes Aug 15, 2023

View reviewed changes

liamqma and others added 3 commits August 16, 2023 09:50

Update packages/babel-plugin/src/css-prop/__tests__/css-map.test.ts

73a7ffb

Co-authored-by: Jake Lane <[email protected]>

Update packages/babel-plugin/src/css-prop/__tests__/css-map.test.ts

f5e8a25

Co-authored-by: Jake Lane <[email protected]>

Remove console.log

0a22e90

dddlr requested changes Aug 16, 2023

View reviewed changes

liamqma added 6 commits August 17, 2023 09:18

Update function name

184c044

Add CSS Map to Parcel example

8a028d5

Refactor error handling

dc772f8

Add doc to cssMap

c0fde8d

Regenerate cssMap Flow types

8002047

Merge branch 'master' into css-map

7c0ac9a

liamqma force-pushed the css-map branch from 3ba5a88 to 2d355f1 Compare August 17, 2023 23:55

Update test

3f6a017

liamqma force-pushed the css-map branch from 2d355f1 to 3f6a017 Compare August 17, 2023 23:56

JakeLane previously approved these changes Aug 18, 2023

View reviewed changes

tomgasson reviewed Aug 18, 2023

View reviewed changes

dddlr reviewed Aug 18, 2023

View reviewed changes

Update error messages

4748a80

liamqma dismissed JakeLane’s stale review via 4748a80 August 21, 2023 01:41

Merge branch 'master' into css-map

3b6f848

dddlr approved these changes Aug 21, 2023

View reviewed changes

liamqma mentioned this pull request Aug 23, 2023

CSS Map alternative compilation approach #1496

Merged

liamqma closed this Aug 31, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add CSS Map #1492

Add CSS Map #1492

liamqma commented Aug 10, 2023 •

edited

Loading

atlassian-cla-bot bot commented Aug 10, 2023 •

edited

Loading

changeset-bot bot commented Aug 10, 2023 •

edited

Loading

dddlr Aug 16, 2023

dddlr Aug 16, 2023

tomgasson left a comment

tomgasson Aug 18, 2023

liamqma Aug 18, 2023

tomgasson Aug 18, 2023

liamqma Aug 18, 2023

tomgasson Aug 18, 2023

tomgasson Aug 18, 2023

liamqma Aug 18, 2023

liamqma Aug 21, 2023 •

edited

Loading

tomgasson Aug 18, 2023

liamqma Aug 18, 2023

dddlr Aug 18, 2023

liamqma Aug 21, 2023

dddlr Aug 21, 2023

dddlr Aug 18, 2023

dddlr Aug 18, 2023

liamqma Aug 21, 2023

dddlr Aug 21, 2023

dddlr left a comment

liamqma commented Aug 31, 2023

		@@ -0,0 +1,12 @@
		import { css, cssMap } from '@compiled/react';

		const styles = cssMap({

Add CSS Map #1492

Add CSS Map #1492

Conversation

liamqma commented Aug 10, 2023 • edited Loading

atlassian-cla-bot bot commented Aug 10, 2023 • edited Loading

changeset-bot bot commented Aug 10, 2023 • edited Loading

⚠️ No Changeset found

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tomgasson left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

liamqma Aug 21, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dddlr left a comment

Choose a reason for hiding this comment

liamqma commented Aug 31, 2023

liamqma commented Aug 10, 2023 •

edited

Loading

atlassian-cla-bot bot commented Aug 10, 2023 •

edited

Loading

changeset-bot bot commented Aug 10, 2023 •

edited

Loading

liamqma Aug 21, 2023 •

edited

Loading