-
-
Notifications
You must be signed in to change notification settings - Fork 721
perf(transformer/styled-components): simplify CSS minification #12224
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
perf(transformer/styled-components): simplify CSS minification #12224
Conversation
How to use the Graphite Merge QueueAdd either label to this PR to merge it via the merge queue:
You must have a Graphite account in order to use the merge queue. Sign up using this link. An organization admin has enabled the Graphite Merge Queue in this repository. Please do not merge from GitHub as this will restart CI on PRs being processed by the merge queue. This stack of pull requests is managed by Graphite. Learn more about stacking. |
CodSpeed Instrumentation Performance ReportMerging #12224 will not alter performanceComparing Summary
|
5fbf952 to
2adcdea
Compare
2adcdea to
f14740e
Compare
This is what the |
|
As far as I can see, Babel plugin will not insert a space in place of the comment in We're doing it all in 1 pass, so I think we need to check for "symbols" ( But that's a separate issue from this PR. I don't think this PR changes behavior from what it was before. |
I see, I can't see any reason it would block us from removing this space. |
Merge activity
|
Simplify CSS minification.
* Remove the step of adding placeholders, which means we also don't need to remove them again, and don't need a hashmap to track which quasis are retained. Instead, just work through the quasis one by one.
* Remove the intermediate `new_raws` `Vec`.
* Skip `\\`, `\'`, `\"` in byte search loop, to avoid backtracking to check for `\"` when exiting a string.
The whole thing squashes down to a single function.
I *believe* I've kept the behavior exactly as it was before. Is there anything I'm missing?
There's one bit of the logic I don't understand. `/* ... */` is replaced with a space, unless there's a space preceding / following it. But I don't think we need to add a space in cases like `height:/* big */${10000}px`. Do we?
f933d20 to
8e13e2a
Compare
…ession in CSS minification (#13370) Fix handling block comments which contain an expression. Previously if that block comment ended exactly before another expression, the comment wouldn't be considered closed, and all remaining parts of the template literal would be removed. Input: ```js styled.div`${x}/* ${y} */${z}` ``` Previous output post-minification: ```js styled.div`${x}` ``` After this PR: ```js styled.div`${x}${z}` ``` This bug was introduced way back in #12224 (my fault!), but had gone unnoticed.

Simplify CSS minification.
new_rawsVec.\\,\',\"in byte search loop, to avoid backtracking to check for\"when exiting a string.The whole thing squashes down to a single function.
I believe I've kept the behavior exactly as it was before. Is there anything I'm missing?
There's one bit of the logic I don't understand.
/* ... */is replaced with a space, unless there's a space preceding / following it. But I don't think we need to add a space in cases likeheight:/* big */${10000}px. Do we?