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

Merged
merged 4 commits into from
Jul 29, 2022

Conversation

nvh95
Copy link
Contributor

@nvh95 nvh95 commented Jul 20, 2022

Description

  • In Vite 3, in index.html, if we refer to a file in publicDir inside url() (e.g: style="background: url(/icon.png)"), it becomes __VITE_PUBLIC_ASSET__hash__ after building (e.g: style="background: url(__VITE_PUBLIC_ASSET__018698dc__)").
  • For more details, see [v3.x] - public/fonts/font.woff in index.html contains __VITE_PUBLIC_ASSET__ #9174
  • This PR updates the playground/assets to reproduce this bug
  • This PR is trying to replace __VITE_PUBLIC_ASSET__hash__ with the actual file in publicDir. (packages/vite/src/node/plugins/html.ts)
  • This PR add tests in playground/assets/__tests__/assets.spec.ts to cover changes. (not passed all yet)

Related PR

Additional context

You can see the preview at https://bestofjs-68pbasy3a-bestofjs.vercel.app/
There is no file such as https://bestofjs-68pbasy3a-bestofjs.vercel.app/__VITE_PUBLIC_ASSET__03d6a1cf__

image

Questions

  • There is one test not passed yet. However, I tried to build it then it work fine. Not sure what makes this inconsistent yet.

https://github.com/vitejs/vite/pull/9247/files#diff-be0c593011deabf61343720611c7586df51ddc269f187678cdbf85ff72a7973bR370-R373


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

@nvh95 nvh95 changed the title Url assets from public fix: Fix __VITE_PUBLIC_ASSET__hash__ in build when refer to file in publicDir Jul 20, 2022
@nvh95
Copy link
Contributor Author

nvh95 commented Jul 20, 2022

I add a new test to assert that if my source code looks like this

<p class="inline-style-public" style="background: url(/icon.png)">
  inline style
</p>

It should become as bellow after building (since the public dir is /static)

<p class="inline-style-public" style="background: url(/foo/icon.png)">
  inline style
</p>

But it hasn't passed yet and I'm not sure what is the problem. I guess it's from how we setup Vitest.

Build
As expected
build

Unit test
URL in unit test doesn't have /foo. It supposes to be http://localhost:5173/foo/icon.png
unit test

@sapphi-red sapphi-red linked an issue Jul 20, 2022 that may be closed by this pull request
7 tasks
@sapphi-red sapphi-red added the p3-minor-bug An edge case that only affects very specific usage (priority) label Jul 20, 2022
playground/assets/index.html Show resolved Hide resolved
Comment on lines +370 to +373
// 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)
Copy link
Member

@sapphi-red sapphi-red Jul 20, 2022

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.

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

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.

@nvh95 nvh95 marked this pull request as ready for review July 20, 2022 15:13
@patak-dev patak-dev changed the title fix: Fix __VITE_PUBLIC_ASSET__hash__ in build when refer to file in publicDir fix: __VITE_PUBLIC_ASSET__hash__ in HTML Jul 29, 2022
@patak-dev patak-dev merged commit a2b24ee into vitejs:main Jul 29, 2022
@nvh95 nvh95 deleted the url-assets-from-public branch July 29, 2022 12:24
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
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.

[v3.x] - public/fonts/font.woff in index.html contains __VITE_PUBLIC_ASSET__
3 participants