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

Use AST transformations in @tailwindcss/postcss #15297

Merged
merged 25 commits into from
Dec 4, 2024

Conversation

RobinMalfait
Copy link
Member

@RobinMalfait RobinMalfait commented Dec 4, 2024

This PR improves the @tailwindcss/postcss integration by using direct AST transformations between our own AST and PostCSS's AST. This allows us to skip a step where we convert our AST into a string, then parse it back into a PostCSS AST.

The only downside is that we still have to print the AST into a string if we want to optimize the CSS using Lightning CSS. Luckily this only happens in production (NODE_ENV=production).

This also introduces a new private compileAst API, that allows us to accept an AST as the input. This allows us to skip the PostCSS AST -> string -> parse into our own AST step.

To summarize:

Instead of:

  • Input: PostCSS AST -> .toString() -> CSS.parse(…) -> Tailwind CSS AST
  • Output: Tailwind CSS AST -> toCSS(ast) -> postcss.parse(…) -> PostCSS AST

We will now do this instead:

  • Input: PostCSS AST -> transform(…) -> Tailwind CSS AST
  • Output: Tailwind CSS AST -> transform(…) -> PostCSS AST

Running this on Catalyst, the time spent in the @tailwindcss/postcss looks like this:

  • Before: median time per run: 19.407687 ms
  • After: median time per run: 11.8796455 ms

This is tested on Catalyst which roughly generates ~208kb worth of CSS in dev mode.

While it's not a lot, skipping the stringification and parsing seems to improve this step by ~40%.

Note: these times exclude scanning the actual candidates and only time the work needed for parsing/stringifying the CSS from and into ASTs. The actual numbers are a bit higher because of the Oxide scanner reading files from disk. But since that part is going to be there no matter what, it's not fair to include it in this benchmark.

RobinMalfait and others added 9 commits December 4, 2024 11:00
These methods can convert PostCSS ASTs to our internal CSS ASTs and vice
versa. This allows us to skip a step where introduce
parsing/stringification.

Instead of:
- `PostCSS AST` -> `.toString()` -> `postcss.parse` -> `CSS AST`
- `CSS AST` -> `toCSS(ast)` -> `CSS.parse` -> `PostCSS AST`

We will now do this instead:
- `PostCSS AST` -> `transform(…)` -> `CSS AST`
- `CSS AST` -> `transform(…)` -> `PostCSS AST`
This introduces an `optimizeAst(…)` function that creates a fresh AST
from an AST but handles all the edge cases we used to have in
`toCss(…)`.

For example, `@property` is deduped (and fallbacks are generated for
older browsers), `Context` nodes are transparent and `AtRoots` move
nodes into the actual root.

This allows us to simplify the `toCss(…)` code to be a 1-to-1
translation and simply print `declarations`, `rules`, `at-rules` and
comments. We don't have to worry about the other special logic.

This also means that we don't have to re-introduce the same logic in our
PostCSS AST <-> Our AST transformations.

---

In addition: I ran some checks on the Catalyst codebase, and noticed
that we have the most declarations, then rules, then at-rules, then
at-roots, then context nodes and last but not least comments.

With this information in mind, the if-branches are using this
information to first check for declarations, then rules, ...
@RobinMalfait RobinMalfait force-pushed the feat/ast-transformations branch from 977c45d to 8c1b0d0 Compare December 4, 2024 10:00
@RobinMalfait RobinMalfait force-pushed the feat/ast-transformations branch from 8c1b0d0 to 2adaf42 Compare December 4, 2024 10:03
If we are not optimizing, we don't need the `context.cachedAst` because
we will use the optimziedAst instead.
packages/@tailwindcss-node/src/compile.ts Outdated Show resolved Hide resolved
packages/tailwindcss/src/css-parser.ts Show resolved Hide resolved
packages/tailwindcss/src/index.ts Outdated Show resolved Hide resolved
let atRoots: string = ''
// Optimize the AST for printing where all the special nodes that require custom
// handling are handled such that the printing is a 1-to-1 transformation.
export function optimizeAst(ast: AstNode[]) {
Copy link
Member

Choose a reason for hiding this comment

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

Hm now that I see this in the whole PR I'm not so super convinced about this name anymore. The reason being that we refer to optimize as the step in post-compilation where we run lightningcss. Now we're overloading the term with something used during the compilation. Maybe applyFallbacksToAst or something like this? 🤔 I also dig your initial name of prepareAstForPrinting honestly lol. Find it less confusing then overloading the term.

Copy link
Member Author

Choose a reason for hiding this comment

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

Hmm I see. The problem is that this is doing multiple things. While applyFallbacksToAst is correct, we are also in addition to tracking the fallbacks making sure that @property is handled once, that Context nodes become transparent and that AtRoot nodes are hoisted.

prepareAstForPrinting is a bit more vague which is good to describe that we are doing multiple things. But the forPrinting part is off because in @tailwindcss/postcss that would be prepareAstForTransformingIntoOtherAST 😅

Copy link
Member

Choose a reason for hiding this comment

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

Yeah, not sure either. Maybe optimize is fine since as a mental model nothing is stopping us from doing the actual lightningcss optimizations in there and frankly it might be where we're heading to get rid of some of the duplicate declarations etc.

packages/@tailwindcss-postcss/src/index.ts Outdated Show resolved Hide resolved
So many ASTs, but which ones?
This allows us to bail earlier and delay the `optimizeAst(…)` call and
only do it when it's necessary.
1. If a PostCSS comment is coming in, only map it to our AST's comment
   node if it starts with `!` (license comments).
2. If a comment from our AST is mapped to a PostCSS AST Comment, then
   remove the default whitespace and rely on the whitespace encoded in
   `node.value`.
Bonus points: this also means that PostCSS doesn't need to do a walk to
try and detect the indentation.
@RobinMalfait RobinMalfait force-pushed the feat/ast-transformations branch from 5db528d to 1a04957 Compare December 4, 2024 13:33
Unrelated to this PR, but noticed it when running `prettier`
@RobinMalfait RobinMalfait marked this pull request as ready for review December 4, 2024 14:34
@RobinMalfait RobinMalfait requested a review from a team as a code owner December 4, 2024 14:34
Copy link
Member

@philipp-spiess philipp-spiess left a comment

Choose a reason for hiding this comment

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

Superb stuff

Copy link
Member

Choose a reason for hiding this comment

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

lol what happened here, I guess i forgot to run prettier or so 🙈

let atRoots: string = ''
// Optimize the AST for printing where all the special nodes that require custom
// handling are handled such that the printing is a 1-to-1 transformation.
export function optimizeAst(ast: AstNode[]) {
Copy link
Member

Choose a reason for hiding this comment

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

Yeah, not sure either. Maybe optimize is fine since as a mental model nothing is stopping us from doing the actual lightningcss optimizations in there and frankly it might be where we're heading to get rid of some of the duplicate declarations etc.

@philipp-spiess philipp-spiess merged commit 408fa99 into next Dec 4, 2024
1 check passed
@philipp-spiess philipp-spiess deleted the feat/ast-transformations branch December 4, 2024 14:44
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.

3 participants