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): handle environment with browser globals #11079

Merged
merged 4 commits into from Nov 29, 2022

Conversation

bluwy
Copy link
Member

@bluwy bluwy commented Nov 25, 2022

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#710

As location.href is shimmed as http://localhost/, the file path sass 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?

  • Bug fix
  • New Feature
  • Documentation update
  • Other

Before submitting the PR, please make sure you do the following

  • Read the Contributing Guidelines.
  • Read the Pull Request Guidelines and follow the Commit Convention.
  • Check that there isn't already a PR that solves the problem the same way to avoid creating a duplicate.
  • Provide a description in this PR that addresses what the PR is solving, or reference the issue that it solves (e.g. fixes #123).
  • Ideally, include relevant tests that fail without this PR but pass with it.

@bluwy bluwy added feat: css p3-minor-bug An edge case that only affects very specific usage (priority) labels Nov 25, 2022
patak-dev
patak-dev previously approved these changes Nov 25, 2022
@nrayburn-tech
Copy link

Does this work when using a location that isn’t just “http://localhost”?
Like a reconfigured jsdom instance, https://stackoverflow.com/a/65741127/13168665.

// trigger scss bug: https://github.com/sass/dart-sass/issues/710
// make sure Vite handles safely
globalThis.window = {}
globalThis.location = new URL('http://localhost/')
Copy link
Member

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.

Comment on lines +1531 to +1532
// NOTE: `sass` always runs it's own importer first, and only falls back to
// the `importer` option when it can't resolve a path
Copy link
Member

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.

Copy link
Member Author

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 👍

@bluwy
Copy link
Member Author

bluwy commented Nov 29, 2022

Does this work when using a location that isn’t just “http://localhost”?
Like a reconfigured jsdom instance, https://stackoverflow.com/a/65741127/13168665.

I didn't know it can be reconfigured that way. In that case it makes sense to support that edge case too then.

@patak-dev patak-dev merged commit e92d025 into main Nov 29, 2022
@patak-dev patak-dev deleted the fix-css-browser-like-node branch November 29, 2022 09:03
futurGH pushed a commit to futurGH/vite that referenced this pull request Feb 26, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
feat: css 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.

Astro Lit integration breaks SCSS imports "Can't find stylesheet to import" when using --threads=false
4 participants