Skip to content

Conversation

@hi-ogawa
Copy link
Contributor

@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)

@bolt-new-by-stackblitz
Copy link

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
Contributor 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
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
Contributor 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
Contributor 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.

moonlitusun pushed a commit to moonlitusun/vite that referenced this pull request May 25, 2025
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