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
Merged
Show file tree
Hide file tree
Changes from 2 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
32 changes: 30 additions & 2 deletions packages/vite/src/node/plugins/css.ts
Expand Up @@ -1495,6 +1495,31 @@ function loadPreprocessor(
}
}

// in unix, scss might append http://localhost in environments that shim `location`
// see https://github.com/sass/dart-sass/issues/710
const cleanScssBugUrl = (url: string) => url.replace(/^http:\/\/localhost/, '')

function fixScssBugImportValue(
data: Sass.ImporterReturnType
): Sass.ImporterReturnType {
// the scss bug doesn't load files properly so we have to load it ourselves
// to prevent internal error when it loads itself
if (
// check bug via `window` and `location` global
typeof window !== 'undefined' &&
typeof location !== 'undefined' &&
data &&
// @ts-expect-error
data.file &&
// @ts-expect-error
data.contents == null
) {
// @ts-expect-error
data.contents = fs.readFileSync(data.file, 'utf-8')
}
return data
}

// .scss/.sass processor
const scss: SassStylePreprocessor = async (
source,
Expand All @@ -1503,11 +1528,14 @@ const scss: SassStylePreprocessor = async (
resolvers
) => {
const render = loadPreprocessor(PreprocessLang.sass, root).render
// NOTE: `sass` always runs it's own importer first, and only falls back to
// the `importer` option when it can't resolve a path
Comment on lines +1546 to +1547
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 👍

const internalImporter: Sass.Importer = (url, importer, done) => {
importer = cleanScssBugUrl(importer)
resolvers.sass(url, importer).then((resolved) => {
if (resolved) {
rebaseUrls(resolved, options.filename, options.alias, '$')
.then((data) => done?.(data))
.then((data) => done?.(fixScssBugImportValue(data)))
.catch((data) => done?.(data))
} else {
done?.(null)
Expand Down Expand Up @@ -1552,7 +1580,7 @@ const scss: SassStylePreprocessor = async (
}
})
})
const deps = result.stats.includedFiles
const deps = result.stats.includedFiles.map((f) => cleanScssBugUrl(f))
const map: ExistingRawSourceMap | undefined = result.map
? JSON.parse(result.map.toString())
: undefined
Expand Down
5 changes: 5 additions & 0 deletions playground/css/vite.config.js
@@ -1,5 +1,10 @@
const path = require('node:path')

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


/**
* @type {import('vite').UserConfig}
*/
Expand Down