-
-
Notifications
You must be signed in to change notification settings - Fork 6.1k
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
fix!: default build.cssMinify
to 'esbuild'
for SSR
#15637
base: main
Are you sure you want to change the base?
fix!: default build.cssMinify
to 'esbuild'
for SSR
#15637
Conversation
Run & review this pull request in StackBlitz Codeflow. |
@sapphi-red you marked this as a breaking change, would we still be able to merge it for Vite 5.1? Vite 6 is too far away right now. |
Ah, ok, I see: I think we could still merge this one in Vite 5.1, but maybe I'm missing something here. |
I think the change has a slightly big impact. Given that there's a workaround, I think it's better to wait for Vite 6. |
Should we also only minify only if |
Even if |
I don't quite understand how one would compare the same file name between client and server if |
Ah I guess checking the Remix issue, it's because the hash is exposed for .css?url 🤔 |
Yep, the the hash is exposed for .css?url. The URL on the server points to nothing. |
@@ -451,7 +451,7 @@ export function resolveBuildEnvironmentOptions( | |||
} | |||
|
|||
if (resolved.cssMinify == null) { | |||
resolved.cssMinify = !!resolved.minify | |||
resolved.cssMinify = raw.ssr ? 'esbuild' : !!resolved.minify |
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.
I'm using raw.ssr
to align with minify: raw.ssr ? false : 'esbuild',
above. Do we want to update that to consumer === 'server'
? I guess we don't want to, because we would want to enable minify for edge runtimes.
Description
Even for SSR, there's no reason to skip minify for CSS files.
SvelteKit and Astro sets this.
https://github.com/sveltejs/kit/blob/6200a48befe538176b1f5c1cb9e32c51af84345b/packages/kit/src/exports/vite/index.js#L607
https://github.com/withastro/astro/blob/7e1db8b4ce2da9e044ea0393e533c6db2561ac90/packages/astro/src/core/build/static-build.ts#L178-L180
Additional context
What is the purpose of this pull request?
Before submitting the PR, please make sure you do the following
fixes #123
).