Skip to content

fix(html): skip html comments in tag injection (fix #18386)#22368

Open
maruthang wants to merge 1 commit into
vitejs:mainfrom
maruthang:fix/issue-18386-skip-html-comments-on-inject
Open

fix(html): skip html comments in tag injection (fix #18386)#22368
maruthang wants to merge 1 commit into
vitejs:mainfrom
maruthang:fix/issue-18386-skip-html-comments-on-inject

Conversation

@maruthang
Copy link
Copy Markdown

Description

fixes #18386

When an HTML file begins with a commented-out HTML document (e.g. an old template kept as a reference at the top of the file), Vite was injecting <script> / CSS tags into the comment instead of into the real <head>. The injected assets were therefore never loaded and dev/build silently broke.

Root cause: the injection regexes in packages/vite/src/node/plugins/html.ts (headInjectRE, headPrependInjectRE, bodyInjectRE, bodyPrependInjectRE, htmlInjectRE, htmlPrependInjectRE, doctypePrependInjectRE) were applied directly to the raw HTML via .test() / .replace() and matched the first occurrence of markers like <head> or </body> — including markers that lived inside <!-- ... -->.

Fix: introduced a small helper maskHtmlComments(html) that replaces the contents of HTML comments with same-length spaces so byte indices stay aligned, plus testOutsideComments / replaceFirstOutsideComments wrappers. injectToHead, injectToBody, and prependInjectFallback now use these wrappers; the existing regexes themselves are unchanged. injectToHead and injectToBody are now exported so the regression tests can exercise them directly (mirrors getCssFilesForChunk).

What is the purpose of this pull request?

  • Documentation update
  • Bug fix
  • New feature
  • Other

Additional context

Alternatives considered:

  • Strip comments outright before injecting. Rejected — comments are part of the user's source and must round-trip through the pipeline unchanged. Masking only during the regex test/replace and replacing back into the original string preserves the original bytes.
  • Switch to a real HTML parser for injection. Rejected for now as out-of-scope and a much larger change; the masking approach is minimal, surgical, and keeps the existing well-tested regex paths intact.

Regression tests (added in packages/vite/src/node/__tests__/html.spec.ts):

  • leading commented-out HTML document — inject goes to the real <head>
  • </head> inside a comment — inject still targets the real </head>
  • </body> inside a comment — inject still targets the real </body>
  • only-commented <head> plus a real <body> — exercises the prependInjectFallback path

Verified the bug shape against the reporter's exact HTML via a standalone repro before the fix; with the fix, an end-to-end Vite build of that HTML correctly injects into the real <head> and leaves the leading comment intact.

Validation run locally:

  • pnpm run typecheck — pass
  • pnpm run test-unit — 783 passed, 4 skipped (incl. the 4 new tests)
  • pnpm run test-build playground/html — 54 passed, 8 skipped
  • pnpm run test-serve playground/html — 54 passed, 8 skipped
  • pnpm run build — pass
  • pnpm run lint — pass (only a pre-existing parse error in playground/preserve-symlinks/module-a/linked.js, unrelated to this PR)

  • I have read the Contributing Guidelines.
  • I have searched and confirmed there is no other PR solving this the same way.
  • Documentation update is not required for this change.
  • Tests have been added that fail without this PR and pass with it.

The head/body injection regexes were applied to the raw HTML and matched
the first occurrence of markers like `<head>` or `</body>`, even when
those markers lived inside an HTML comment. As a result, a leading
commented-out HTML document caused Vite to inject `<script>`/CSS tags
into the comment instead of the real document.

Mask comment contents (preserving byte offsets) before testing or
replacing, so injection always targets real markup.

fixes vitejs#18386
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant