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

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

Closed
wants to merge 1 commit into from

Conversation

naruaway
Copy link
Contributor

@naruaway naruaway commented Jul 18, 2023

Note: this PR might not be what Kuma UI core devs want to have (See "Tradeoffs" section). In this case, feel free to just reject it! It was just fun to explore the code base for my learning purpose

This PR implements a new way to make more patterns "static".
During the build time, the following:

<Box
  fontSize={props.flagA ? 32 : 16}
  color="#123456"
  bg={props.flagA ? "red" : "green"}
  display={props.flagB ? "flex" : "block"}
/>

becomes

<Box
  className={[
    "🐻-3426465140",
    "🐻-3583227940",
    "🐻-676607144",
    "🐻-2454055305",
  ][1 * !(props.flagA) + 2 * !(props.flagB)]}
/>

with static CSS output. There is a limitation for the maximum number of combinations (hardcoded as 8 in this PR) but practically we should not have too many flags in styles anyway.

  • Now extractAttribute can return UnevaluatedConditionalExpression, which holds the conditional expression, whenTrue, and whenFalse
  • Then extractProps calls evaluateStaticBranching in a newly added static-branching.ts to statically branch the potential paths. For each possibility, it will generate CSS along with "index expression", which will be used to choose actual className in runtime.
  • the logic to update className is extracted into dedicated function updateClassNameAttr to be used in the final stage after the static branching
  • I also added vitest tests to compiler package since I wanted to add focused unit test for static-branching.ts but not sure whether this is the right way

Note that I did not modify extractPseudoAttribute for now. I think it's better to start small rather than modifying too many places until this gets proven to be actually useful

Tradeoffs

This strategy has several benefits and drawbacks.
I would like Kuma UI core devs to think about these trade offs before deciding whether we should merge this PR or not.

Benefits

  • I think having a few conditionals in styling is really common pattern in any styling solutions, one example can be like "modal open / closed based on useState". Some of these patterns can be solved in CSS level such as special selectors but in many cases it's easier to just rely on some JS variable to switch styling.
    • Notably, this pattern has been really easy with both traditional CSS convention (e.g. SUIT state) and Tailwind (e.g. just flag ? 'bg-red' : 'bg-blue') without adding performance cost. People can still say "CSS-in-JS" is slow just due to this pattern since it is common pattern.
  • Kuma UI might decide to implement other strategies to make more patterns static, however, this "static branching" strategy is orthogonal to other strategies. For example, even if Kuma UI adopts Linaria's way, this "static branching" strategy is still beneficial since there will be always "unresolvable" variables during build time.

Drawbacks

  • Increased maintenance cost (to mitigate this, I tried to isolate the logic from the existing code. The main logic lives in static-branching.ts with unit tests)
  • Increased compile time (haven't measured yet but I suspect anyway there is several other places to optimize)
  • For end users, it will be harder to understand "which pattern can be statically extracted or not" (This might not be an big issue since we have 🐻 emoji during development)
  • Increased CSS size: it will generate larger CSS. This can be mitigated by CSS optimizers in production such as lightning CSS. We might be able to rely on features such as CSSO's "scopes" option as well for maximum optimization. Note that I created an example using CSSO here. To mitigate this issue from another angle, I think we can consider atomic CSS output like Linaria does. However this also has its own trade off: HTML output will be larger for excessively repeated elements.

Alternatives considered

As an alternative, we could generate multiple static classNames for each conditional, however in this PR I did not choose this path simply because the code will be more complicated. I believe that practically generated styles will not be so large especially with CSS optimizers and the runtime performance matters more. This way can be easier if the overall design is based on "atomic styles" like Linaria does in callstack/linaria#867

Future potential improvements

Here is the list of things I did not implement in this PR to avoid too much change at this point:

  • static-branching.ts can be further improved like recognizing "actually the same flag" such as !flag and flag. This kind of improvement should not add much maintenance cost since it's completely encapsulated in static-branching.ts
  • We can also support conditional expressions inside arbitrary expressions such as {margin: flag ? 1 : 2}. This will be especially useful for pseudo props such as _hover

We might want to do further refactor if we go with this path. For example, probably it's better to reconsider the usage of ts-morph in favor of babel if there is no actual plan to utilize type information. Babel has very useful utilities such as nodePath.evaluate (StackBlitz example)

Manual testing with Next.js

I tested this change with Next.js using the following code:

const Page = () => {
  return (
    <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}>
          Item {i}
        </Box>
      ))}
    </Box>
  );
};

