ReviewOS

stacks/stx

fix(process): a <script server> that throws renders the component as nothing, silently

#1991
Closed glennmichael123 opened this 9 days ago ·
9 days ago

Summary

When a component's <script server> throws, the component renders as nothing. No error, no fallback, no message — even with debug: true. The page simply has a hole where the UI was.

Reproduction

<CodeBlock code="const x = 1" /> where its server block cannot load its highlighter:

<div data-stx-scope="stx_code_block_1_2cmbai" data-stx-props="{&quot;code&quot;:&quot;const x = 1&quot;}">
  <div class="relative group">
  </div>
</div>

The markup skeleton survives; everything the server block computed is gone, and every that depended on it resolves to nothing. Console output with debug: true: nothing.

Why this is expensive

The symptom points nowhere near the cause. In this case the real failure was ts-syntax-highlighter@0.2.17 shipping a bundle that imports bunfig while declaring no dependencies, so a static import threw wherever that package was not reachable. What the developer sees is an empty div. Finding the actual error took instrumenting the pipeline by hand.

It also hides upstream breakage indefinitely: a dependency can start throwing and every page keeps rendering "successfully", just without that component.

Ask

  1. Log the error when a <script server> block throws — file, component name and the original stack. Unconditionally at debug: true, and at least once per process otherwise.
  2. In development, render a visible error boundary in place of the component rather than an empty element.
  3. Consider surfacing it through the existing @errorBoundary machinery, so an app can decide what a failed component looks like in production.
9 days ago

Ask 1 is done in 81f6dd8: the warning is unconditional now, once per (file, cause), naming the file and the cause and saying what it costs. It was behind STX_DEBUG, which nobody sets while looking at a page that merely has a gap in it.

Kept open for asks 2 and 3, which are a different kind of change:

  • A visible error boundary in development. Needs a decision about what replaces the component — the scope wrapper is already emitted by the time the script has failed, so this is a render-path change rather than a logging one.
  • Routing it through @errorBoundary. Worth doing, and it subsumes 2: an app that has declared a boundary should get its own fallback rather than a built-in one, in dev and in production.

Two notes from implementing the log:

  • The silence had one legitimate case — a server script reaching for a browser global — and it was already classified separately, so nothing that reaches the new branch is anything but a bug. That is what made the unconditional warning safe.
  • The warning's claim that "every variable in that script is undefined" is literally true: a declaration ahead of the throw does not survive the fallback either. That is why the hole is total rather than partial, and it is pinned in packages/stx/test/server-script-throw-is-reported.test.ts.
9 days ago

Asks 1 and 2 are done. Ask 3 is not, deliberately, and is the one worth discussing.

Correction first

Ask 1 was already done before I started, in 81f6dd8248 fix(process): a server script that throws says so instead of leaving a hole. The warnings are unconditional now, classified by failure type, once per distinct cause, with the file and the cause. My first attempt at reproducing this looked completely silent because I only intercepted console.error, and they go to console.warn. So the "no error, no message, even with debug: true" part of the report has not been true for a while.

But the classifier was missing the message your case produces

Worth knowing, because it means ask 1 was only mostly done. Only require() produces "cannot find module". An ESM import produces:

Cannot find package 'bunfig' imported from /app/dist/index.js

and isModuleResolutionFailure matched cannot find module|could not resolve|module not found|failed to resolve, so it did not match that. Your exact scenario, ts-syntax-highlighter importing bunfig without declaring it, was therefore classified as an unknown failure and got the generic threw, so every variable… warning rather than imports a module that does not resolve. The generic branch is also deduplicated per cause, so the clearest failure in the set was getting the quietest treatment. Fixed.

Ask 2: the boundary

extractVariables warns and returns normally on purpose, since one failed script must not take the page down, but that left nothing able to find out, which is why the hole could not be wrapped. It now takes an onFailure option reporting the cause and a classification, and extractServerScriptVariables renders a boundary in place of the failing script:

<div data-stx-server-script-error role="alert" style="…">
  <strong>stx: a &lt;script server&gt; imports a module that does not resolve, so this component rendered nothing.</strong>
  <span>/…/CodeBlock.stx</span>
  <span>Cannot find package 'ts-syntax-highlighter-missing' imported from …</span>
</div>

Inline styles, because a boundary that depends on the app's CSS is invisible exactly when the app is what is broken. The file and cause are escaped rather than spliced.

Three gates, each one load-bearing:

  • Never in production. A page must not grow a diagnostic.
  • Never under test. It would rewrite the output of every suite that renders a deliberately broken script. Verified rather than assumed: bun test sets NODE_ENV=test, so this is off unless a test opts in.
  • Never for the unknown classification. That is where a server script reaching for window, or constructing something browser-only, lands. It is the case the quiet fallback exists for, and a boundary on each of those would be wrong on pages that work.

Ask 3, and why I left it

Routing this through @errorBoundary is a production behaviour decision rather than a diagnostic one: it means choosing what a failed component looks like to a user, whether a boundary suppresses the 200, and whether an app can opt into failing the render instead. The machinery exists in error-boundaries.ts and nothing here conflicts with it. Happy to open a separate issue for it if you want that, since it is a feature rather than the missing diagnostic this issue is about.

Verification

12 tests, 6 of which fail without the change; the other 6 assert absence, which was already true. 287 tests across the server-script and variable-extractor suites pass, tsc --noEmit clean, lint clean.

9 days ago

Ask 3 done in 8ffdd721cf, so this is complete.

I said last time this was a production behaviour decision rather than a diagnostic one, and that is still the right read — which is why it is built as your markup rather than ours. A boundary with a failed server script inside it now shows its @fallback, in production, with no diagnostic involved.

What was missing

The boundary already shipped both halves and flipped them from the client. A <script server> that failed never reaches the client runtime — it failed before the page was sent — so the component rendered empty inside a boundary whose fallback sat there hidden with style="display: none;". The one case a boundary exists for was the one it could not cover.

Three pieces

The component marks its own failure. <!--stx-render-failed:KIND--> prefixed to its output. A marker rather than a flag on the context, because locality is the whole point: a page with two boundaries and one failure must flip exactly one, and a page-wide flag cannot express that. There is a test for precisely that shape.

The boundary reads it and emits data-has-error="true" with the fallback visible. The failed content is dropped rather than hidden — it is markup with every value undefined, so leaving it in the document means a screen reader still reads it and a :show still binds to it.

processDirectives strips any marker no boundary claimed, so a component failing outside a boundary keeps today's behaviour exactly, and nothing reaches a browser either way.

The gate, same as ask 2

unknown is excluded. A server script reaching for window, or building something browser-only, classifies there, and it is the case the quiet fallback exists for — replacing those components with a fallback would break pages that work. There is a test with a window.localStorage read asserting the boundary stays closed on it.

One thing worth knowing

Ask 2's development boundary fires for a page-level <script server>. A component's failing script took a different path (renderComponentWithSlot) which never had the onFailure hook wired, so the dev boundary did not appear for components — it rendered as an empty element. That path is where this change hooks in, so a component failure is now visible in both modes: the dev boundary where there is no @errorBoundary, your fallback where there is.

7 tests, covering the failure, the working component, the browser-only exclusion, the no-boundary case, marker containment in both directions, and two boundaries with one failure. Suite green: 14,187 pass.

Sign in to comment on this issue.