perf(css): only run postcss when needed #19061
Conversation
sapphi-red
added
feat: css
p2-nice-to-have
| // postcss processing is not needed | ||
| if ( | ||
| lang === 'css' && | ||
| lang !== 'sss' && | ||
| !postcssConfig && | ||
| !isModule && | ||
| !needInlineImport && | ||
| !hasUrl | ||
| ) { | ||
| return { code, map: null } | ||
| return { code, map: preprocessorMap ?? null, deps } | ||
| } |
There was a problem hiding this comment.
With this PR, If the preprocessor removed all @import, we might be able to return at this line. (previously postcss was run)
| if ( | ||
| urlReplacer && | ||
| // if there's an @import, we need to add this plugin | ||
| // regradless of whether it contains url() or image-set(), | ||
| // because we don't know the content referenced by @import | ||
| (needInlineImport || cssUrlRE.test(code) || cssImageSetRE.test(code)) | ||
| ) { | ||
| postcssPlugins.push( | ||
| UrlRewritePostcssPlugin({ | ||
| replacer: urlReplacer, |
There was a problem hiding this comment.
UrlRewritePostcssPlugin was always pushed, so postcss was ran always.
| @@ -1428,7 +1433,7 @@ async function compileCSS( | |||
| ) | |||
| } | |||
|
|
|||
| if (!postcssPlugins.length) { | |||
| if (lang !== 'sss' && !postcssPlugins.length) { | |||
There was a problem hiding this comment.
This lang !== 'sss' was not needed because !postcssPlugins.length was always false.
There was a problem hiding this comment.
I'm a bit confused about this part. Isn't sugarss support added as a postcss plugin, so postcssPlugins.length should be at least one?
There was a problem hiding this comment.
It's because sugarss is a parser and not a plugin.
vite/packages/vite/src/node/plugins/css.ts
Line 1447 in da0caf5
Now that you mentioned about it, I think we should change this condition to
lang !== 'sss' && !postcssOptions.parser && !postcssPlugins.length.
There was a problem hiding this comment.
Ah I didn't know it's a parser. Yeah I think checking for parser seems better too.
| @@ -1509,7 +1516,6 @@ async function compileCSS( | |||
|
|
|||
| if (!devSourcemap) { | |||
| return { | |||
| ast: postcssResult, | |||
There was a problem hiding this comment.
ast was not used anywhere.
There was a problem hiding this comment.
The sourcemaps changed because postcss does not run on these files anymore.
|
/ecosystem-ci run |
commit: |
|
📝 Ran ecosystem CI on
✅ analogjs, astro, laravel, marko, nuxt, previewjs, quasar, qwik, rakkas, storybook, sveltekit, unocss, vike, vite-environment-examples, vite-plugin-pwa, vite-plugin-react, vite-plugin-react-swc, vite-plugin-vue, vite-setup-catalogue, vitepress, vuepress |
|
ladle fail is not related to this PR. vitest is failing because of the change in #19004. So it should be fine. |
| // if there's an @import, we need to add this plugin | ||
| // regradless of whether it contains url() or image-set(), | ||
| // because we don't know the content referenced by @import | ||
| (needInlineImport || cssUrlRE.test(code) || cssImageSetRE.test(code)) |
There was a problem hiding this comment.
| (needInlineImport || cssUrlRE.test(code) || cssImageSetRE.test(code)) | |
| (needInlineImport || hasUrl) |
It seems like we can reuse the variable here
Description