Before this PR, this falled back to "dynamic mode" and without KumaRegistry, it was causing FOUC.

before

After this PR, this generates two class names with styles statically and there is no FOUC since now it's "static" (almost exactly the same performance as Tailwind / manual CSS except for the overhead of Box itself, which is different thing we could optimize out in the future)

after

Misc: How other libraries are doing?

TODO: This might be kind of novel idea (similar idea originally coming from my closed source prototype several years ago) but not sure. I suspect some popular libs might have similar logic, let me know if anyone find such code since I am interested!

Things to care if we decide to ship this PR

@changeset-bot
Copy link

changeset-bot bot commented Jul 18, 2023

⚠️ No Changeset found

Latest commit: 23fe484

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
Copy link

vercel bot commented Jul 18, 2023

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 18, 2023 8:08am

@codesandbox-ci
Copy link

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 23fe484:

Sandbox Source
React Configuration
React Typescript Configuration

@astahmer
Copy link

Hey, this looks cool ! I noticed this PR after @poteboy 's tweet, since we have a similar feature in Panda I can maybe provide a few tips


Increased compile time (haven't measured yet but I suspect anyway there is several other places to optimize)

I don't think it'll be much slower unless the eval fn has to traverse files to resolve identifiers (which we disabled mostly for that reason)


For end users, it will be harder to understand "which pattern can be statically extracted or not" (This might not be an big issue since we have 🐻 emoji during development)

indeed, for that I recommend having an online playground and/or a debug command, where you can see the generated CSS / what was extracted


For example, probably it's better to reconsider the usage of ts-morph in favor of babel if there is no actual plan to utilize type information. Babel has very useful utilities such as nodePath.evaluate (StackBlitz example)

fwiw, we use ts-evaluator to eval conditions (and simple CallExpression)


let me know if anyone find such code since I am interested!

we do this here:

then we have this maybeResolveConditionalExpression function that tries to only pick 1 branch when possible https://github.com/chakra-ui/panda/blob/be0ad578156d1bed81ab26be770569674b56fb3d/packages/extractor/src/maybe-box-node.ts#L265


Increased maintenance cost (to mitigate this, I tried to isolate the logic from the existing code. The main logic lives in static-branching.ts with unit tests)

regarding btw, now that I'm thinking about it, you could very well use the maybeBoxNode method from @pandacss/extractor to handle the static extraction part, since it's published on npm on its own as well

we use it though the extract fn (also exposed from @pandacss/extractor) that traverses a ts-morph SourceFile to find components/functions/tagged template literals and return a list of BoxNode results in a map by identifier name

@naruaway
Copy link
Contributor Author

naruaway commented Jul 18, 2023

@astahmer thanks for the comment! Somehow I haven't looked at Panda's implementation yet and I only checked other zero-runtime libs such as linaria or atlassian/compiled.

Just to provide some more context, around 2021, I applied similiar techniques together with Linaria-like evaluator to Indeed's web sites. This information is public:

We built a webpack plugin that precompiled many of our most commonly used components, reducing their render costs and helped address the problem.

From Speed Matters, But It Isn’t Everything - Indeed Engineering Blog

There I created generic optimizer which can even completely compile out Chakra UI's Box since it runs SSR-like logic during the build time to extract style tags generated from emotion.
Although Indeed is not using Chakra UI, it's still using my plugin. But the plugin is too complicated and slow (I suspect that mainly due to the evaluator) and it's not open sourced still although I wanted to open source it.

ts-evaluator is cool! I did not know that lib. It looks like it's using actual exeuction so I suspect that it can be slower than babel's one although it can be much more powerful?

Related to this, do you think ts-morph is better than babel for this kind of logic? Maybe you chose ts-morph to use ts-evaluator?

Thanks for providing links to Panda's corresponding implementations, I'll take a look later!

For @pandacss/extractor, I was thinking the similar thing for a while since I created closed source lib several years ago to "precompute" existing runtime CSS-in-JS libs so that we can separate the effort for "static extractor" and "actual styling lib". Maybe it can be beneficial to have fast & reliable static extractor in reusable way across CSS-in-JS libs. I guess for Kuma UI, it's something core devs (I am not a core dev and this is my first PR in Kuma UI) need to decide whether it should rely on @pandacss/extractor or not.

@naruaway
Copy link
Contributor Author

naruaway commented Jul 18, 2023

@astahmer I played with Panda finally and realized that it's so smart! I really wanted to have this 2 years ago 😅
It looks like there are still some patterns we can statically decide; like chooseVariant in this playground. Trying to cover all these potential cases while keeping it performant is definitely not an easy task

@astahmer
Copy link

Although Indeed is not using Chakra UI, it's still using my plugin. But the plugin is too complicated and slow (I suspect that mainly due to the evaluator) and it's not open sourced still although I wanted to open source it.

this sounds very interesting ! would be cool if you have the time to opensource it 🙏


ts-evaluator is cool! I did not know that lib. It looks like it's using actual exeuction so I suspect that it can be slower than babel's one although it can be much more powerful?

I can't say for sure as I haven't dug into the library's code too deeply but yes I think so, since we can pass an "environment" with variables/functions etc it can be pretty powerful
no idea if it's faster or slower than babel, a benchmark of different static analyzer could be interesting indeed !


Related to this, do you think ts-morph is better than babel for this kind of logic? Maybe you chose ts-morph to use ts-evaluator?

(I've been asked this question before, I'm pasting from this tweet):

I havent used much babel for that, but what I like regardless with ts-morph is:

  • typesafety with Node.isXXX
  • it "just works" with js(x)/ts(x)
  • the manual/fast traversing with forEachDescendant
  • is somehow able to parse files with custom syntax mostly correctly -> as in, not crashing and not messing with the rest of AST nodes after encountering an unknown syntax such as svelte {#xxx expr} or astro xxx:yyy directives
  • the structures, basically simplified AST (less details, easier manipulation to get X props from Y interface for ex)
  • also, the gorgeous viewer https://ts-ast-viewer.com

@astahmer
Copy link

astahmer commented Jul 18, 2023

@astahmer I played with Panda finally and realized that it's so smart! I really wanted to have this in 2 years ago 😅
It looks like there are still some patterns we can statically decide; like chooseVariant in this playground. Trying to cover all these potential cases while keeping it performant is definitely not an easy task

yes this one is something that ideally should be supported but this would require a bit of work on the maybeBoxNode logic because currently we don't have a way to return an array of multiple possible values (we do have the BoxNodeConditional that can hold whenTrue and whenFalse recursively), and this pattern suggests that the return value of object[prop] COULD be any values in object if prop is not statically extractable

so definitely not high in the issue priority since we have the cva alternative (that encourage a better pattern in the end, with more powerful features !), but would be "nice to have" nonetheless

we covered that in the docs here
https://panda-css.com/docs/guides/dynamic-styling#runtime-reference-on-known-objects

@naruaway
Copy link
Contributor Author

Note: we discussed with core maintainers and decided to go with slightly different way and continue in #233

For now we avoid relying on external dependencies such as packages from Panda CSS. We might reconsider though.

Thanks for all the comments and pointers!

Closing this PR

@naruaway naruaway closed this Jul 20, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants