-
-
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
fix(css): handle environment with browser globals #11079
Conversation
Does this work when using a location that isn’t just “http://localhost”? |
// trigger scss bug: https://github.com/sass/dart-sass/issues/710 | ||
// make sure Vite handles safely | ||
globalThis.window = {} | ||
globalThis.location = new URL('http://localhost/') |
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.
FYI when navigator
is shimmed (globalThis.navigator = {}
), sass
crashes. (#5815 (comment))
This is not directly related to this PR, but just in case you hit this in future.
// NOTE: `sass` always runs it's own importer first, and only falls back to | ||
// the `importer` option when it can't resolve a path |
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.
Let me confirm that my understanding is correct just to be sure. Here's what I understand:
- sass's own importer only resolves relative path. relative paths are correctly resolved by sass's own importer even if
window
/location
exists. - here Vite's custom importer handles non-relative path and resolves to an absolute path. absolute paths will be broken by sass so we need to load the content on our side.
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.
Yup that's exactly what happens right now 👍
I didn't know it can be reconfigured that way. In that case it makes sense to support that edge case too then. |
Description
When running
sass
in an environment that has browser globals, e.g.jsdom
, it trips up when trying to absolute-ify file paths sass/dart-sass#710As
location.href
is shimmed ashttp://localhost/
, the file pathsass
might absolute-ify to would look something like:http://localhost/Users/bjorn/project/foo/bar.scss
.This PR handles this edge case by stripping it, and also tries to avoid internal sass errors when reading file from the fs by reading it ourselves.
Fixes withastro/astro#5456
Fixes vitest-dev/vitest#1230
Additional context
Astro shims globals by default. I'm probably on the side that we shouldn't do it, but since this helps Vitest too it might be worth fixing here first.
What is the purpose of this pull request?
Before submitting the PR, please make sure you do the following
fixes #123
).