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): fix missing source file warning with sass modern api custom importer #18113

Merged
merged 4 commits into from
Sep 24, 2024

Conversation

hi-ogawa
Copy link
Collaborator

@hi-ogawa hi-ogawa commented Sep 16, 2024

Description

I added data: to "missing source" false positive list since Sass modern API uses data:... when the code is loaded from custom importer. This would mean css.devSourcemap works only for the initial import of sass files, which is pretty bad, but I couldn't find any solution for this. EDIT: sourceMapUrl works #18113 (comment)

Copy link

stackblitz bot commented Sep 16, 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 September 16, 2024 10:02
@sapphi-red
Copy link
Member

https://sass-lang.com/documentation/js-api/interfaces/importerresult/#sourceMapUrl
Returning sourceMapUrl seems to change the sources value.

return { contents, syntax }

Could you try to see if it works?

@sapphi-red sapphi-red added feat: css p3-minor-bug An edge case that only affects very specific usage (priority) feat: sourcemap Sourcemap support labels Sep 17, 2024
@hi-ogawa
Copy link
Collaborator Author

https://sass-lang.com/documentation/js-api/interfaces/importerresult/#sourceMapUrl
Returning sourceMapUrl seems to change the sources value.

Oops, I totally missed that somehow. This totally works, thanks!

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.

Thank you 👍

@sapphi-red sapphi-red merged commit d7763a5 into vitejs:main Sep 24, 2024
13 checks passed
sapphi-red pushed a commit to sapphi-red/vite that referenced this pull request Sep 24, 2024
@hi-ogawa hi-ogawa deleted the fix-sass-modern-sourcemap branch September 24, 2024 03:58
sapphi-red added a commit that referenced this pull request Sep 24, 2024
…modern api custom importer (#18183)

Co-authored-by: Hiroshi Ogawa <[email protected]>
@Defite
Copy link

Defite commented Sep 24, 2024

Will it be merged into v4?

@sapphi-red
Copy link
Member

@Defite I guess you meant v5. It's been released in 5.4.8.

@Defite
Copy link

Defite commented Sep 25, 2024

@Defite I guess you meant v5. It's been released in 5.4.8.

Nope, I meant v4, because I can't migrate to v5 right now. I've solved my issue by downgrading sass version to lower one.

@sapphi-red
Copy link
Member

We don't have plan to backport this PR to v4 for now.

@Defite
Copy link

Defite commented Sep 25, 2024

We don't have plan to backport this PR to v4 for now.

I got it, thanks for answer!

@hi-ogawa
Copy link
Collaborator Author

hi-ogawa commented Sep 25, 2024

Nope, I meant v4, because I can't migrate to v5 right now. I've solved my issue by downgrading sass version to lower one.

@Defite Is it possible the warning you're seeing is "legacy js API" one like in #18164? I thought downgrading sass version wouldn't matter for sourcemap warning on modern API, which is what this PR is fixing.

@Defite
Copy link

Defite commented Sep 25, 2024

Nope, I meant v4, because I can't migrate to v5 right now. I've solved my issue by downgrading sass version to lower one.

@Defite Is it possible the warning you're seeing is "legacy js API" one like in #18164? I thought downgrading sass version wouldn't matter for sourcemap warning on modern API, which is what this PR is fixing.

Yes, the message is the same as in #18164. But somehow after downgrading sass version I don't see this messages anymore.

@hi-ogawa
Copy link
Collaborator Author

hi-ogawa commented Sep 25, 2024

@Defite Okay, so the warning you see is not technically same as this PR. If you follow the link in the warning https://sass-lang.com/documentation/breaking-changes/legacy-js-api, I think you should be able to still upgrade sass and use modern API (though you cannot use css.devSourcemap: true).

@Defite
Copy link

Defite commented Sep 25, 2024

@hi-ogawa yep, sorry for bothering in wrong PR.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
feat: css feat: sourcemap Sourcemap support p3-minor-bug An edge case that only affects very specific usage (priority)
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Sourcemap for ... points to missing source files for @import with sass modern api
4 participants