Skip to content

dogfood: SSR instantiates a custom element inside an HTML comment #1128

Description

@vivek7405

Problem

A registered custom-element tag name written inside an HTML comment in an html template
is instantiated and rendered at SSR, and the surrounding markup is corrupted in the process.
Writing a component's tag in a comment is the natural way to document a template, so this
is easy to hit and hard to diagnose: the page renders, no error is thrown, and the damage
shows up as mangled HTML somewhere after the comment.

Found while documenting a shared element in website/app/layout.ts for #1127. The comment
read <!-- Shared aria-describedby target for every <copy-cmd> on the page. ... --> and the
layout silently grew a fully rendered copy button. The workaround was to drop the angle
brackets from the comment.

Minimal repro, run against the current tree:

import { html, WebComponent } from '@webjsdev/core';
import { renderToString } from '@webjsdev/core/server';

class Marker extends WebComponent {
  render() { return html`<span class="INSTANTIATED">rendered</span>`; }
}
Marker.register('x-marker');

await renderToString(html`<div><!-- see <x-marker> for details --></div>`);
await renderToString(html`<div><!-- <x-marker></x-marker> --></div>`);
await renderToString(html`<div><!-- <x-unknown> --></div>`);

Actual output:

INSTANTIATED | comment containing the tag
  "<div><!-- see <x-marker data-wj-host><!--webjs-hydrate--><span class=\"INSTANTIATED\">rendered</span></x-marker>"
INSTANTIATED | comment wrapping a full tag
  "<div><!-- <x-marker data-wj-host><!--webjs-hydrate--><span class=\"INSTANTIATED\">rendered</span></x-marker> --></div>"
inert        | comment with unregistered tag
  "<div><!-- <x-unknown> --></div>"

Two distinct damage modes, both worse than a wasted render:

  1. Tag mentioned in prose inside a comment. Everything after the tag name, INCLUDING the
    closing --> and the </div>, is dropped. The output ends with an unterminated <!--,
    so a browser parses the entire rest of the document as comment text. In the repro the
    trailing for details --></div> is simply gone.
  2. A full element inside a comment. The component renders inside the comment, and its
    own <!--webjs-hydrate--> marker terminates the OUTER comment early (HTML comments do not
    nest, the first --> closes). The component's markup then leaks out as visible content
    followed by a stray --> in the text.

Note the third case: an UNREGISTERED tag inside a comment is inert. So whether a comment is
safe depends on whether its tag name happens to be a registered component, which is why this
reads as random rather than as a rule.

Design / approach

The custom-element walk needs to skip comment regions, the way the template scanner already
does for holes.

injectDSD (packages/core/src/render-server.js L426) builds one flat regex over the
assembled HTML (L444) and instantiates every match:

const pattern = new RegExp(
  `<(${sortedTags.map(escapeRegex).join('|')})(?=[\\s>/])((?:"[^"]*"|'[^']*'|[^>])*?)(/?)>`,
  'g'
);
for (const m of html.matchAll(pattern)) { ... }

Nothing in that loop knows about <!-- / -->. The template scanner DOES track comment
state (L151 to L184, with a duplicate for the streaming path at L1375 to L1410), but it uses
it only to decide how to escape interpolated holes, and injectDSD runs afterward over the
already-assembled string.

So the fix is to make the walk comment-aware: compute the comment ranges once and skip any
match whose index falls inside one, or tokenize instead of regex-matching. Whichever way, it
must hold for the streaming scanner too, not just the buffered one.

The browser side is expected to differ, since the client parses templates with the real HTML
parser (via <template>), which never instantiates an element inside a comment. That makes
this an SSR-versus-client divergence and therefore a hydration mismatch, not only a rendering
bug. Worth confirming as part of the fix rather than assuming.

Implementation notes (for the implementing agent)

Where to edit

  • packages/core/src/render-server.js L426 injectDSD(), specifically the pattern regex
    at L444 and the for (const m of html.matchAll(pattern)) loop at L450. This is the
    instantiation site.
  • The same file's streaming scanner around L1375 to L1410 mirrors the buffered comment-state
    machine at L151 to L184. Check whether the streaming render path reaches the same walk and
    needs the same guard; a fix in one that skips the other leaves the bug live under
    <webjs-suspense> / streamed SSR.
  • processSuspenseElements() and substituteSlotsInRender() run over the same assembled
    markup and are worth auditing for the identical blind spot.
  • Contrast with L877, where the slot-partition code already special-cases rest.startsWith('<!--').
    That is the precedent for handling comments correctly in this file.

Landmines

  • The framework emits its OWN comments and depends on them: <!--webjs-hydrate--> (L585,
    L604, L676) and the keyed client-router boundary pairs <!--wj:children:<segment>:<key>-->
    from Rebuild the client-router swap on route-keyed comment boundaries #1015. injectDSD RECURSES on rendered output that already contains these, so a
    comment-skipping pass must treat them as self-contained comments and must not start
    skipping at a marker and swallow real elements after it. Getting this wrong breaks the
    client router's boundary scan, which is strict: a mispaired or duplicated boundary poisons
    the scan and degrades navigation to a full page load.
  • The boundary comments are load-bearing for Rebuild the client-router swap on route-keyed comment boundaries #1015 and dogfood: client router intermittently falls back to a full page load (tab spinner + whole-document flash) #1114. Any change to how comments are
    traversed needs the nested-layout partial-swap tests run, not just the SSR unit tests.
  • Do NOT fix this by stripping comments from SSR output. Author comments are legitimate
    output, and the framework's own markers must survive.
  • The (?=[\s>/]) lookahead and longest-tag-first sort at L438 to L443 exist to stop
    my-card matching <my-card-2>. Preserve that behaviour when restructuring the match.

Invariants to respect

  • Invariant 9 (no backticks inside html template bodies) already constrains what can go in
    a comment; this issue is about angle brackets, which are NOT currently constrained.
  • SSR output must stay byte-identical to what it produces today for every template that does
    not contain a comment, so the elision differential and the ETag stability work in dogfood: webjs.dev pages are never cached at any layer #1127 are
    not disturbed.

Tests + docs surfaces

  • Unit: packages/core/test/** alongside the existing SSR / DSD tests. Cover all three repro
    cases (tag in prose, full element, unregistered tag) plus a comment that legitimately
    contains a framework marker.
  • Counterfactual: the test must fail when the comment guard is reverted.
  • Browser: assert the client render of the same template leaves the comment inert, which is
    what pins the SSR-versus-client parity claim.
  • Bun parity: the TS stripper and SSR dispatch are runtime-sensitive surfaces, so add or
    update a test/bun/*.mjs assertion if the change touches the shared render path.
  • Docs: references/components.md and references/muscle-memory-gotchas.md are the natural
    homes for "a component tag inside a template comment is still instantiated", if the
    decision is to document any residual constraint rather than fully fix it.

Acceptance criteria

  • A registered tag inside an HTML comment is NOT instantiated at SSR
  • The comment and all markup after it survive unchanged (no dropped -->, no dropped
    trailing siblings)
  • A component rendered adjacent to a comment still gets its normal DSD / hydration marker
  • The framework's own <!--webjs-hydrate--> and <!--wj:children:...--> markers still
    round-trip, with the nested-layout partial-swap tests green
  • The streaming SSR path behaves identically to the buffered one
  • Client-side render of the same template is confirmed to match the SSR result
  • A counterfactual proves the new test fails when the guard is removed

Metadata

Metadata

Assignees

Labels

bugSomething isn't working

Type

No type

Projects

Status
Done

Milestone

No milestone

Relationships

None yet

Development

No branches or pull requests

Issue actions