feat(compiler): static branching to handle conditional expressions #233

naruaway · 2023-07-20T02:32:25Z

This is a continuation of #229

After discussing with core maintainers, we decided to separate class names per conditionals.
In this way we no longer have to limit the combination limit up to arbitrary number such as "8" as seen in the previous PR.

The strong assumption here is that each style prop is isolated with each other. It's like "atomic CSS" but this PR just generates minimum number of classnames rather than actually generating separate classnames per each style prop.

Due to this "atomic-ish" assumption, now the logic is actually simpler

Most of the things in the previous PR description still applies. For example, if we decide to ship this PR, we should update documentation to clarify "static vs. dynamic"

Example

function MyComponent({flag, flag2}) {
  return <Box fontSize={flag ? 24 : 16} p={12} m={flag ? 3 : 7} color={flag2 ? "blue" : "red"} />
}

becomes:

<Box className={`🐻-652625356 ${(flag) ? "🐻-2967870670" : "🐻-833645408"} ${(flag2) ? "🐻-3852842568" : "🐻-944610715"}`} />

Note that flag is used twice in the origianl code but the compiled one only includes one conditional for flag

Verification using Next.js

I tested with the following:

<Box fontFamily="sans-serif" color="#555555" m={32} border="1px solid #cccccc">
  {Array.from({ length: 10 }, (_, i) => (
    <Box key={i} bg={i % 2 === 0 ? "#cccccc" : "#ffffff"} p={8} color={i % 3 === 0 ? "#cccccc" : "#ffffff"}>
      Item {i}
    </Box>
  ))}
</Box>

Before this PR

Conditionals were treated as "dynamic" (indicated by 🦄 classNames) and there is a flash of unstyled content (FOUC) since I did not set up KumaRegistry

After this PR

Conditionals are treated as "static" (indicated by 🐻 classNames). There is no FOUC

Note that I confirmed it works as well for production build

changeset-bot · 2023-07-20T02:32:27Z

⚠️ No Changeset found

Latest commit: 8522fde

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

vercel · 2023-07-20T02:32:28Z

The latest updates on your projects. Learn more about Vercel for Git ↗︎

Name	Status	Preview	Comments	Updated (UTC)
kuma-ui-website	✅ Ready (Inspect)	Visit Preview	💬 Add feedback	Jul 30, 2023 3:00pm

codesandbox-ci · 2023-07-20T02:33:37Z

This pull request is automatically built and testable in CodeSandbox.

To see build info of the built libraries, click here or the icon next to each commit SHA.

Latest deployment of this branch, based on commit 8522fde:

Sandbox	Source
React	Configuration
React Typescript	Configuration

naruaway · 2023-07-20T02:37:50Z

