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

Switch to Lightning CSS #18177

Closed
wants to merge 8 commits into from
Closed

Conversation

KTibow
Copy link
Contributor

@KTibow KTibow commented Oct 10, 2023

Proposed change

  • Use Lightning CSS instead of clean-css to minify CSS.
  • Smaller bundles, eg -3kb from authorize.js
    Some bundles go up, because LCSS doesn't do some minifications, plus we started doing prefixing
  • Also, builds faster and is more compatible with old browsers
  • Is safe to use

    Lightning CSS does not perform any optimizations that change the behavior of your CSS unless it can prove that it is safe to do so. For example, only adjacent style rules are merged to avoid changing the order and potentially breaking the styles.

Currently a draft because I'm waiting for the LCSS team to respond to one of my issues to prove that the repo is active

Type of change

  • Dependency upgrade
  • Bugfix (non-breaking change which fixes an issue)
  • New feature (thank you!)
  • Breaking change (fix/feature causing existing functionality to break)
  • Code quality improvements to existing code or addition of tests

Additional information

N/A

Checklist

  • The code change is tested and works locally.
  • There is no commented out code in this PR.
  • Tests have been added to verify that the new code works.

@github-actions github-actions bot added Dependencies Pull requests that update a dependency file Build Related to building the code labels Oct 10, 2023
Copy link
Member

@steverep steverep left a comment

Choose a reason for hiding this comment

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

In general when comparing for something like this, it would be useful to know the impact on the whole bundle:

du --apparent-size --total -h hass_frontend/frontend_latest/*.js
du --apparent-size --total -h hass_frontend/frontend_es5/*.js

build-scripts/bundle.cjs Outdated Show resolved Hide resolved
build-scripts/bundle.cjs Outdated Show resolved Hide resolved
build-scripts/bundle.cjs Outdated Show resolved Hide resolved
@home-assistant
Copy link

Please take a look at the requested changes, and use the Ready for review button when you are done, thanks 👍

Learn more about our pull request process.

@home-assistant home-assistant bot marked this pull request as draft October 10, 2023 17:07
compatibility: "*,-properties.zeroUnits",
minifyCSS: (text, type) => {
const input = wrapCSS(text, type);
const { code } = lightningcss.transform({
Copy link
Member

Choose a reason for hiding this comment

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

Should also collect the warnings array here from output, and log them if not empty

Copy link
Contributor Author

@KTibow KTibow Oct 10, 2023

Choose a reason for hiding this comment

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

Hmm. I get warnings about Unsupported pseudo class or element: b2lopmc8akq whenever someone uses a variable inside of a css template literal, because one of the plugins replaces all variables with things like babel-plugin-template-html-minifier:b2lopmc8akq. I also get some warnings like "unknown rule @apply" wherever we use that.

Copy link
Member

Choose a reason for hiding this comment

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

That plugin is what's running html-minifier-terser on all the tagged templates. For debugging (if you already haven't), do dev builds with:

  1. Comment out the isProdBuild && in front of that plugin in the Babel options
  2. Change the devTool setting in webpack.cjs to get rid of eval-

After that, it should be much easier to find and inspect various templates to see what's happening. Wherever there are substitutions, technically there should be nothing but numbers or other CSS templates because that's all lit allows without unsafeCSS.

For @apply, I think you can add exceptions in the settings?

@KTibow
Copy link
Contributor Author

KTibow commented Oct 11, 2023

Oddly, while Lightning CSS is better in most cases (including the entrypoint files), it's producing a ~20kb bigger total output even when no targets are specified.

@KTibow KTibow force-pushed the patch-lightningcss branch from 878e279 to 919d224 Compare October 11, 2023 01:55
@KTibow
Copy link
Contributor Author

KTibow commented Oct 11, 2023

FYI to anyone else building this, I recently found out that you may need to do rm -rf node_modules/.cache in order to properly process the CSS.

@steverep
Copy link
Member

FYI to anyone else building this, I recently found out that you may need to do rm -rf node_modules/.cache in order to properly process the CSS.

That's probably the babel-loader cache getting a little confused. It's not a perfect system as it just uses the options object as part of the key.

@KTibow
Copy link
Contributor Author

KTibow commented Oct 11, 2023

Turns out that in a number of cases, LCSS doesn't do certain optimizations, which is why its bundle is larger:

  • Replace "outline: none" with "outline: 0"
  • Replace "color: var(--thing, white)" with "color: var(--thing, #fff)"
  • Removing px for svg elements

I'm going to leave this PR as a draft because I opened issues for those things in LCSS's repo. While I could do something like minifying with both CCSS and LCSS, that seems unwise.
* CCSS = clean-css, LCSS = lightningcss

@KTibow KTibow force-pushed the patch-lightningcss branch from 919d224 to e212e97 Compare October 12, 2023 02:26
@KTibow
Copy link
Contributor Author

KTibow commented Oct 12, 2023

Actually no. Something really weird is going on - while LCSS is worse in some cases, the total bundle should be 25kb smaller (according to tallying within the parsecss function), not 20kb bigger.

@KTibow KTibow force-pushed the patch-lightningcss branch from 4b158de to 6a37e22 Compare October 12, 2023 02:55
@KTibow
Copy link
Contributor Author

KTibow commented Oct 18, 2023

Closing as it's unlikely that my concerns will be addressed, and I can always recreate the PR later if they are

@KTibow KTibow closed this Oct 18, 2023
@github-actions github-actions bot locked and limited conversation to collaborators Oct 17, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
Build Related to building the code cla-signed Dependencies Pull requests that update a dependency file hacktoberfest
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants