vite icon indicating copy to clipboard operation
vite copied to clipboard

Published 20 hours ago • verified publisher icon vitejs

feat: support postcss sugarss

Open vitalybaev opened this issue 3 years ago • 10 comments

Description

Hi, this PR adds support for handling SugarSS - Indent-based CSS syntax for PostCSS.

Additional context

At first, I had taken a look at #2436, but I've chosen a different approach. Instead of providing additional context and use it in postcss.config.js, I think it's better to treat .sss files as regular PostCSS case, but automatically change parser to sugarss.

What is the purpose of this pull request?

  • [ ] Bug fix
  • [x] New Feature
  • [ ] Documentation update
  • [ ] Other

Before submitting the PR, please make sure you do the following

  • [x] Read the Contributing Guidelines.
  • [x] Read the Pull Request Guidelines and follow the Commit Convention.
  • [x] Check that there isn't already a PR that solves the problem the same way to avoid creating a duplicate.
  • [x] Provide a description in this PR that addresses what the PR is solving, or reference the issue that it solves (e.g. fixes #123).
  • [x] Ideally, include relevant tests that fail without this PR but pass with it.

vitalybaev avatar Jan 31 '22 23:01 vitalybaev

Thanks for the PR, IMO the added complexity is ok given how popular SugarSS is. We'll be discussing the feat in our next team meeting.

patak-dev avatar Feb 09 '22 21:02 patak-dev

@patak-dev any updates on this?

vitalybaev avatar Mar 14 '22 20:03 vitalybaev

Sorry, we got a bit backlogged for new features. I hope we can get to it soon.

patak-dev avatar Mar 14 '22 20:03 patak-dev

@vitalybaev sorry if I ask something obvious, does this pr allow to use <style lang="sss"></style> inside .vue files?

https://github.com/vitejs/vite/pull/6705/files#diff-2cfbd4f4d8c32727cd8e1a561cffbde0b384a3ce0789340440e144f9d64c10f6R88

ksevelyar avatar Apr 14 '22 22:04 ksevelyar

@ksevelyar I don't know Vue.js well actually and how it resolves types of content from its files. But it seems that the answer is yes - this PR should work with <style lang="sss"></style>

I can test it locally and share the result

vitalybaev avatar Apr 19 '22 06:04 vitalybaev

@vitalybaev would you be so kind and rebase this PR? In today's meeting we discussed this and might want to merge it

Shinigami92 avatar Sep 09 '22 13:09 Shinigami92

@vitalybaev to add a bit more context, the PR looks good to move forward but the team doesn't have experience with sugars. So let's merge it after it is rebased and we can include it in Vite 3.2, we may ping you if there are issues related to sugarss moving forward if that sounds good. Sorry it took us this long to get back to you.

patak-dev avatar Sep 09 '22 14:09 patak-dev

@patak-dev @Shinigami92 glad to hear that!

I've rebased my PR, and it now fails only on test-serve, not only for SugarSS but also for other CSS tests. But test-build works fine and running playground/css as dev also works fine 🤔 I'll try to find an issue

vitalybaev avatar Sep 09 '22 16:09 vitalybaev

I need more time since something has changed in Vite itself. Maybe you could help.

So, the main idea behind my PR is to trait .sss as PostCSS files but with explicit parser: "sugarss" option set. Everything works fine (if you run pnpm run dev in playground/css you'll see correct processing of SugarSS files). Building also works fine. But tests work differ - test-build passes successfully, but test-serve failed. Moreover, importing sugarss.sss in main.js entry point inplayground/css breaks not only SugarSS tests, but some other tests in css.spec.ts.

So, I'm going to deep dive and debug what causes these problems.

vitalybaev avatar Sep 12 '22 12:09 vitalybaev

If you want to create a new PR and close this one, please link the new one here, so I can keep track

Shinigami92 avatar Sep 12 '22 13:09 Shinigami92

@Shinigami92 I believe it's not due to the conflicts or wrong rebase. But I tried to replicate these changes onto the latest main but to no avail.

What I can confirm:

  1. compileCSS works fine. At least if you pass .sss file there, you'll get correctly transpiled CSS.
  2. pnpm run test-build passes all tests successfully, but
  3. pnpm run test-serve fails with a bunch of failed tests. Not only regarding SugarSS, but to other CSS tests.
  4. But if you serve CSS playground via pnpm run dev from playground/css directory - you'll see correct styles applied.
  5. I also can confirm that importing .sss file breaks tests.
  6. Some tests (for example, SugarSS tests) pass successfully when they are executed separately.

So, I need to deep dive more. There must be some side effects that break things.

🤯🤯🤯

vitalybaev avatar Sep 16 '22 10:09 vitalybaev

Oh well, the cause of the problem was updating parser option in the PostCSS options config, which is indeed just a mutation of the original config that can be used with other files.

So, I've updated resolvePostcssConfig implementation to distinguish cached configs also by dialect. IDK whether this is an appropriate way to solve this, but it works correctly.

vitalybaev avatar Sep 16 '22 16:09 vitalybaev

But there is an issue with DX for pnpm users - due to the fact that sugarss has postcss as a peer dependency, we have to encourage users to install not only sugarss package but also postcss despite having postcss already in Vite itself (it's a transitive dependency, so pnpm doesn't allow us to use it).

So it should be noticed in the docs.

vitalybaev avatar Sep 16 '22 16:09 vitalybaev

@Shinigami92 @patak-dev Have you found time to take a look at my latest fixes?

vitalybaev avatar Sep 20 '22 18:09 vitalybaev

@Shinigami92 I've rebased this PR again, could you please review it when you'll have time

vitalybaev avatar Sep 24 '22 15:09 vitalybaev

@patak-dev @Shinigami92 is there anything to be done here? It also still has needs rebase label

vitalybaev avatar Sep 28 '22 14:09 vitalybaev

Thanks for your patience @vitalybaev! Let's release this one as part of Vite 3.2

patak-dev avatar Sep 30 '22 09:09 patak-dev