-
-
Notifications
You must be signed in to change notification settings - Fork 8.7k
fix(dev): fix html-proxy cache key mismatch for /@fs/ HTML paths #21762
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
Changes from 2 commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,13 @@ | ||
| <!doctype html> | ||
| <html> | ||
| <head> | ||
| <meta charset="UTF-8" /> | ||
| <title>Test Index HTML</title> | ||
| </head> | ||
| <body> | ||
| <div id="app"></div> | ||
| <script type="module"> | ||
| console.log('test module loaded') | ||
| </script> | ||
| </body> | ||
| </html> |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,88 @@ | ||
| import fs from 'node:fs' | ||
| import path from 'node:path' | ||
| import { describe, expect, onTestFinished, test } from 'vitest' | ||
| import { createServer } from '../../../server' | ||
| import { FS_PREFIX } from '../../../constants' | ||
|
|
||
| const FIXTURE_DIR = path.resolve(import.meta.dirname, 'fixtures') | ||
| const HTML_PATH = path.resolve(FIXTURE_DIR, 'root/index.html') | ||
| const HTML_CONTENT = fs.readFileSync(HTML_PATH, 'utf-8') | ||
|
|
||
| async function createTestServer(rootDir?: string) { | ||
| const root = path.resolve(import.meta.dirname, rootDir ?? 'fixtures/root') | ||
|
|
||
| const server = await createServer({ | ||
| configFile: false, | ||
| root, | ||
| logLevel: 'error', | ||
| server: { | ||
| middlewareMode: true, | ||
| ws: false, | ||
| }, | ||
| optimizeDeps: { | ||
| noDiscovery: true, | ||
| include: [], | ||
| }, | ||
| }) | ||
|
|
||
| onTestFinished(() => server.close()) | ||
| return server | ||
| } | ||
|
|
||
| describe('indexHtml middleware — /@fs/ inline script proxy cache', () => { | ||
| test('inline <script type="module"> in an /@fs/ HTML file is loadable via the html-proxy module', async () => { | ||
| const server = await createTestServer() | ||
| const fsUrl = path.posix.join(FS_PREFIX, HTML_PATH) | ||
|
|
||
| const transformed = await server.transformIndexHtml(fsUrl, HTML_CONTENT) | ||
|
|
||
| expect(transformed).toContain('html-proxy') | ||
| expect(transformed).toContain('index=0') | ||
|
|
||
| const proxyUrlMatch = transformed.match(/src="([^"]*html-proxy[^"]*)"/) | ||
| expect( | ||
| proxyUrlMatch, | ||
| 'devHtmlHook should have rewritten the inline <script> to a ?html-proxy src', | ||
| ).toBeTruthy() | ||
|
|
||
| const proxyModuleUrl = proxyUrlMatch![1] | ||
|
|
||
| const result = | ||
| await server.environments.client.transformRequest(proxyModuleUrl) | ||
|
|
||
| expect( | ||
| result, | ||
| 'proxy module should resolve without a cache-miss error', | ||
| ).not.toBeNull() | ||
| expect(result!.code).toContain('module loaded') | ||
| }) | ||
|
|
||
| test('inline <script type="module"> in an HTML file served from root is loadable via the html-proxy module', async () => { | ||
| const server = await createTestServer('fixtures/root') | ||
| const url = '/index.html' | ||
|
|
||
| const htmlPath = path.resolve( | ||
| import.meta.dirname, | ||
| 'fixtures/root/index.html', | ||
| ) | ||
| const htmlContent = fs.readFileSync(htmlPath, 'utf-8') | ||
|
|
||
| const transformed = await server.transformIndexHtml(url, htmlContent) | ||
|
|
||
| expect(transformed).toContain('html-proxy') | ||
| expect(transformed).toContain('index=0') | ||
|
|
||
| const proxyUrlMatch = transformed.match(/src="([^"]*html-proxy[^"]*)"/) | ||
| expect( | ||
| proxyUrlMatch, | ||
| 'devHtmlHook should rewrite the inline <script> to a ?html-proxy src', | ||
| ).toBeTruthy() | ||
|
|
||
| const proxyModuleUrl = proxyUrlMatch![1] | ||
| const result = | ||
| await server.environments.client.transformRequest(proxyModuleUrl) | ||
|
|
||
| expect(result, 'proxy module should resolve without error').not.toBeNull() | ||
| expect(result!.code).toContain('module loaded') | ||
| }) | ||
| }) | ||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -211,8 +211,11 @@ const devHtmlHook: IndexHtmlTransformHook = async ( | |
|
|
||
| const trailingSlash = htmlPath.endsWith('/') | ||
| if (!trailingSlash && fs.existsSync(filename)) { | ||
| proxyModulePath = htmlPath | ||
| proxyModuleUrl = proxyModulePath | ||
| // If htmlPath is a /@fs/ URL (e.g. vitest-browser always uses this form | ||
| // for testerHtmlPath), normalise to an absolute FS path so proxyCacheUrl | ||
| // is always root-relative. | ||
| proxyModulePath = htmlPath.startsWith(FS_PREFIX) ? filename : htmlPath | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Is there any reason it cannot be
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Well I know the issue is specific to htmlpaths that prefix with /@fs/ and not all paths. So to avoid changing to logic for the parts that work the fix is specific to the paths that fail. The htmlpath can differ from the file name and the proxy code relies on ensuring the key is correct, which is what the problem specific to /@fs/ was causing so it could inadvertently break the key downstream if we just change them all.
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Right, I agree with the choice, but if changing
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Not necessary getting ahead with the fix, but it's good to acknowledge the limitation of |
||
| proxyModuleUrl = htmlPath | ||
|
sapphi-red marked this conversation as resolved.
|
||
| } else { | ||
| // There are users of vite.transformIndexHtml calling it with url '/' | ||
| // for SSR integrations #7993, filename is root for this case | ||
|
|
||
Uh oh!
There was an error while loading. Please reload this page.