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

feat(css): add support for Lightning CSS #12807

Merged
merged 8 commits into from Jun 19, 2023
Merged

feat(css): add support for Lightning CSS #12807

merged 8 commits into from Jun 19, 2023

Conversation

ArnaudBarre
Copy link
Member

@ArnaudBarre ArnaudBarre commented Apr 10, 2023

Fixes #11029

Based on an initial work from @devongovett

I made the choice to make it incompatible with preprocessor. With the current state of CSS (vars, nesting) and modules supported by Lightning CSS, adding a preprocessor will juts hurt performances for very little (negative?) DX.

@stackblitz
Copy link

stackblitz bot commented Apr 10, 2023

Review PR in StackBlitz Codeflow Run & review this pull request in StackBlitz Codeflow.

Copy link

@devongovett devongovett left a comment

Choose a reason for hiding this comment

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

Nice work! 🥳

packages/vite/src/node/plugins/css.ts Outdated Show resolved Hide resolved
default:
throw new Error(`Unsupported dependency type: ${dep.type}`)
}
}

Choose a reason for hiding this comment

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

One thing that might be worth trying would be to use the visitor API to do this instead of doing string replacement afterward (using analyzeDependencies). You should test the perf and see which is faster. Here's an example.

Copy link
Member Author

Choose a reason for hiding this comment

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

I will try it out. A nice thing it that this would give correct source map no?

Choose a reason for hiding this comment

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

Yes

Copy link
Member Author

Choose a reason for hiding this comment

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

I looked at it but the issue is that the function is async (and it's rooted deep into shared resolve logic being async in Vite)

packages/vite/src/node/plugins/css.ts Outdated Show resolved Hide resolved
@PuruVJ
Copy link

PuruVJ commented Apr 21, 2023

Looks awesome! Great work.

I just have one request to keep preprocessor support. Sass/less is non negotiable for me, and many consumers of vite as well.

If performance isn't that improved with preprocessor, so be it. It's the consumer's choice. But removing preprocessors entirely will break many apps and projects.

Again, thanks for this amazing improvement to Vite. Can't wait to test it out

@Razunter
Copy link

Useless for me without preprocessor support... Also, it will be confusing, sometimes it works, sometimes it doesn't?

@ArnaudBarre
Copy link
Member Author

ArnaudBarre commented Apr 22, 2023

@PuruVJ what's your need for using a pre-processor and Lightning CSS as a transformer ? (the option will be provided to use it a minifier only because not everyone will be able to migrate to it due the high usage of Tailwind)

@@ -828,6 +895,10 @@ async function compileCSS(
modules?: Record<string, string>
deps?: Set<string>
}> {
if (config.css?.transformer === 'LightningCSS') {
return compileLightningCSS(id, code, config, config.css, urlReplacer)

Choose a reason for hiding this comment

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

Should it be possible to use both Lightning CSS and PostCSS/SASS/Less together? In my prototype, I ran those pre-processors first (if needed), and then fed the output into Lightning CSS. That would make it possible to use things like Tailwind, but still use Lightning CSS for other transforms instead of autoprefixer for example.

Copy link
Member Author

Choose a reason for hiding this comment

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

This can be done in build to get better minification. I'm not sure of the benefit in dev (compare to the complexity of not messing up with sourcemaps for example)

Choose a reason for hiding this comment

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

Lightning CSS accepts an input source map so you can pass the source map from postcss/sass in and it'll remap the output. Aside from that, is there any reason not to support it? Lightning CSS has different ways of compiling modern CSS features for older browsers than postcss-preset-env/autoprefixer, for example, and even includes support for some features that other compilers don't. Until recently, Lightning CSS was the only tool supporting relative color syntax, for example. Users may also want to use custom Lightning CSS transform plugins.

I think limiting it to be only Lightning CSS or PostCSS/Sass will severely limit the adoption, since so many people use Tailwind. I think it will be useful to give users the choice of what combination of tools they want to use.

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 relative color syntax is a good example.

The main reason for not adding it for now was to reduce the API surface and so the number of bugs that then we have to consider. On top of that the API would not be as trivial (for example on how to be explicit on CSS modules being handled by one or the other). And in the future we will able to use Tailwind without PostCSS.

What's your opinion @patak-dev?

Copy link
Member

Choose a reason for hiding this comment

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

I'm also leaning toward starting with Lightning CSS without preprocessing. But I think it is a case we should think about up front and design the config API to allow us to go there later. Using postcss as a preprocessor for lightning CSS, we could do it as:

css: {
  transformer: 'lightingcss',
  lightningcss: { ... },
  postcss: { ... }
}

Or maybe allow postcss options as preprocessorOptions.postcss?

Copy link
Member Author

Choose a reason for hiding this comment

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

For now we are on the side of not being able to use pre-processor & LightningCSS. The elephant in the room is Tailwind, but given the fact that Tailwind without postcss is something coming and UnoCSS can give you the same experience without the PostCSS requirement (for people like liking the config part with fixed number of possibilities, I built this)

@patak-dev patak-dev added feat: css p2-nice-to-have Not breaking anything but nice to have (priority) p3-significant High priority enhancement (priority) and removed p2-nice-to-have Not breaking anything but nice to have (priority) labels May 18, 2023
@ArnaudBarre ArnaudBarre added this to the 4.4 milestone May 31, 2023
packages/vite/src/node/build.ts Outdated Show resolved Hide resolved
packages/vite/src/node/plugins/css.ts Outdated Show resolved Hide resolved
packages/vite/src/node/build.ts Outdated Show resolved Hide resolved
@patak-dev
Copy link
Member

/ecosystem-ci run

@vite-ecosystem-ci
Copy link

vite-ecosystem-ci bot commented Jun 15, 2023

📝 Ran ecosystem CI: Open

suite result
analogjs ⏹️ cancelled
astro ✅ success
histoire ❌ failure
iles ❌ failure
ladle ✅ success
laravel ✅ success
marko ✅ success
nuxt ✅ success
nx ✅ success
previewjs ✅ success
qwik ✅ success
rakkas ✅ success
sveltekit ✅ success
unocss ✅ success
vite-plugin-pwa ✅ success
vite-plugin-ssr ✅ success
vite-plugin-react ✅ success
vite-plugin-react-pages ✅ success
vite-plugin-react-swc ✅ success
vite-plugin-svelte ✅ success
vite-plugin-vue ✅ success
vite-setup-catalogue ✅ success
vitepress ✅ success
vitest ✅ success

patak-dev
patak-dev previously approved these changes Jun 15, 2023
@patak-dev
Copy link
Member

Thanks for implementing this @ArnaudBarre! And thanks @devongovett for the initial POC and for pushing us to try the lib out. Let's merge this PR as we discussed Arnaud so we can let users test it in 4.4. Let me know if you intend to send a follow-up PR with some minimal docs, I'll do it if not.

@patak-dev patak-dev merged commit c6c5d49 into main Jun 19, 2023
13 checks passed
@patak-dev patak-dev deleted the lightningcss branch June 19, 2023 13:46
@patak-dev
Copy link
Member

If you have feedback about Lightning CSS and the experimental API added in this PR, please share it at:

We'll check if this feature could be moved out of experimental in Vite 5 based on it.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
feat: css p3-significant High priority enhancement (priority)
Projects
Archived in project
Development

Successfully merging this pull request may close these issues.

Explore using Lightning CSS instead of PostCSS in core (or as an option)
7 participants