packages/compiler/src/collector/collect.ts

  jsxAttributes.forEach((jsxAttribute) => {
    if (Node.isJsxAttribute(jsxAttribute)) {
-      const propName = jsxAttribute.getNameNode().getFullText();
+      const propName = jsxAttribute.getNameNode().getText();


Note: @poteboy I guess the usage of getFullText was probably a mistake. Due to this we have several trim() in compiler package. I did not include the removals of trim() in this PR but we can remove them as well in later PRs

Ah I see, thanks this helps a lot :)

naruaway · 2023-07-20T02:43:00Z

Note on code structure: things like classifyProps lives in existing file rather than static-branching.ts, I might try refactor the structure more later although it should not affect the behavior

kotarella1110

Amazing! I feel that the logic has become simpler and more readable compared to the previous pull request. It would be even better if we could use this logic to support pseud props.
LGTM 🎉

kotarella1110 · 2023-07-26T15:02:04Z

@naruaway please resolve conflict 🙏

naruaway · 2023-07-27T01:32:10Z

@kotarella1110 thanks! Conflict resolved now
I guess additional processing such as pseudo props handling can be added later to reduce the risk of introducing many new bugs

Note that as discussed, one of potential bugs of this PR could be "style override rule can change between static and dynamic" but we think this PR does not make the problem worse since if any of our style props have conflicting output style, this problem already exists

poteboy · 2023-07-28T23:56:10Z

packages/compiler/src/static-branching.ts

@@ -0,0 +1,21 @@
+export type AttributeValue =


Since AttributeValue has already been defined in compile.ts, it seems redundant to me to redefine the same type.

poteboy · 2023-07-29T00:20:05Z

packages/compiler/src/collector/collect.ts

@@ -50,6 +63,28 @@ const extractAttribute = (jsxAttribute: JsxAttribute) => {
        const expression = initializer.getExpression();
        if (!expression) return;

+        // fontSize={... ? ... : ...}
+        const conditionalExpression = match(expression)
+          .when(Node.isConditionalExpression, (conditional) => {


I actually prefer this logic to be placed in expression.ts because, as a reader, it's more intuitive to have all expressions handled in expression.ts

Sure, the initial reason why I put it here is to limit the scope of the change; but at this point probably better to clean and unify the logic there. Let me try a little bit refactoring although it will affect more such as pseduo props handling, which might be actually desired as long as.... it works without unexpected bug :)

@poteboy, now I updated this PR to include some refactorings and pseudo props should be also handled properly for conditional expression (cc @kotarella1110) 👍

naruaway · 2023-07-30T14:14:59Z

packages/babel-plugin/src/__test__/k.test.ts

+        const inputCode = `
+        import { k } from '@kuma-ui/core'
+        function App({ flag }) {
+          return <k.div p={2} m={flag ? 100 : 10} _hover={flag ? { color: 'blue', bgColor: 'gray'} : {color: 'green'}} />


Note: See the snapshot for how this is compiled. Note that conditional expression can appear at the top level of pseudo props or per prop level. If conditional expressions are nested, it will bail out

naruaway · 2023-07-30T14:21:26Z

packages/compiler/src/collector/collect.ts

-      let propValue;
-      // If the propName starts with underscore, use extractPseudoAttribute
-      if (propName.trim().startsWith("_")) {
-        propValue = extractPseudoAttribute(jsxAttribute);


Note: now extractPseudoAttribute is gone and we just use the same function, extractAttribute for both

With the current changes, the logic for determining the pseudo prop has been removed. This is unsafe, so it is necessary to add similar determination logic in handleJsxExpression by tracing back to the parent node from the expression.

naruaway · 2023-07-30T14:23:01Z

packages/compiler/src/collector/expression.ts

+
+export const handleJsxExpression = (
+  node: Node<ts.Node>
+): types.Value | undefined => {


Note: Now the semantics of the return value of this function is clearer: undefined means "failed to evaluate" and types.Value can contain undefined. Previously these are not separated and <Box color={undefined} /> was not statically compiled since undefined means "it failed to extract in the logic"

naruaway · 2023-07-30T14:28:16Z

packages/compiler/src/collector/expression.ts

+/**
+ * Normalize ts-morph Node to make further processing simpler
+ */
+const normalizeNode = (node: Node): Node => {


Note: I copied this from decode.ts, which is now removed. Now this is used internally by handleJsxExpression consistently. Previous pattern was error prone since people might forget to call decode where necessary. I think there is no reason to split the call between decode and normalizeNode. Note that in the future probably we can replace this kind of logic with some library such as ts-evaluate

naruaway · 2023-07-30T14:30:38Z

packages/compiler/src/collector/expression.ts

      .when(Node.isObjectLiteralExpression, (obj) => {
-        // TODO
-        return undefined;
+        const entries: [string, types.Value][] = [];


I mostly copied the logic in pseudo.ts, which is now removed.
Note that in the original logic, computed properties were silently erased, which is unexpected.

For example, _hover prop was removed from <Box _hover={{ ['color']: 'red'}} /> without generating the red color CSS. In my PR, when it has computed property, it is just considered as "dynamic". I think introducing more powerful evaluator can make this pattern "static" as well in the future

naruaway · 2023-07-30T14:32:54Z

.github/workflows/main.yml

@@ -56,4 +56,4 @@ jobs:
        run: pnpm install

      - name: Run lint
-        run: pnpm lint
+        run: pnpm build && pnpm lint


Note: I realized ESLint emits false positive errors without pnpm build. Probably the correct way to fix it to somehow create a command using turborepo for lint to depend on build but I am not familiar. This should work for now at least for PR pipelines

naruaway · 2023-07-30T14:48:54Z

packages/compiler/src/extractor/extract.ts

+    Object.entries(props)
+      .filter(([, value]) => value !== undefined)
+      .sort((a, b) => a[0].localeCompare(b[0]))
+      // eslint-disable-next-line @typescript-eslint/restrict-template-expressions -- FIXME


This ESLint error is an actual bug. This shows that proper TypeScript typing (basically, eliminating any) with type-aware ESLint rules are really powerful to detect bugs 🎉

The error is the following:

error Invalid type "StaticValue" of template literal expression @typescript-eslint/restrict-template-expressions

This indicates that we are trying to serialize object into string, which is not expected. I tested main (2648f5eb5ac8d8cb1a50dd8c84539e314eb4afaa) and confirmed that generateKey returns something like _hover:[object Object]|bg:#576ddf|borderRadius:14px|color:white|cursor:pointer|fontWeight:600|p:16px 32px, which is broken since nested object is stringified as [object Object].

I filed #246

naruaway · 2023-07-30T15:02:23Z

packages/compiler/src/types.ts

+  whenFalse: StaticValue;
+};
+
+export type Value =


Note: This is the most important type in this PR, which tries to encode the potential patterns (e.g. we see that conditional can only contain StaticValue, which means that nested conditionals are not allowed)

poteboy · 2023-08-02T13:27:19Z

packages/compiler/src/collector/expression.ts

-        return undefined;
+        const entries: [string, types.Value][] = [];
+        for (const prop of obj.getProperties()) {
+          if (Node.isPropertyAssignment(prop)) {


I prefer early returns over full-body conditional wrapping in function declaration :)

poteboy · 2023-08-02T13:40:42Z

packages/compiler/src/collector/expression.ts

+          return types.staticValue(
+            Object.fromEntries(
+              entries.map((e) =>
+                e[1].type === "Static" ? [e[0], e[1].value] : ({} as never)


The seems redundant as we've already confirmed that all elements in entries are static. We could directly pass entries to Object.fromEntries

Oh right, actually this is accidental since the argument of staticValue is StaticValue but the type of Object.fromEntries(entries) is { [k: string]: types.Value; }. Ideally type check should fail and somehow I thought it fails but it actually passes 🤦

Anyway the runtime logic is correct so as long as we cover this part in test cases it should be acceptable, thanks for removing redundant checks 👍

My bad, this operation is actually necessary to handle pseudo props. I'm not sure why though.

I also missed this although I wrote it: it's because e[1].value is needed to unwrap the value from Static variant. e[1] is like {type: "Static", value: /* actual value here */ }

handle isArrayLiteralExpression iff pseudo props

vercel bot deployed to Preview July 20, 2023 02:33 View deployment

naruaway commented Jul 20, 2023

View reviewed changes

naruaway mentioned this pull request Jul 20, 2023

feat(compiler): static branching to handle conditional expressions #229

Closed

kotarella1110 approved these changes Jul 24, 2023

View reviewed changes

kotarella1110 mentioned this pull request Jul 26, 2023

docs(website): improve hybrid example for static branching #243

Open

naruaway force-pushed the static-branching-atomic-ish branch 3 times, most recently from 4f1879e to f23b415 Compare July 27, 2023 01:27

vercel bot deployed to Preview July 27, 2023 01:28 View deployment

poteboy reviewed Jul 28, 2023

View reviewed changes

poteboy reviewed Jul 29, 2023

View reviewed changes

naruaway added 2 commits July 30, 2023 23:07

feat(compiler): static branching to handle conditional expressions

981993e

chore(compiler): add tests for conditionals with pseudo props

5b55106

naruaway force-pushed the static-branching-atomic-ish branch from f23b415 to 5b55106 Compare July 30, 2023 14:08

vercel bot deployed to Preview July 30, 2023 14:10 View deployment