-
-
Notifications
You must be signed in to change notification settings - Fork 4.7k
Show Lightning CSS warnings when optimizing/minifying in production #18918
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
We can't properly map them to the input, so I think that he line numbers will only be more confusing
7f49a35
to
e3234bf
Compare
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.
Let's not add a new dependency, especially when it's only like 30 LOC
@@ -1,6 +1,7 @@ | |||
import remapping from '@jridgewell/remapping' | |||
import { Features, transform } from 'lightningcss' | |||
import MagicString from 'magic-string' | |||
import pc from 'picocolors' |
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.
Can we inline this dependency? https://unpkg.com/[email protected]/picocolors.js
Probably don't even need all of the options 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.
Inlined it 👍
Only reason I added the dependency is because we were already using it in other packages (@tailwindcss/cli
and @tailwindcss/upgrade
). But since we don't use it in the @tailwindcss/postcss
or @tailwindcss/vite
, you're right let's just inline them. If they were used there already it's not a big deal.
Also I think we probably want a way to silence these warnings, WDYT? Have you tried running e.g. tailwindcss.com CSS through it? Will it cause any unexpected warnings? |
You sure? Running it on Tailwind Plus or tailwindcss.com doesn't result in any warnings. The warnings also only show up when using Lightning CSS which is typically only when you explicitly have |
Yeah that sounds good then 👍 |
pnpm-lock.yaml
Outdated
@@ -6,39 +6,9 @@ settings: | |||
|
|||
catalogs: | |||
default: | |||
'@types/node': |
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.
Cleaning this up as well because nothing should change here now that I removed the dependency again.
This PR improves the DX by showing all the Lightning CSS warnings when using a "production" build (or using
--optimize
or--minify
flags when using the CLI).Right now Tailwind CSS itself doesn't care about the exact syntax you are using in the CSS as long as it looks valid. We do this because otherwise we would have to parse a lot more CSS syntax and validate it even though it would be valid CSS in 99.99% of the cases.
Even worse, if you want to use newer CSS syntax that Tailwind CSS doesn't validate yet, then you would get warnings for valid CSS.
Another reason why we don't do this is because the browser already does a great job at ignoring invalid CSS.
So the linked issue #15872 would still silently fail in development mode. In this case, everything would work, except the shadow with the invalid syntax.
But in production mode, you would now get a proper warning from Lightning CSS, because they try to optimize the CSS and remove invalid CSS.
One potential issue here is that we run Lightning CSS on the generated CSS, not on the input CSS. So the current output shows the warnings in the output CSS not the input CSS. Any thoughts if we would just skip the line numbers?
Test plan
@tailwindcss/node
so the CLI/Vite/PostCSS plugins would all get the same behavior.Screenshots:
If you have a single issue:

If you have multiple issues:

Fixes: #15872