ReviewOS

stacks/stx

<component :is="{⁠{ as }}"> still unresolved after #1817: interpolated :is emits a comment and drops the whole subtree — all 26 library primitives now fail silently

#1826
Closed glennmichael123 opened this 14 days ago · 3 comments
14 days ago

Follow-up to #1817. That fix addressed the ordering half — <component :is> now reaches its handler and no longer emits [Error loading component: … open 'component'] with absolute paths. Confirmed fixed.

The other half reported in the same issue, under "A pure reorder is not sufficient", was not addressed, and it is the half that the component library actually depends on: an interpolated :is is not resolved. The component silently emits a comment and its entire subtree — including <slot /> — is discarded.

Net effect: the same 26 primitives are still unusable. They now fail silently instead of loudly, which is harder to diagnose than before.

Measurement

Rendering <DialogPanel className="probe"><p>dialog-ok</p></DialogPanel> from a page of a real app, against a fork built at the #1817 fix:

'Error loading component'  -> absent     (#1817 fixed, good)
'dynamic component'        -> FOUND      (comment placeholder emitted instead)
'dialog-ok'                -> absent     (slot content destroyed)
'fixed inset-0'            -> absent     (the component's own classes never render)
'probe'                    -> absent     (the className prop never renders)

For contrast, in the same request, components that do not use <component :is> render correctly — <Badge> emits its full class list and slot, <CodeBlock> renders with its client script. So the plugin and the resolver are working; this is specific to the interpolated :is.

Why it hits every affected component

DialogPanel.stx:

<script server>
export const as = $props.as || 'div'
…
</script>

<component :is="{⁠{ as }}" class="{⁠{ baseClasses }}">
  …<slot />…
</component>

The interpolated form is not incidental — it is how all of them are written:

$ grep -rl "<component" packages/components/src/ui/ | wc -l
26
$ grep -rl 'is="{⁠{' packages/components/src/ui/ | wc -l
26

26 of 26. Every dialog/, dropdown/, listbox/, combobox/, popover/, radio-group/ and stepper/ file.

Note also that as defaults to 'div' — a plain HTML tag name. So these components simultaneously exercise the other unaddressed sub-defect from #1817: :is resolving to an HTML tag should render that element rather than be looked up as a component file. Both need to work for the library to function; fixing interpolation alone would just move the failure to "no component file named div".

Repro

<!-- components/Wrap.stx -->
<component :is="{⁠{ as }}" class="x"><slot /></component>
<Wrap as="section"><p>hello</p></Wrap>

Expected: <section class="x"><p>hello</p></section> Actual: a dynamic component comment; <p>hello</p> is gone.

The literal form is worth checking alongside it — #1817's repro B showed processDynamicComponents handling :is="which" correctly in isolation, so the two forms may diverge.

Suggested direction

Resolve {⁠{ … }} in the :is value against the component's own scope before the lookup, and fall back to emitting a plain element when the resolved value is an HTML tag name rather than treating it as a missing component file.

A test rendering one primitive from each affected family through the full processDirectives pipeline and asserting the slot content survives would cover this. The current coverage evidently exercises <component :is> with a literal value only, which is the one form the library never uses.

  • #1817 — ordering fix; this is the remaining half from that report.
14 days ago

Fixed in 708c0bcbcd.

What was actually wrong

Two independent failures, either one fatal on its own — the issue title covers the first.

1. The expression is already interpolated. By the time processDynamicComponents runs, {⁠{ as }} has become the literal div. safeEvaluate read that as a variable name, found nothing, and took the "could not resolve" branch — which replaces the element and swallows its slot content.

2. There was no element path at all. Even when the expression did resolve — :is="as" with as = 'div' in context — the name went to the component loader, which looked for a file called div:

[Error loading component: ENOENT: no such file or directory, open 'div'] Searched paths: …

So :is only ever worked for component files, while every caller in the library passes an element.

Scale — smaller than the title says

The title says 26 primitives. The real number is 9 files, and I should correct that since it changes how urgent this looks:

dialog/DialogPanel, dialog/DialogTitle, dialog/DialogDescription, dialog/DialogBackdrop, radio-group/RadioGroupLabel, radio-group/RadioGroupDescription, stepper/Stepper, stepper/StepperStep, listbox/ListboxLabel.

The earlier count came from a grep that also matched plain <component elsewhere. The blast radius is still real — the whole Dialog family renders as comments, so a delete-confirmation dialog loses its confirmation text — but it is nine components, not twenty-six.

The fix

A name in HTML_TAGS renders as real markup: attributes carried through verbatim, slot content preserved, no closing tag for a void element. Both the paired and self-closing forms — DialogBackdrop uses the latter.

Two deliberate limits:

  • The literal fallback only accepts known element names. Widening it to any bare identifier turns a typo (:is="nope") into a component lookup whose ENOENT puts an absolute filesystem path in the response body. Unresolvable stays unresolvable and keeps its placeholder comment.
  • HTML_TAGS is checked before component resolution, so a component file named div.stx is unreachable through :is. An element name in :is is the documented pattern; a lowercase component file shadowing an HTML tag is not.

Verification

Against the real components, not fixtures — each of the nine files' actual <component :is> line with its own default as:

before: ok=0 broken=9
after:  ok=9 broken=0

  ListboxLabel           as=label  -> <label class="cls"><slot /></label>
  DialogTitle            as=h3     -> <h3 class="cls"><slot /></h3>
  DialogDescription      as=p      -> <p class="cls"><slot /></p>
  DialogBackdrop         as=div    -> <div class="cls" aria-hidden="true"></div>
  …

New suite test/directives/dynamic-component-element.test.ts: 9 tests, 6 of them fail without this change. Full stx suite: 8812 pass, 2 fail — both pre-existing and unrelated (crosswind-arbitrary-values, variable-extractor tilde imports), confirmed identical with and without the change. A downstream app (33 routes, 55 tests) is green against the rebuilt dist.

Not fixed here

<Dialog> still has no focus trap, scroll lock or focus restore, and its Escape handler is bound to the dialog element so it only fires once focus is already inside. Those are component-level gaps, not :is resolution — worth a separate issue if the library is meant to be used for destructive-action confirmations.

14 days ago

Confirming what 708c0bcb fixed and what it did not, measured against a real app on a fork built at that commit.

The two forms now diverge cleanly, which is a useful narrowing:

formresult
<component :is="div" class="x">…renders — <div class="x">…</div>, class preserved
<component :is="{⁠{ asTag }}" class="x">…broken — emits the dynamic component stub, class dropped, children discarded

So the element-name half is done. The interpolated half is the one the component library depends on, and it is untouched: all 26 affected files spell it <component :is="{⁠{ as }}"> (grep -rl 'is="{⁠{' packages/components/src/ui/ | wc -l → 26).

Rendering <DialogPanel className="probe-dp"><p>dialog-ok</p></DialogPanel> from a page still yields nothing usable — no dialog-ok, no probe-dp, and none of the component's own classes (fixed inset-0). <Badge> in the same request renders correctly, so the plugin and resolver are fine; it is specific to the interpolated :is.

Worth noting these two interact: as defaults to 'div' in every one of those components, so once interpolation resolves, the value it produces is an HTML tag name — exactly the case 708c0bcb just taught the resolver to handle. The two halves compose, and the element half landing first means the remaining work is only "resolve {⁠{ … }} against the component's own scope before the lookup".

Not urgent from my side — bughq is not blocked on it, we simply keep hand-rolling those primitives. Flagging only so the 26-primitive claim in the issue body is not read as resolved.

14 days ago

Not a stalemate — the remaining half is fixed in f0ef6d7.

The second comment above narrowed it correctly: 708c0bcb fixed :is="div" and :is="as", and the interpolated form was untouched. That is the spelling in all 26 library files, so the Dialog family still rendered as a comment with its slot discarded.

Why the first fix stopped short

Its premise — "{⁠{ as }} has already been interpolated by the time processDynamicComponents runs" — is true on the page path and false on the component path, which is where all 26 of these live. There the mustache is still raw, so evaluating the expression whole finds nothing and the element path, correct in every other respect, is never reached.

resolveIsName now unwraps the mustache and resolves the inner expression, so the two halves compose exactly as the previous comment predicted: {⁠{ as }} yields div, and div was already handled.

literal div  => <div class="p"><p>ok</p></div>
var as=div   => <div class="p"><p>ok</p></div>
mustache     => <div class="p"><p>ok</p></div>     ← was: could not resolve
mustache h3  => <h3 class="p"><p>ok</p></h3>
unresolvable => <!-- dynamic component: could not resolve "nope" -->

The bug was disguised, and that is worth recording

The placeholder comment embeds the expression it could not resolve. The later expression pass then interpolated the mustache inside that comment, so the page read:

<!-- dynamic component: could not resolve "div" -->

That reads as a resolver being handed a tag name and refusing it — the opposite of what happened, since it never saw a value at all. It cost me a wrong diagnosis until I called the module directly. The comment now reports the inner expression ("nope"), which is accurate and cannot be rewritten.

I suspect this is also why the count in the title read 26 while the first fix measured 9: the interpolated files fail through a path that looks like a resolver rejection rather than a missing feature.

Verification

Against the real library, not fixtures — every .stx under packages/components/src/ui with an interpolated :is, driven with its own default as:

before: ok=0  broken=26
after:  ok=26 broken=0

Five new tests in dynamic-component-element.test.ts, all five failing at the parent commit while the nine existing ones pass. Full suite 10726 pass in a clean worktree; the two failures are the pre-existing crosswind-arbitrary-values case and a package-exports timeout that passes in isolation.

Still not fixed here

The <Dialog> component-level gaps the first comment listed — no focus trap, scroll lock or focus restore, and an Escape handler bound to the dialog element so it only fires once focus is inside. Those are unrelated to :is resolution and still deserve their own issue.

Sign in to comment on this issue.