feat(css): support sass modern api by hi-ogawa · Pull Request #17728 · vitejs/vite

hi-ogawa · 2024-07-21T06:58:21Z

Description

closes Use the modern SASS compiler API #7116
(supersedes) closes feat: support SASS modern api #7170

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:

should we try integrating Compiler API? (cf. Use the Sass Compiler API #14689)
- webpack supports this by api: "modern-compiler" https://github.com/webpack-contrib/sass-loader?tab=readme-ov-file#api
allow switching sass implementation? (cf. Allow usage of sass-embedded instead of sass package #6734)
- it looks like webpack checks package.json to automatically pick sass, node-sass, sass-embedded or allow explicitly choosing from implementation option https://github.com/webpack-contrib/sass-loader?tab=readme-ov-file#implementation

bolt-new-by-stackblitz · 2024-07-21T06:58:24Z

Run & review this pull request in StackBlitz Codeflow.

packages/vite/src/node/plugins/css.ts

bluwy · 2024-07-23T03:20:29Z

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.

hi-ogawa · 2024-07-23T04:43:40Z

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 · 2024-07-23T05:13:37Z

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

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

hi-ogawa · 2024-07-23T05:52:23Z

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

In playground/css, create vite.config-scss-modern.js that imports the baseConfig from vite.config.js and sets api: 'modern'.

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 · 2024-07-23T07:29:50Z

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 · 2024-07-23T07:35:37Z

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

yyx990803 · 2024-07-24T01:22:17Z

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 · 2024-07-24T01:31:21Z

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 · 2024-07-24T02:12:15Z

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 · 2024-07-24T07:36:08Z

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

hi-ogawa · 2024-07-24T10:43:11Z

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 · 2024-07-24T20:47:05Z

@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.

Other changes: - Ace: Fix AceRange import to match new Ace exports - Remove Vite SCSS preprocessing option, as it is now the default since Vite 6.x (vitejs/vite#17728, vitejs/vite#7116).

hi-ogawa marked this pull request as ready for review July 22, 2024 05:32

bluwy reviewed Jul 23, 2024

View reviewed changes

packages/vite/src/node/plugins/css.ts Outdated Show resolved Hide resolved

bluwy changed the title ~~feat: support sass modern api~~ feat(css): support sass modern api Jul 23, 2024

bluwy added feat: css p3-significant High priority enhancement (priority) labels Jul 23, 2024

hi-ogawa mentioned this pull request Jul 24, 2024

feat(css): support sass compiler api and sass-embedded package #17754

Merged

4 tasks

bluwy reviewed Jul 24, 2024

View reviewed changes

packages/vite/src/node/plugins/css.ts Outdated Show resolved Hide resolved

packages/vite/src/node/plugins/css.ts Outdated Show resolved Hide resolved

hi-ogawa added 12 commits July 24, 2024 17:03

feat: support sass modern api

4948f86

test: add playground

0fea9c4

wip: sassInternalImporter

2feb149

test: add custom importer example

e572b9f

test: test internal importer

6619ade

chore: cleanup playground

fba1d4d

wip: rebaseUrls

cbfe70d

test: test "modern" in playground/css

b515950

chore: cleanup

f04f067

debug

f8154b1

fix: fix stats.includedFiles

1d5ce19

chore: cleanup

af7e6dc

hi-ogawa added 8 commits July 24, 2024 17:03

ci: test sass legacy

96a7336

ci: filter more

c2a6256

test: split vite config

35bf91a

chore: no console

a9a7038

test: separate testDir for variant tests

ddd3c14

chore: cleanup

6adda0e

ci: cleanup

6e847c0

refactor: fsp + remove toLowerCase

3bf2dc1

hi-ogawa force-pushed the feat-sass-modern branch from d2395c9 to 3bf2dc1 Compare July 24, 2024 08:08

bluwy added this to the 5.4 milestone Jul 24, 2024

sapphi-red mentioned this pull request Jul 24, 2024

css.preprocessorMaxWorkers = true hi-ogawa/test-vite-sass#1

Open

bluwy previously approved these changes Jul 25, 2024

View reviewed changes

meteorlxy mentioned this pull request Jul 25, 2024

feat(bundler-webpack): add api field in sass-loader option type vuepress/core#1588

Merged

hi-ogawa mentioned this pull request Jul 29, 2024

Dependency Pre-Bundling does not bundle styles #7719

Open

7 tasks

hi-ogawa added 2 commits July 30, 2024 08:51

Merge branch 'main' into feat-sass-modern

2125ef6

fix: fix fs, path

c1df7b6

hi-ogawa dismissed bluwy’s stale review via c1df7b6 July 29, 2024 23:52

bluwy approved these changes Jul 30, 2024

View reviewed changes

bluwy merged commit 73a3de0 into vitejs:main Jul 30, 2024

hi-ogawa deleted the feat-sass-modern branch July 30, 2024 05:59

userquin mentioned this pull request Aug 15, 2024

docs: include modern sass vite options vuetifyjs/vuetify#20325

Merged

hi-ogawa mentioned this pull request Aug 24, 2024

feat(css)!: change default sass api to modern/modern-compiler #17937

Merged

2 tasks

Illawind mentioned this pull request Sep 20, 2024

Legacy JS API is deprecated for sass rezozero/nuxt-starter#66

Closed

xiaoxian521 mentioned this pull request Nov 7, 2024

refactor: 升级vite至v6版本，升级sass至最新版，重构主题写法，弃用@pureadmin/theme pure-admin/vue-pure-admin#1188

Merged

sapphi-red mentioned this pull request May 1, 2025

refactor(css)!: remove sass legacy API support #19977

Merged

morganney mentioned this pull request Jun 5, 2025

css.preprocessorOptions type error with sass importers #20150

Closed

7 tasks

Uh oh!

Comments

Conversation

hi-ogawa commented Jul 21, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Description

Uh oh!

bolt-new-by-stackblitz bot commented Jul 21, 2024

Uh oh!

Uh oh!

bluwy commented Jul 23, 2024

Uh oh!

hi-ogawa commented Jul 23, 2024

Uh oh!

bluwy commented Jul 23, 2024

Uh oh!

hi-ogawa commented Jul 23, 2024

Uh oh!

hi-ogawa commented Jul 23, 2024

Uh oh!

bluwy commented Jul 23, 2024

Uh oh!

yyx990803 commented Jul 24, 2024

Uh oh!

nex3 commented Jul 24, 2024

Uh oh!

hi-ogawa commented Jul 24, 2024

Uh oh!

yyx990803 commented Jul 24, 2024

Uh oh!

Uh oh!

Uh oh!

hi-ogawa commented Jul 24, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

nex3 commented Jul 24, 2024

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

hi-ogawa commented Jul 21, 2024 •

edited

Loading

hi-ogawa commented Jul 24, 2024 •

edited

Loading