Skip to content

fix(css): avoid using default export from sass-embedded#19053

Closed
cod1r wants to merge 3 commits intovitejs:mainfrom
cod1r:fix-sass-dynamic-import
Closed

fix(css): avoid using default export from sass-embedded#19053
cod1r wants to merge 3 commits intovitejs:mainfrom
cod1r:fix-sass-dynamic-import

Conversation

@cod1r
Copy link

@cod1r cod1r commented Dec 23, 2024

fixes #19052

I pray that this doesn't mess anything else up :)

@cod1r cod1r changed the title fix: remove default property access from sass-embedded dynamic import fix(build): remove default property access from sass-embedded dynamic import Dec 23, 2024
@sapphi-red sapphi-red changed the title fix(build): remove default property access from sass-embedded dynamic import fix(css): avoid using default export from sass-embedded Dec 24, 2024
@sapphi-red sapphi-red added feat: css p2-edge-case Bug, but has workaround or limited in scope (priority) labels Dec 24, 2024
sapphi-red
sapphi-red previously approved these changes Dec 24, 2024
Copy link
Member

@sapphi-red sapphi-red left a comment

Choose a reason for hiding this comment

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

While I wasn't able to reproduce it, the default export seems to be deprecated, so I think we should avoid using it anyways.
https://sass-lang.com/documentation/breaking-changes/default-export/

@cod1r
Copy link
Author

cod1r commented Dec 24, 2024

You most likely need to have sass-embedded installed first; but yay...

@hi-ogawa
Copy link
Contributor

hi-ogawa commented Dec 24, 2024

There was a similar issue in #17880. As I commented in the previous PR, sassPath here is the result of require.resolve, so that should be always cjs entry of sass or sass-embedded. We might need to keep .default as a fallback for sass since it doesn't look like it's compatible with cjs named export lexer.

references

I still have no idea why .default fails for some users.

@pkg-pr-new
Copy link

pkg-pr-new bot commented Dec 24, 2024

Open in Stackblitz

npm i https://pkg.pr.new/vite@19053

commit: 3745309

@sapphi-red sapphi-red self-requested a review December 24, 2024 03:03
@sapphi-red sapphi-red dismissed their stale review December 24, 2024 03:04

see hi-ogawa's comment

@cod1r
Copy link
Author

cod1r commented Dec 24, 2024

I might be wrong but the code I changed is in a function that is only called if the sassPackage is sass-embedded. It calls different functions based on config api options in css.ts

https://github.com/vitejs/vite/blob/main/packages/vite/src/node/plugins/css.ts#L2575

@cod1r
Copy link
Author

cod1r commented Dec 24, 2024

the code that handles the specific sass package is different

@hi-ogawa
Copy link
Contributor

hi-ogawa commented Dec 24, 2024

I might be wrong but the code I changed is in a function that is only called if the sassPackage is sass-embedded. It calls different functions based on config api options in css.ts

sass users can also specify api: "modern-compiler" explicitly, so the code path you mentioned is relevant for both sass and sass-embedded. Here is the repro:

@cod1r
Copy link
Author

cod1r commented Dec 24, 2024

I added a conditional check. Maybe that fixes this situation a little better?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

feat: css p2-edge-case Bug, but has workaround or limited in scope (priority) trigger: preview

Projects

None yet

Development

Successfully merging this pull request may close these issues.

sass dynamic import is undefined when building with "modern-compiler" and sass-embedded

3 participants