-
-
Notifications
You must be signed in to change notification settings - Fork 4.8k
Generalize the walk implementation
#19126
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
Conversation
packages/tailwindcss/src/ast.test.ts
Outdated
| if (node.kind === 'context') { | ||
| walkContext = { ...walkContext, ...node.context } | ||
| return | ||
| } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If you read this commit by commit, this will get replaced in future commits!
| expect(visited).toMatchInlineSnapshot(` | ||
| Set { | ||
| "@media ", | ||
| "<context>", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is added because the generic walk doesn't skip context nodes.
| walk(valueAst, { | ||
| exit(valueNode) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This diff looks big, but it's mainly re-indents
1ab29dd to
df7f3a3
Compare
| Continue: { kind: WalkKind.Continue } as const, | ||
| Skip: { kind: WalkKind.Skip } as const, | ||
| Stop: { kind: WalkKind.Stop } as const, | ||
| Replace: <T>(nodes: T | T[]) => | ||
| ({ kind: WalkKind.Replace, nodes: Array.isArray(nodes) ? nodes : [nodes] }) as const, | ||
| ReplaceSkip: <T>(nodes: T | T[]) => | ||
| ({ kind: WalkKind.ReplaceSkip, nodes: Array.isArray(nodes) ? nodes : [nodes] }) as const, | ||
| ReplaceStop: <T>(nodes: T | T[]) => | ||
| ({ kind: WalkKind.ReplaceStop, nodes: Array.isArray(nodes) ? nodes : [nodes] }) as const, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The only reason these are uppercase is because they represent enum values. We don't have ADTs in JS sadly, so we work around it by having a function that returns a specific object.
All of them have a unique kind discriminant.
We already had Continue, Skip, Stop, and replaceWith. I then added Replace(…), ReplaceSkip(…) and ReplaceStop(…).
Open for suggestions for better names here.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
These names seem completely fine to me imo.
Like you could go with ReplaceAndSkip(…) or w/e if you wanted but I really don't think that is necessary. This perfectly readable as is.
I also dig the ADT-ish syntax here so also like the uppercase names 🤷♂️
While refactoring, I ran into issues because sometimes we destructure `parent` which happens to be a "global" value that TypeScript is fine with if it's undefined (tests would crash). Using `ctx`, also helped to know where we are using all context related values. This will make future commits a bit more readable as well. Might go back to a destructured pattern, but let's do this for now. In future commits you will also see that often the `ctx` is just gone, because instead of using `ctx.replaceWith()` becomes a `return …`
This is completely generic, the only requirement is that "parent" nodes
have `{ nodes: T[] }` as the signature.
It's a new iterative walk with minimal memory usage and performance
better than the recursive based walk function we had.
This also adds new functionality where we now can have an `enter` and
`exit` phase. The `exit` phase is like a depth-first where we visit leaf
nodes first.
If you have both, we only do a single walk down and up again instead of
you having to write 2 separate walks that traverse the entire tree.
Some API changes:
1. `replaceWith(…)` doesn't exist anymore, instead you return any of the
`WalkAction`s. E.g.: `return WalkAction.Replace(…)`
2. `path` is not build while traversing, instead it's a function you can
call to lazily compute it because in most cases we don't even need
this functionality.
One benefit is that there is no call stack, so the moment you stop a
walk it's an instant return without unwinding the call stack.
All actions you can do are:
- `WalkAction.Continue`
- `WalkAction.Skip`
- `WalkAction.Stop`
- `WalkAction.Replace(…)` — replace nodes and continue walking the new nodes
- `WalkAction.ReplaceSkip(…)` — replace nodes and skip walking the new nodes
- `WalkAction.ReplaceStop(…)` — replace nodes and stop the entire traversal
The CSS AST has `context` nodes that you can access while walking the tree, where they build up information over time. Additionally, when looking at the `path` or the `parent` all the `context` nodes are hidden. This adds a small wrapper for the context object when walking over AstNode's and have to access the `.context`, `.parent` or `.path()`
1. Use single function with noop `enter` and `exit` functions instead of 3 separate functions. 2. Instead of repeating some logic with sub-switches, just toggle the offset and let the loop go around. 3. Cleanup some comments
b9f134f to
76a5ea1
Compare
This PR generalizes the
walkimplementations we have. What's important here is that we currently have multiplewalkimplementations, one for the AST, one for theSelectorParser, one for theValueParser.Sometimes, we also need to go up the tree in a depth-first manner. For that, we have
walkDepthimplementations.The funny thing is, all these implementations are very very similar, even the kinds of trees are very similar. They are just objects with
nodes: []as children.So this PR introduces a generic
walkfunction that can work on all of these trees.There are also some situations where you need to go down and back up the tree. For this reason, we added an
enterandexitphase:This means that you don't need to
walk(ast)and laterwalkDepth(ast)in case you wanted to do something after visiting all nodes.The API of these walk functions also slightly changed to fix some problems we've had before. One is the
replaceWithfunction. You could technically call it multiple times, but that doesn't make sense so instead you always have to return an explicitWalkAction. The possibilities are:To make sure that we can walk in both directions, and to make sure we have proper control over when to walk which nodes, the
walkfunction is implemented in an iterative manner using a stack instead of recursion.This also means that a
WalkAction.StoporWalkAction.ReplaceStopwill immediately stop the walk, without unwinding the entire call stack.Some notes:
contextnodes, for this we can build up the context lazily when we need it. I added acssContext(ctx)that gives you an enhanced context including thecontextobject that you can read information from.walkfunction can still be a normal function, which is equivalent to{ enter: fn }.Let's also take a look at some numbers. With this new implementation, each
walkis roughly ~1.3-1.5x faster than before. If you look at the memory usage (especially in Bun) we go from~2.2GBpeak memory usage, to~300mbpeak memory usage.Some benchmarks on small and big trees (M1 Max):
We also ran some benchmarks on @thecrypticace's M3 Max:
In node the memory difference isn't that big, but the performance itself is still better:
In summary:
walkimplementation for multiple use casesenterandexitphasesWalkActionpossibilities for better controlTest plan
walkwas used inside tests).walkimplementation