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.

Sign up for GitHub

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: __VITE_PUBLIC_ASSET__hash__ in HTML #9247

Merged
merged 4 commits into from
Jul 29, 2022
Merged

fix: __VITE_PUBLIC_ASSET__hash__ in HTML #9247

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
4cd9625
chore: Add reproduce for #9174
nvh95 Jul 19, 2022
2c3d85a
fix: Replace __VITE_PUBLIC_ASSET__ by files in public folder
nvh95 Jul 20, 2022
d1bb5e9
chore: Skip a failed test for missing `base` url in `url` as inline s…
nvh95 Jul 20, 2022
edb7ab2
chore: extract `toOutputAssetFilePath` to `toOutputFilePath`
nvh95 Jul 22, 2022
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
7 changes: 7 additions & 0 deletions packages/vite/src/node/plugins/asset.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -239,6 +239,13 @@ export function getAssetFilename(
return assetHashToFilenameMap.get(config)?.get(hash)
}

export function getPublicAssetFilename(
hash: string,
config: ResolvedConfig
): string | undefined {
return publicAssetUrlCache.get(config)?.get(hash)
}

export function resolveAssetFileNames(
config: ResolvedConfig
): string | ((chunkInfo: PreRenderedAsset) => string) {
Expand Down
23 changes: 21 additions & 2 deletions packages/vite/src/node/plugins/html.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -34,6 +34,8 @@ import {
assetUrlRE,
checkPublicFile,
getAssetFilename,
getPublicAssetFilename,
publicAssetUrlRE,
urlToBuiltUrl
} from './asset'
import { isCSSRequest } from './css'
Expand Down Expand Up @@ -606,13 +608,16 @@ export function buildHtmlPlugin(config: ResolvedConfig): Plugin {
for (const [id, html] of processedHtml) {
const relativeUrlPath = path.posix.relative(config.root, id)
const assetsBase = getBaseInHTML(relativeUrlPath, config)
const toOutputAssetFilePath = (filename: string) => {
const toOutputFilePath = (
filename: string,
type: 'asset' | 'public'
) => {
if (isExternalUrl(filename)) {
return filename
} else {
return toOutputFilePathInHtml(
filename,
'asset',
type,
relativeUrlPath,
'html',
config,
Expand All @@ -621,6 +626,12 @@ export function buildHtmlPlugin(config: ResolvedConfig): Plugin {
}
}

const toOutputAssetFilePath = (filename: string) =>
toOutputFilePath(filename, 'asset')

const toOutputPublicAssetFilePath = (filename: string) =>
toOutputFilePath(filename, 'public')

const isAsync = isAsyncScriptMap.get(config)!.get(id)!

let result = html
Expand Down Expand Up @@ -709,6 +720,14 @@ export function buildHtmlPlugin(config: ResolvedConfig): Plugin {
)
})

result = result.replace(publicAssetUrlRE, (_, fileHash) => {
return normalizePath(
toOutputPublicAssetFilePath(
getPublicAssetFilename(fileHash, config)!
)
)
})

if (chunk && canInlineEntry) {
// all imports from entry have been inlined to html, prevent rollup from outputting it
delete bundle[chunk.fileName]
Expand Down
11 changes: 11 additions & 0 deletions playground/assets/__tests__/assets.spec.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -364,3 +364,14 @@ test('relative path in html asset', async () => {
expect(await page.textContent('.relative-js')).toMatch('hello')
expect(await getColor('.relative-css')).toMatch('red')
})

test('url() contains file in publicDir, in <style> tag', async () => {
expect(await getBg('.style-public-assets')).toContain(iconMatch)
})

test.skip('url() contains file in publicDir, as inline style', async () => {
// TODO: To investigate why `await getBg('.inline-style-public') === "url("http://localhost:5173/icon.png")"`
// It supposes to be `url("http://localhost:5173/foo/icon.png")`
// (I built the playground to verify)
expect(await getBg('.inline-style-public')).toContain(iconMatch)
Comment on lines +373 to +376
Copy link
Member

@sapphi-red sapphi-red Jul 20, 2022 •
edited
Loading

Choose a reason for hiding this comment

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

This test is failing because during dev await getBg('.inline-style-public') is "url("http://localhost:5173/icon.png")".
Vite's test runs with dev and then runs with build+preview.

I think there is a different bug here. await getBg('.inline-style-public') should return url("http://localhost:5173/foo/icon.png") and not url("http://localhost:5173/icon.png")" during dev too. (because src="/raw.js"(at assets/index.html line 18) is rewrote to src="/foo/raw.js" during dev.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yeah. I guess there is a bug in Vitest configuration here but I'm not familiar so I haven't catched it yet. I will investigate more. But if you spot the bug, please let me know, or just push the change directly to my branch. Much appreciated.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The weird thing is that if it's class style, it works fine. The test fail with inline style only. So I am nor sure it's a bug in Vitest config or the code implementation 😂

Copy link
Member

Choose a reason for hiding this comment

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

I guess there is a bug in Vitest configuration here but I'm not familiar so I haven't catched it yet.

No, I meant there is a bug in Vite.

The weird thing is that if it's class style, it works fine.

Good finding 👍 I think this is a different bug.
I haven't checked closely yet but I suppose style attributes needs to be rewrote here during dev.

vite/packages/vite/src/node/server/middlewares/indexHtml.ts

Lines 185 to 253 in 2c3d85a

await traverseHtml(html, htmlPath, (node) => {
if (node.type !== NodeTypes.ELEMENT) {
return
}
// script tags
if (node.tag === 'script') {
const { src, isModule } = getScriptInfo(node)
if (src) {
processNodeUrl(src, s, config, htmlPath, originalUrl, moduleGraph)
} else if (isModule && node.children.length) {
addInlineModule(node, 'js')
}
}
if (node.tag === 'style' && node.children.length) {
const children = node.children[0] as TextNode
styleUrl.push({
start: children.loc.start.offset,
end: children.loc.end.offset,
code: children.content
})
}
// elements with [href/src] attrs
const assetAttrs = assetAttrsConfig[node.tag]
if (assetAttrs) {
for (const p of node.props) {
if (
p.type === NodeTypes.ATTRIBUTE &&
p.value &&
assetAttrs.includes(p.name)
) {
processNodeUrl(p, s, config, htmlPath, originalUrl)
}
}
}
})
await Promise.all(
styleUrl.map(async ({ start, end, code }, index) => {
const url = `${proxyModulePath}?html-proxy&direct&index=${index}.css`
// ensure module in graph after successful load
const mod = await moduleGraph.ensureEntryFromUrl(url, false)
ensureWatchedFile(watcher, mod.file, config.root)
const result = await server!.pluginContainer.transform(code, mod.id!)
s.overwrite(start, end, result?.code || '')
})
)
html = s.toString()
return {
html,
tags: [
{
tag: 'script',
attrs: {
type: 'module',
src: path.posix.join(base, CLIENT_PUBLIC_PATH)
},
injectTo: 'head-prepend'
}
]
}
}

If you prefer, I think we could go with commenting out this line for now since it's a different bug. (I'll create a issue or PR later)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thank for your help. I will separate it to a different test case then skip it for now. I will push it soon.

Copy link
Contributor Author

@nvh95 nvh95 Jul 20, 2022 •
edited
Loading

Choose a reason for hiding this comment

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

I just re-ran the code again in DEV mode and BUILD mode. I confirm that the url() in inline style is missing base url.

Dev mode
Missing /foo
SCR-20220720-ukj

This is what I get when I visit http://127.0.0.1:5173/icon.png directly.
image

But I don't understand why it works fine as a background image in a CSS rule
SCR-20220720-urv

Build mode
Have /foo
SCR-20220720-nbz

So I guess we just discovered another bug.

I also pushed the commit to skip the failed test for DEV mode.

})
10 changes: 10 additions & 0 deletions playground/assets/index.html
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -266,6 +266,16 @@ <h3>base64</h3>
inline style
</p>
<p class="style-base64-assets">use style class</p>
<h3>from publicDir</h3>
<style>
.style-public-assets {
background: url('/icon.png');
}
</style>
<p class="inline-style-public" style="background: url(/icon.png)">
inline style
</p>
<p class="style-public-assets">use style class</p>

<h3 class="import-css">@import</h3>
<style class="style-import">
Expand Down