-
-
Notifications
You must be signed in to change notification settings - Fork 6.2k
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
Conversation
Run & review this pull request in StackBlitz Codeflow. |
// workaround for windows since import("D:...") fails | ||
const sass: typeof Sass = createRequire(import.meta.url)(sassPath) |
There was a problem hiding this comment.
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
vite/packages/vite/src/node/plugins/css.ts
Lines 2048 to 2049 in 57f48f7
const sssPath = loadPreprocessorPath(PostCssDialectLang.sss, root) | |
cachedSss = createRequire(import.meta.url)(sssPath) |
There was a problem hiding this comment.
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)
There was a problem hiding this comment.
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
.
There was a problem hiding this comment.
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
forsugarss
dependency
We should definitely use import()
there, maybe in a separate PR. Good catch 👍
Hi, after switching to the modern api, I am meeting issue with 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 |
@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. |
@Mister-Hope It looks like
I have a reproduction using sass directly https://github.com/hi-ogawa/reproductions/tree/main/vite-17754-sass-absolute-file-reference Probably |
Description
Added sass compiler api integration (see
makeModernCompilerScssWorker
). Difference from other mode is that I didn't use Vite's own worker abstractionWorkerWithFallback
since I assumed Sass compiler API (at least onsass-embedded
?) already off-loads work from main process.todo
sass-embedded
optional peer dep + auto api selection