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

fix(vite): remove postcss-url and duplicate postcss-import #23861

Merged
merged 5 commits into from Oct 25, 2023

Conversation

danielroe
Copy link
Member

@danielroe danielroe commented Oct 22, 2023

πŸ”— Linked issue

resolves #15765

❓ Type of change

  • πŸ“– Documentation (updates to the documentation, readme or JSdoc annotations)
  • 🐞 Bug fix (a non-breaking change that fixes an issue)
  • πŸ‘Œ Enhancement (improving an existing functionality like performance)
  • ✨ New feature (a non-breaking change that adds functionality)
  • 🧹 Chore (updates to the build process or auxiliary tools and libraries)
  • ⚠️ Breaking change (fix or feature that would cause existing functionality to change)

πŸ“š Description

This was a fun deep-dive.

Investigating the linked issue, I found that we are adding postcss-import - and so is vite. In addition, I believe the key features postcss-url (which was causing the bug) are in fact already covered by Vite's list of features:

Vite improves @import resolving for Sass and Less so that Vite aliases are also respected. In addition, relative url() references inside imported Sass/Less files that are in different directories from the root file are also automatically rebased to ensure correctness.
https://vitejs.dev/guide/features.html#css-pre-processors

Accordingly, this PR removes both plugins (or rather, doesn't add them).

I suspect removing these two plugins will improve our performance as well as fix the bug. I would also consider we reconsider whether to include cssnano or defer to vite's esbuild/lightningcss minificiation step in future (cc: antfu).

πŸ“ Checklist

  • I have linked an issue or discussion.
  • I have updated the documentation accordingly.

@stackblitz
Copy link

stackblitz bot commented Oct 22, 2023

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

@danielroe danielroe marked this pull request as draft October 22, 2023 12:53
@danielroe danielroe marked this pull request as ready for review October 23, 2023 06:11
@danielroe danielroe requested a review from pi0 October 23, 2023 09:50
@mrleblanc101
Copy link

Shouldn't this mention/link to #20533 since all the related issue were merged into that one ?

@antfu
Copy link
Member

antfu commented Oct 24, 2023

For CSS minification, I am not sure if users would want full control over which tool to use, maybe could be an option as 'cssnano' | 'esbuild' | 'lightningcss' where we built a thin layer of adapter?

@danielroe danielroe changed the title fix(vite): remove postcss-url and duplicate postcss-import plugins fix(vite): remove postcss-url and duplicate postcss-import Oct 25, 2023
@danielroe danielroe merged commit 8e44395 into main Oct 25, 2023
34 checks passed
@danielroe danielroe deleted the fix/vite-postcss-plugins branch October 25, 2023 00:38
@danielroe
Copy link
Member Author

I think that would be a very nice idea.

@github-actions github-actions bot mentioned this pull request Oct 25, 2023
@nathanchase
Copy link
Contributor

nathanchase commented Nov 6, 2023

@danielroe @antfu

For CSS minification, I am not sure if users would want full control over which tool to use, maybe could be an option as 'cssnano' | 'esbuild' | 'lightningcss' where we built a thin layer of adapter?

Full control is good, but also with the most performant (sensible) default (likely lightningcss). I was recently looking at whether or not something like nuxt-modules/critters or nuxt-beastcss was even necessary, or if Nuxt 3 already accomplishes the same goals.

There's not much in the docs other than Nuxt is pre-configured to use cssnano for "minification and purge" (at the end of "Using Postcss") - but nothing specifically about best practices of inlining critical css.

I know there's the inlineSSRStyles experimental flag defaulting to true, but unsure if that's doing the same thing that critters / beastcss are or not.

Anyway, ideally Nuxt 3 takes care of all of the above for us (inlined critical CSS, fastest minification and purging), and maybe explains the optimal pre-configured choice (lightning) - but then lets us opt for other less performant options if it's somehow a project requirement.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Load a external font failed the server response is incorrect
6 participants