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 compiler api and sass-embedded package #17754

Merged
merged 59 commits into from
Jul 31, 2024

Conversation

hi-ogawa
Copy link
Collaborator

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

Description

Added sass compiler api integration (see makeModernCompilerScssWorker). Difference from other mode is that I didn't use Vite's own worker abstraction WorkerWithFallback since I assumed Sass compiler API (at least on sass-embedded?) already off-loads work from main process.

todo

Copy link

stackblitz bot commented Jul 24, 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 30, 2024 07:48
Comment on lines 2313 to 2314
// workaround for windows since import("D:...") fails
const sass: typeof Sass = createRequire(import.meta.url)(sassPath)
Copy link
Collaborator Author

@hi-ogawa hi-ogawa Jul 30, 2024

Choose a reason for hiding this comment

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

I had import(sassPath) but it was failing on Windows due to drive D: prefix. Using require to just workaround this seems pretty odd. Does anyone know a safe way to normalize path into file:... for node esm?

btw, there is also createRequire for sugarss dependency

const sssPath = loadPreprocessorPath(PostCssDialectLang.sss, root)
cachedSss = createRequire(import.meta.url)(sssPath)

Copy link
Contributor

Choose a reason for hiding this comment

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

Maybe using pathToFileURL from node:url:
cachedSss = await import(sssPath.match(/^[a-z]:/i) ? pathToFileURL(sssPath).href : sssPath)

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

That might be it. I remembered Vitest had similar Windows hack somewhere and probably this one https://github.com/vitest-dev/vitest/blob/5d6d8013371b46522ff55cb64015d29e617994f2/packages/vite-node/src/client.ts#L368-L369
Linked issue on Node nodejs/node#31710 seems to suggest pathToFileURL(sssPath).href.

Copy link
Member

Choose a reason for hiding this comment

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

btw, there is also createRequire for sugarss dependency

We should definitely use import() there, maybe in a separate PR. Good catch 👍

packages/vite/src/node/plugins/css.ts Outdated Show resolved Hide resolved
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 merged commit 1025bb6 into vitejs:main Jul 31, 2024
11 checks passed
@hi-ogawa hi-ogawa deleted the feat-sass-compiler branch August 1, 2024 00:06
@tomleo tomleo mentioned this pull request Aug 12, 2024
8 tasks
@Mister-Hope
Copy link

Hi, after switching to the modern api, I am meeting issue with file:// imports with our vuepress project:

image

It's reporting can not find a file, but it exisited.

Since we both support webpack and vite, and webpack is working fine (also I am sure native sass is working fine), so I doubt the internalImporter in vite is working as expected? Could some related test about file:// imports being added about the new api?

@patak-dev
Copy link
Member

@Mister-Hope would you create a new issue with a minimal reproduction? Your comment here may quickly get lost and will not be properly tracked.

@hi-ogawa
Copy link
Collaborator Author

@Mister-Hope It looks like file: not being supported out-of-the-box is a known breaking change in modern api

I have a reproduction using sass directly https://github.com/hi-ogawa/reproductions/tree/main/vite-17754-sass-absolute-file-reference Probably file: resolution needs to be handled by integration side now. I'll look into it.

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

Successfully merging this pull request may close these issues.

Use the Sass Compiler API Allow usage of sass-embedded instead of sass package
5 participants