fix(html): handle trailing slash in transformIndexHtml pre-transform#21872
fix(html): handle trailing slash in transformIndexHtml pre-transform#21872XploitMonk0x01 wants to merge 1 commit intovitejs:mainfrom
Conversation
travisbreaks
left a comment
There was a problem hiding this comment.
Review
Verdict: The fix is correct and well-scoped. A few observations and one suggestion.
The bug
path.posix.dirname('/example/dir/') returns /example (not /example/dir/), so when htmlPath ends with / (common in SSR with appType: 'custom' and req.originalUrl), relative script references like ./filename.js resolve to the wrong parent directory. The pre-transform request then 404s.
The fix
The getHtmlDirnameForRelativeUrl function is the right approach: if the path already ends with /, it IS the directory, so return it unchanged. Otherwise, delegate to dirname() as before.
The function is pure, exported, and unit-tested separately from the middleware. Good.
Edge cases
- Root path
/: Both branches produce/(dirname of/is/, and/ends with/). No regression. path.posix.join('/', '/example/dir/', './filename.js')correctly yields/example/dir/filename.js. Verified.../references:path.posix.join('/', '/example/dir/', '../other.js')correctly yields/example/other.js. Works as expected.- Non-trailing-slash paths (e.g.,
/example/dir/index.html): Fall through todirname()as before. No behavior change.
Suggestion for test coverage
The tests are solid for the extracted function. One additional case worth considering:
test('handles parent-relative resolution from trailing slash directory', () => {
const preTransformUrl = path.posix.join(
'/',
getHtmlDirnameForRelativeUrl('/example/dir/'),
'../other.js',
)
expect(preTransformUrl).toBe('/example/other.js')
})This would cover the ../ case, which is common in real HTML and exercises different path.posix.join behavior.
One note on scope
The first replacer block (lines ~142-157, non-pre-transform URL rewriting) does not use dirname(htmlPath) for relative URLs, so it's unaffected by this bug and correctly left untouched.
Minor: query strings in htmlPath
If htmlPath arrives with a query string (e.g., /example/dir/?t=123), endsWith('/') would be false and the bug would still trigger. In the current Vite codebase, htmlPath appears to come in clean (no query/hash), so this is not a practical concern today, but worth noting for robustness.
Overall: clean, minimal, correct. Nice contribution.
Fixes #18964
Normalizes relative URL pre-transform path resolution when htmlPath ends with /
Adds regression tests in packages/vite/src/node/server/middlewares/tests/indexHtml.spec.ts