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): support sass modern api #17728

Merged
merged 30 commits into from
Jul 30, 2024
Merged

Conversation

hi-ogawa
Copy link
Collaborator

@hi-ogawa hi-ogawa commented Jul 21, 2024

Description

This PR adds preprocessorOptions.scss.api: "modern" option to switch using sass.compileStringAsync API instead of legacy sass.render API.

I enabled api: "modern" in playground/css and existing tests are passing.

questions

  • should there be separate tests for "modern" and "legacy"?
    • I thought of adding a separate one for "modern", but probably what we want is to simply check playground/css tests pass, so I just flip the switch. maybe we should add a smaller playground for "legacy"?
  • any plan to switch modern as default? (in v6?)

In this PR, I only tackled the issue #7116, but I think there are somewhat related requests for overall sass integration experience. I'm wondering if these should be also considered for "modern" api integration, or they can be addressed separately later:

Copy link

stackblitz bot commented Jul 21, 2024

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

@hi-ogawa hi-ogawa marked this pull request as ready for review July 22, 2024 05:32
@bluwy bluwy changed the title feat: support sass modern api feat(css): support sass modern api Jul 23, 2024
@bluwy
Copy link
Member

bluwy commented Jul 23, 2024

  • should there be separate tests for "modern" and "legacy"?

I think like you suggested, we can try re-running the css playground while flipping the switch to modern/legacy so we don't have to rewrite the test.

  • any plan to switch modern as default? (in v6?)

I think we should migrate to modern in v6 for sure, even better to modern-compiler if it's stabilizes then.

I'm wondering if these should be also considered for "modern" api integration, or they can be addressed separately later:

I think we can tackle them later 👍

  • should we try integrating Compiler API?

We should!

  • allow switching sass implementation?

I think we can follow webpack by checking sass-embedded first, then sass. I don't think Vite ever supported node-sass. We can hold off exposing a new option to choose until there's a feature request.

@bluwy bluwy added feat: css p3-significant High priority enhancement (priority) labels Jul 23, 2024
@hi-ogawa
Copy link
Collaborator Author

  • should there be separate tests for "modern" and "legacy"?

I think like you suggested, we can try re-running the css playground while flipping the switch to modern/legacy so we don't have to rewrite the test.

This is possible for local manual testing by USE_LEGACY_SCSS=1 pnpm test-serve playground/css, but I'm actually not sure how to do that for CI.

So, maybe the simplest way would be to create something like playground/css-legacy-scss and copy some code from playground/css. What do you think?

@bluwy
Copy link
Member

bluwy commented Jul 23, 2024

I think we should try to avoid copying if possible. Maybe we can have a setup like this?

  1. In playground/css, create vite.config-scss-modern.js that imports the baseConfig from vite.config.js and sets api: 'modern'.
  2. Create __tests__/scss-modern/scss-modern.spec.ts which has import "../css.spec.ts"?

@hi-ogawa
Copy link
Collaborator Author

I think we should try to avoid copying if possible. Maybe we can have a setup like this?

  1. In playground/css, create vite.config-scss-modern.js that imports the baseConfig from vite.config.js and sets api: 'modern'.
  2. Create __tests__/scss-modern/scss-modern.spec.ts which has import "../css.spec.ts"?

Wow, I wasn't aware test runner automatically picks up vite config. This looks almost working, but file edit is probably affecting each other and hmr related tests are failing.

@hi-ogawa
Copy link
Collaborator Author

This looks almost working, but file edit is probably affecting each other and hmr related tests are failing.

I updated playground/vitestSetup.ts and vitestGlobalSetup.ts to allow having a dedicated testDir for variant tests. This doesn't look so elegant, but I guess this works?

@bluwy
Copy link
Member

bluwy commented Jul 23, 2024

It doesn't seem to slow down the test, and does make the test more isolated, so fine by me 👍

@yyx990803
Copy link
Member

Feedback from @nex3:

  • For the best performance it's ideal to use the Compiler API via sass-embedded. The API is stable and will be much more performant (and probably also doesn't need Vite to manage workers?)

  • The downside is that sass-embedded's platform support is limited to the range that Dart VM supports (also not sure if it works on e.g. StackBlitz), so it is recommended to still be able to fallback to the JS version.

Maybe Compiler API support can be a separate PR as an opt-in feature.

@nex3
Copy link

nex3 commented Jul 24, 2024

One option would be to list sass-embedded under optionalDependencies, so it'll get installed if it's available. Then you can check if it's loadable and use modern-compiler if it is and not if it's not. (I think there's some discussion of doing something similar by default in sass-loader as well.)

@hi-ogawa
Copy link
Collaborator Author

Thanks everyone for the review!

Regarding compiler API integration, I have another (draft) PR ready #17754. I didn't use Vite's own WorkerWithFallback system, assuming that wouldn't benefit if sass can already off loads work out of main process. I plan to compare performance like what @sapphi-red did in #13584 (comment) so we can decide on what to do after that.


One option would be to list sass-embedded under optionalDependencies, so it'll get installed if it's available. Then you can check if it's loadable and use modern-compiler if it is and not if it's not. (I think there's some discussion of doing something similar by default in sass-loader as well.)

@nex3 I think this matches with my expectation, but just to confirm, if Vite would have some heuristics to automatically pick the API, would you suggest something like this?

  • both sass and sass-embeeded is installed (for whatever reason) --> use modern-compiler with sass-embedded
  • only sass-embedded is installed --> use modern-compiler with sass-embedded
  • only sass is installed --> use compiler with sass
  • none is installed --> suggest installing sass-embedded

@yyx990803
Copy link
Member

Conclusion from today's team meeting:

  • API usage heuristics based on presence of sass-embedded and sass (precedence described by @hi-ogawa here)
  • The imports API surface is different between legacy and modern, so it is technically a breaking change
    • We are going to land modern API support in a 5.x minor behind an opt-in option, so users on Vite 5.x can still suppress possible deprecation warnings. This will be out in a few weeks.
    • Vite 6 will default to the modern API but supports users forcing the legacy API (marked deprecated).
    • Vite 7 will drop legacy API support

packages/vite/src/node/plugins/css.ts Outdated Show resolved Hide resolved
packages/vite/src/node/plugins/css.ts Outdated Show resolved Hide resolved
@bluwy bluwy added this to the 5.4 milestone Jul 24, 2024
@hi-ogawa
Copy link
Collaborator Author

hi-ogawa commented Jul 24, 2024

I made a performance comparison of 6 patterns (sass, sass-embedded) x (legacy, modern, modern-compiler) using ~100 sass files from https://github.com/vuetifyjs/vuetify. I'm not sure if this artificial usage represents a real use case well, but just having something to start with, let me share here. Btw, I'm using a local build of the next PR #17754.

code: https://github.com/hi-ogawa/test-vite-sass/tree/main/vuetify

(sass/sass-embedded) (modern-compiler/modern/legacy) pnpm build time
sass-embedded modern-compiler 441ms
sass-embedded modern 575ms
sass-embedded legacy 1.38s
sass modern-compiler 2.78s
sass modern 2.75s (1.76s with max workers)
sass legacy 3.42s (2.04s with max workers)
(baseline without vuetify imports) (53ms)

@nex3
Copy link

nex3 commented Jul 24, 2024

@nex3 I think this matches with my expectation, but just to confirm, if Vite would have some heuristics to automatically pick the API, would you suggest something like this?

  • both sass and sass-embeeded is installed (for whatever reason) --> use modern-compiler with sass-embedded
  • only sass-embedded is installed --> use modern-compiler with sass-embedded
  • only sass is installed --> use compiler with sass
  • none is installed --> suggest installing sass-embedded

You can use modern-compiler no matter what—it won't impose any penalty if you're using it with sass, it just won't provide a particular benefit either. Otherwise this looks right.

bluwy
bluwy previously approved these changes Jul 25, 2024
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.

Use the modern SASS compiler API
4 participants