Skip to content

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

Open
sun0day wants to merge 4 commits into
vitejs:mainfrom
sun0day:fix/cssRequestFallback
Open

fix(indexHtml): support transform assets url to safe path #12334
sun0day wants to merge 4 commits into
vitejs:mainfrom
sun0day:fix/cssRequestFallback

Conversation

@sun0day

@sun0day sun0day commented Mar 8, 2023

Copy link
Copy Markdown
Member

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.

@bolt-new-by-stackblitz

Copy link
Copy Markdown

Review PR in StackBlitz Codeflow Run & review this pull request in StackBlitz Codeflow.

@patak-cat

Copy link
Copy Markdown
Member

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

sun0day commented Mar 9, 2023

Copy link
Copy Markdown
Member Author

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

bluwy commented Mar 9, 2023

Copy link
Copy Markdown
Member

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

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

sun0day commented Mar 9, 2023

Copy link
Copy Markdown
Member Author

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

bluwy commented Mar 9, 2023

Copy link
Copy Markdown
Member

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

sun0day commented Mar 9, 2023

Copy link
Copy Markdown
Member Author

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-cat

Copy link
Copy Markdown
Member

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

sun0day commented Mar 9, 2023

Copy link
Copy Markdown
Member Author

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
sun0day force-pushed the fix/cssRequestFallback branch from 92e7256 to b7d0ec4 Compare March 10, 2023 16:48
@sun0day

sun0day commented Mar 11, 2023

Copy link
Copy Markdown
Member Author

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

@sun0day sun0day changed the title fix(resolve): css request fallback to node_modules fix(indexHtml): support transform assets url to safe path Mar 11, 2023
@bluwy

bluwy commented Mar 12, 2023

Copy link
Copy Markdown
Member

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.

@patak-cat patak-cat added on hold p3-minor-bug An edge case that only affects very specific usage (priority) labels Mar 15, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

on hold 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.

Assets referencing libraries with bare specifiers in HTML do not work in dev

3 participants