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(hmr): reload CSS when inlined SVG is changed #18972

Draft
wants to merge 3 commits into
base: main
Choose a base branch
from
Draft
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
5 changes: 5 additions & 0 deletions packages/vite/src/node/plugins/asset.ts
Original file line number Diff line number Diff line change
Expand Up @@ -184,6 +184,11 @@ export function assetPlugin(config: ResolvedConfig): Plugin {
id = removeUrlQuery(id)
let url = await fileToUrl(this, id)

if (id.endsWith('.css')) {
// .css?url depends on .css?direct
this.addWatchFile(id + '?direct')
}

// Inherit HMR timestamp if this asset was invalidated
if (!url.startsWith('data:') && this.environment.mode === 'dev') {
const mod = this.environment.moduleGraph.getModuleById(id)
Expand Down
72 changes: 45 additions & 27 deletions packages/vite/src/node/plugins/css.ts
Original file line number Diff line number Diff line change
Expand Up @@ -373,20 +373,20 @@ export function cssPlugin(config: ResolvedConfig): Plugin {
const resolveUrl = (url: string, importer?: string) =>
idResolver(environment, url, importer)

const urlReplacer: CssUrlReplacer = async (url, importer) => {
const urlResolver: CssUrlResolver = async (url, importer) => {
const decodedUrl = decodeURI(url)
if (checkPublicFile(decodedUrl, config)) {
if (encodePublicUrlsInCSS(config)) {
return publicFileToBuiltUrl(decodedUrl, config)
return [publicFileToBuiltUrl(decodedUrl, config), undefined]
} else {
return joinUrlSegments(config.base, decodedUrl)
return [joinUrlSegments(config.base, decodedUrl), undefined]
}
}
const [id, fragment] = decodedUrl.split('#')
let resolved = await resolveUrl(id, importer)
if (resolved) {
if (fragment) resolved += '#' + fragment
return fileToUrl(this, resolved)
return [await fileToUrl(this, resolved), resolved]
}
if (config.command === 'build') {
const isExternal = config.build.rollupOptions.external
Expand All @@ -405,7 +405,7 @@ export function cssPlugin(config: ResolvedConfig): Plugin {
)
}
}
return url
return [url, undefined]
}

const {
Expand All @@ -418,7 +418,7 @@ export function cssPlugin(config: ResolvedConfig): Plugin {
id,
raw,
preprocessorWorkerController!,
urlReplacer,
urlResolver,
)
if (modules) {
moduleCache.set(id, modules)
Expand Down Expand Up @@ -1058,17 +1058,20 @@ export function cssAnalysisPlugin(config: ResolvedConfig): Plugin {
// main import to hot update
const depModules = new Set<string | EnvironmentModuleNode>()
for (const file of pluginImports) {
depModules.add(
isCSSRequest(file)
? moduleGraph.createFileOnlyEntry(file)
: await moduleGraph.ensureEntryFromUrl(
await fileToDevUrl(
this.environment,
file,
/* skipBase */ true,
),
),
)
if (isCSSRequest(file)) {
depModules.add(moduleGraph.createFileOnlyEntry(file))
} else {
const url = await fileToDevUrl(
this.environment,
file,
/* skipBase */ true,
)
if (url.startsWith('data:')) {
depModules.add(moduleGraph.createFileOnlyEntry(file))
} else {
depModules.add(await moduleGraph.ensureEntryFromUrl(url))
}
}
}
moduleGraph.updateModuleInfo(
thisModule,
Expand Down Expand Up @@ -1261,7 +1264,7 @@ async function compileCSS(
id: string,
code: string,
workerController: PreprocessorWorkerController,
urlReplacer?: CssUrlReplacer,
urlResolver?: CssUrlResolver,
): Promise<{
code: string
map?: SourceMapInput
Expand All @@ -1271,7 +1274,7 @@ async function compileCSS(
}> {
const { config } = environment
if (config.css.transformer === 'lightningcss') {
return compileLightningCSS(id, code, environment, urlReplacer)
return compileLightningCSS(id, code, environment, urlResolver)
}

const { modules: modulesOptions, devSourcemap } = config.css
Expand Down Expand Up @@ -1380,10 +1383,11 @@ async function compileCSS(
)
}

if (urlReplacer) {
if (urlResolver) {
postcssPlugins.push(
UrlRewritePostcssPlugin({
replacer: urlReplacer,
resolver: urlResolver,
deps,
logger: environment.logger,
}),
)
Expand Down Expand Up @@ -1717,6 +1721,12 @@ async function resolvePostcssConfig(
return result
}

type CssUrlResolver = (
url: string,
importer?: string,
) =>
| [url: string, id: string | undefined]
| Promise<[url: string, id: string | undefined]>
type CssUrlReplacer = (
url: string,
importer?: string,
Expand All @@ -1733,7 +1743,8 @@ export const importCssRE =
const cssImageSetRE = /(?<=image-set\()((?:[\w-]{1,256}\([^)]*\)|[^)])*)(?=\))/

const UrlRewritePostcssPlugin: PostCSS.PluginCreator<{
replacer: CssUrlReplacer
resolver: CssUrlResolver
deps: Set<string>
logger: Logger
}> = (opts) => {
if (!opts) {
Expand All @@ -1757,8 +1768,12 @@ const UrlRewritePostcssPlugin: PostCSS.PluginCreator<{
const isCssUrl = cssUrlRE.test(declaration.value)
const isCssImageSet = cssImageSetRE.test(declaration.value)
if (isCssUrl || isCssImageSet) {
const replacerForDeclaration = (rawUrl: string) => {
return opts.replacer(rawUrl, importer)
const replacerForDeclaration = async (rawUrl: string) => {
const [newUrl, resolvedId] = await opts.resolver(rawUrl, importer)
if (resolvedId) {
opts.deps.add(resolvedId)
}
return newUrl
}
if (isCssUrl && isCssImageSet) {
promises.push(
Expand Down Expand Up @@ -3129,7 +3144,7 @@ async function compileLightningCSS(
id: string,
src: string,
environment: PartialEnvironment,
urlReplacer?: CssUrlReplacer,
urlResolver?: CssUrlResolver,
): ReturnType<typeof compileCSS> {
const { config } = environment
const deps = new Set<string>()
Expand Down Expand Up @@ -3215,11 +3230,14 @@ async function compileLightningCSS(
css = css.replace(dep.placeholder, () => dep.url)
break
}
if (urlReplacer) {
const replaceUrl = await urlReplacer(
if (urlResolver) {
const [replaceUrl, resolvedId] = await urlResolver(
dep.url,
toAbsolute(dep.loc.filePath),
)
if (resolvedId) {
deps.add(resolvedId)
}
css = css.replace(dep.placeholder, () => replaceUrl)
} else {
css = css.replace(dep.placeholder, () => dep.url)
Expand Down
1 change: 1 addition & 0 deletions packages/vite/src/node/server/hmr.ts
Original file line number Diff line number Diff line change
Expand Up @@ -793,6 +793,7 @@ function propagateUpdate(
// #3716, #3913
// For a non-CSS file, if all of its importers are CSS files (registered via
// PostCSS plugins) it should be considered a dead end and force full reload.
// TODO
if (
!isCSSRequest(node.url) &&
[...node.importers].every((i) => isCSSRequest(i.url))
Comment on lines +796 to 799
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

With this PR, changing any file referenced by CSS will cause a full reload because of this code (slightly related: this code is the cause of #9512). That'll probably degrade the UX quite a bit.

For this reason, I'm considering to only handle inlined SVGs for now (in a different PR).

Expand Down
26 changes: 25 additions & 1 deletion playground/assets/__tests__/assets.spec.ts
Original file line number Diff line number Diff line change
Expand Up @@ -287,12 +287,36 @@ describe('css url() references', () => {
})

test('url() with svg', async () => {
expect(await getBg('.css-url-svg')).toMatch(/data:image\/svg\+xml,.+/)
const bg = await getBg('.css-url-svg')
expect(bg).toMatch(/data:image\/svg\+xml,.+/)
expect(bg).toContain('blue')
expect(bg).not.toContain('red')

if (isServe) {
editFile('nested/fragment-bg-hmr.svg', (code) =>
code.replace('fill="blue"', 'fill="red"'),
)
await untilUpdated(() => getBg('.css-url-svg'), 'red')
}
})

test('image-set() with svg', async () => {
expect(await getBg('.css-image-set-svg')).toMatch(/data:image\/svg\+xml,.+/)
})

test('url() with svg in .css?url', async () => {
const bg = await getBg('.css-url-svg-in-url')
expect(bg).toMatch(/data:image\/svg\+xml,.+/)
expect(bg).toContain('blue')
expect(bg).not.toContain('red')

if (isServe) {
editFile('nested/fragment-bg-hmr2.svg', (code) =>
code.replace('fill="blue"', 'fill="red"'),
)
await untilUpdated(() => getBg('.css-url-svg'), 'red')
}
})
})

describe('image', () => {
Expand Down
4 changes: 4 additions & 0 deletions playground/assets/css/css-url-url.css
Original file line number Diff line number Diff line change
@@ -0,0 +1,4 @@
.css-url-svg-in-url {
background: url(../nested/fragment-bg-hmr2.svg);
background-size: 10px;
}
2 changes: 1 addition & 1 deletion playground/assets/css/css-url.css

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

9 changes: 9 additions & 0 deletions playground/assets/index.html
Original file line number Diff line number Diff line change
Expand Up @@ -148,6 +148,9 @@ <h2>CSS url references</h2>
<div class="css-url-svg">
<span style="background: #fff">CSS SVG background</span>
</div>
<div class="css-url-svg-in-url">
<span style="background: #fff">CSS (?url) SVG background</span>
</div>

<div class="css-image-set-svg">
<span style="background: #fff">CSS SVG background with image-set</span>
Expand Down Expand Up @@ -528,6 +531,12 @@ <h3>assets in template</h3>
import cssUrl from './css/icons.css?url'
text('.url-css', cssUrl)

import cssUrlUrl from './css/css-url-url.css?url'
const linkTag = document.createElement('link')
linkTag.href = cssUrlUrl
linkTag.rel = 'stylesheet'
document.body.appendChild(linkTag)

// const url = new URL('non_existent_file.png', import.meta.url)
const metaUrl = new URL('./nested/asset.png', import.meta.url)
text('.import-meta-url', metaUrl)
Expand Down
21 changes: 21 additions & 0 deletions playground/assets/nested/fragment-bg-hmr.svg
Loading
Sorry, something went wrong. Reload?
Sorry, we cannot display this file.
Sorry, this file is invalid so it cannot be displayed.
21 changes: 21 additions & 0 deletions playground/assets/nested/fragment-bg-hmr2.svg
Loading
Sorry, something went wrong. Reload?
Sorry, we cannot display this file.
Sorry, this file is invalid so it cannot be displayed.
Loading