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

Optimize generated CSS output #14873

Merged
merged 7 commits into from
Nov 6, 2024
Merged

Optimize generated CSS output #14873

merged 7 commits into from
Nov 6, 2024

Conversation

RobinMalfait
Copy link
Member

This PR improves the generated CSS by running it through Lightning CSS twice.Right now Lightning CSS merges adjacent at-rules and at the end flattens the nesting. This means that after the nesting is flattened, the at-rules that are adjacent and could be merged together will not be merged.

This PR improves our output by running Lightning CSS twice on the generated CSS which will make sure to merge adjacent at-rules after the nesting is flattened.

Note: in the diff output you'll notice that some properties are duplicated. These need some fixes in Lightning CSS itself but they don't break anything for us right now.

Related PR in Lightning CSS for the double -webkit-backdrop-filter can be found here: parcel-bundler/lightningcss#850

@RobinMalfait RobinMalfait requested a review from a team as a code owner November 5, 2024 12:40
@adamwathan
Copy link
Member

Hard to tell from just the diff — is this change applied just to production builds or to dev and production? Also any chance you have benchmarked it on a realistic codebase? That would help to know if we should only do this in production or if it's fine to do it all the time.

@RobinMalfait
Copy link
Member Author

RobinMalfait commented Nov 5, 2024

It's always applied whenever we use Lightning CSS. For example in the CLI we don't use --minify or --optimize, which means that we won't hit this code path.

But if you do use --minify or --optimize then each optimize() call takes ~6ms when running it on Catalyst for example.

[5.50ms] optimize()
[5.59ms] optimize()
[11.10ms] optimizeCss()

@RobinMalfait RobinMalfait force-pushed the feat/optimize-output branch 3 times, most recently from 07f617d to dbf77e5 Compare November 6, 2024 11:11
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.

Any idea why the postcss snapshot now has three -webkit-text-decoration: inherit; on the a tag lol

I think this makes sense and I'd rather pay a small compile time fee of a few ms instead of expensive egress charges because the CSS can be more optimized 😅 We should explain with an example why we do this, though. transform(transform(foo)) looks like a typo otherwise 😄

CHANGELOG.md Outdated Show resolved Hide resolved

// Running Lightning CSS twice to ensure that adjacent rules are merged after
// nesting is applied. This creates a more optimized output.
return optimize(optimize(Buffer.from(input))).toString()
Copy link
Member

Choose a reason for hiding this comment

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

I wonder if we can get away with running optimize just once if lightningcss is configured as the CSS processor... It might have a different config so probably not 😬

Copy link
Member Author

Choose a reason for hiding this comment

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

Yeah that's tricky. Our config sets Safari to a version that doesn't understand nesting yet.

One suggestion that Devon had was to put @media queries on the outside instead of nested to do the merging, but the current change is a bit easier especially once Lightning CSS can handle this natively.

CHANGELOG.md Outdated Show resolved Hide resolved
RobinMalfait and others added 7 commits November 6, 2024 12:33
This will improve the output because Lightning CSS will first merge
adjacent rules, then flatten the nesting. After that it does not try to
merge adjacent rules anymore.
Co-authored-by: Philipp Spiess <hello@philippspiess.com>
@RobinMalfait RobinMalfait enabled auto-merge (squash) November 6, 2024 11:33
@RobinMalfait RobinMalfait merged commit c5b6df2 into next Nov 6, 2024
1 check passed
@RobinMalfait RobinMalfait deleted the feat/optimize-output branch November 6, 2024 11:39
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