fix(indexHtml): support transform assets url to safe path

[sun0day]

Description

fix #12260

vite server will try to resolve files from the root directory while receiving assets requests which start with '/', but some requests maybe expect to visit the assets from the node_modules, this PR is trying to add a fallback to resolve assets from the node_modules while requested files don't exist in the root directory.

We could mock a fake entry file _vite_entry_.js to import assets URLs, then covert asset URLs to import statement and transform them to the safe URLs via import-analysis plugin.

Additional context

Also, I think we could enhance to resolve asset requests which match the alias pattern for better DX.

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 PR Title 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.

[stackblitz]

Run & review this pull request in StackBlitz Codeflow.

[patak-dev]

You're fixing a ton of bugs! 👏🏼
Would it be possible to explain a bit more about how this fix works and add the repro as a test case? (in the css or html playground?)

[sun0day]

You're fixing a ton of bugs! 👏🏼 Would it be possible to explain a bit more about how this fix works and add the repro as a test case? (in the css or html playground?)

Sorry for missing the PR's description, I've updated it. Not sure whether this PR is a good solution. I will add the test cases later

[bluwy]

I'm not sure if this is the right fix. I think we need to change it here

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

Lines 232 to 247 in dd7a2f1

	const assetAttrs = assetAttrsConfig[node.nodeName]
	if (assetAttrs) {
	for (const p of node.attrs) {
	const attrKey = getAttrKey(p)
	if (p.value && assetAttrs.includes(attrKey)) {
	processNodeUrl(
	p,
	node.sourceCodeLocation!.attrs![attrKey],
	s,
	config,
	htmlPath,
	originalUrl,
	)
	}
	}
	}

Something like server.pluginContainer.resolve to resolve to the right path, but it get's trickier because import-analysis doesn't run here to transform into URL-safe paths. So the entire feature is quite a big one.

I think the initial idea is to have the HTML be as plain as possible (without much processing). The implementation detail when building HTML files enabled #12260, but isn't something we publicly support, so I guess we could say that this is intended behaviour.

[sun0day]

Yeah. I've thought about that approach too, but it's a bit trickier for now. So I was wondering whether we can support resolving URLs during runtime in these special cases.

I don't think converting URLs in html via indexHtml in the first place conflicts with resolving them at runtime, both approaches rely on the capabilities of the resolve and import-analysis plugins.

On the other hand, reserving the html's URLs in dev mode has a better DX.

[bluwy]

The issue I think is that this PR carves out a part of the resolve logic for a case in HTML, so it doesn't cover aliases etc, and causes extra resolving for unrelated parts of the lifecycle. So it's probably better to do it once that supports all correctly.

[sun0day]

I think it's possible to add alias resolving to resolve plugin, even though I haven't added it to this PR.

I agree with you

So it's probably better to do it once that supports all correctly.

I had an idea that we could convert the assets URLs to js code like import 'style.css' first, then call import-analysis.transform API to transform the js code to URL-safe paths and replace the origin assets URLs with the new URL-safe paths at last.

[patak-dev]

I'm also not sure if we should support this. My first reaction is that we should do it because it is valid to do the same with a script tag and js. But I agree with @bluwy that we shouldn't bend the current resolve logic to support this. We discussed today about perf with the team, and we should try to remove and simplify the resolve plugin moving forward instead of adding more checks.

[sun0day]

Yeah. I agree with @bluwy too. As I dig deeper into this, I think it's possible to reuse the capability of import-analysis plugin in indexHtml middleware via an easy way. We could covert the URLs to import statements and mock importing them in a js file so that we can call import-analysis plugin's transform API. Thus we can bypass the resolve plugin and avoid the runtime's resolving perf loss.

[sun0day]

@patak-dev @bluwy sorry to ping you again, I just implemented the idea mentioned before, it works for the regression tests and new cases of alias pattern link, there may be still some edge cases that need to be handled, but I guess the solution will work fine.

If you still have questions about this solution, that's fine to close the pr directly.

[bluwy]

This looks like a neat trick! But I'm not sure if it's a good thing to keep this hack within Vite, and I can't say I've not done this myself before though 😄

I think ideally we should try to extract normalizeUrl from import-analysis, but I've tried that before and it's harder than expected (It relies on this IIRC to emit stuff). Maybe I should re-look into it, I think it'll be better to do this the right way in case it bites us back in the future.

Thanks for testing this out though. We could probably reserve this hack as a last resort if needed.