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

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

Open
wants to merge 3 commits into
base: main
Choose a base branch
from

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
Collaborator

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.

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
Collaborator

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?

@sapphi-red sapphi-red removed their request for review December 26, 2024 05:40
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