Repository navigation
fix(web): do not cache a server component that throws during render; trim frames/server exports - #3961
Merged
Conversation
respond() headers describe the success. A synchronous throw is still a 200 whose body carries the error, so an author's cache-control stored the failure. The render marks that response and the transport finalizer sends it no-store, then strips the mark. A returned error value keeps the headers the author set. Co-authored-by: Grok via Cursor <noreply@cursor.com> Co-authored-by: Cursor <cursoragent@cursor.com>
🦋 Changeset detectedLatest commit: 81ae09d The changes in this PR will be included in the next version bump. This PR includes changesets to release 12 packages
Not sure what this means? Click here to learn what changesets are. Click here if you're a maintainer who wants to add another changeset to this PR |
Merging this PR will improve performance by 14.64%
|
| Benchmark | BASE |
HEAD |
Efficiency | |
|---|---|---|---|---|
| ⚡ | memo + sync render effect only (reference) |
30.8 ms | 26.6 ms | +15.5% |
| ⚡ | projection derive: write one NESTED field (reference) |
2.4 ms | 2.1 ms | +13.79% |
Tip
Curious why performance improved? Comment @codspeedbot explain why performance improved on this PR, or directly use the CodSpeed MCP with your agent.
Comparing fix/frame-error-not-cached (81ae09d) with next (2322d64)
The frames server artifact resolves its shared.js imports through the server-functions server entry. FAILED_VALUE_HEADER was read there and never re-exported, so example SSR builds failed with a missing export. Co-authored-by: Grok via Cursor <noreply@cursor.com> Co-authored-by: Cursor <cursoragent@cursor.com>
Size (brotli, eager entry chunk)
|
A null literal asserted non-null is never, so user!.id failed the package typecheck. The value stays null at runtime. Co-authored-by: Grok via Cursor <noreply@cursor.com> Co-authored-by: Cursor <cursoragent@cursor.com>
Coverage Report for CI Build 38296501253Coverage remained the same at 77.429%Details
Uncovered ChangesNo uncovered changes found. Coverage RegressionsNo coverage regressions found. Coverage Stats
💛 - Coveralls |
…onse Rework of the internal-mark approach: no new export, no internal header and no change to the public renderServerComponent result or createFrameSink hooks. serverComponentResponse already sees every record of the sync pass in its own write; an unkeyed error there makes the response no-store before the head is built, and drops CDN-Cache-Control, <CDN>-CDN-Cache-Control and Surrogate-Control, which would override Cache-Control at a CDN (RFC 9213). A direct serverComponentResponse caller gets the same policy. FAILED_VALUE_HEADER and the finalizer change are reverted. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_017PDt8Jv6uf5xRDsrux4UwF
renderServerComponent, renderToFrameStream, serverComponentResponse and createFrameSink were exported from @solidjs/web/frames/server, but apps never call them: a server component is returned from a server function (directly or through respond()) and the transform hooks frame it. Keep the hooks and document-shell pieces as the entry's surface. The direct serverComponentResponse case leaves the not-cached spec, and the two built-package probes now reach frames through the server-function handler, the way an app does. Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_018LY7F9czoRqdMEvk2QwPr1
ryansolid
changed the title
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Requested by Ryan · project thread
Before:
respond(View, { headers: { "cache-control": "public, max-age=60" } })copied those headers onto the frame response, and a throw during the component's synchronous render (user.idonnull) still went out as a 200 carrying the:errorrecord, so the author'sCache-Controlstored the failure for a minute. Separately,@solidjs/web/frames/serverexported four internal building blocks (renderServerComponent,renderToFrameStream,serverComponentResponse,createFrameSink) that apps have no reason to call.After: a server component that throws during its synchronous render is sent
Cache-Control: no-store, with the CDN-targeted cache fields removed. The status stays 200 and the body still carries the error record. The four building blocks are no longer exported; a server component reaches the client by being returned from a server function, directly or throughrespond(), and the transform hooks frame it.How
The frame response is built inside the frames package, which already sees every record of the synchronous render as it writes the body, before the
Responseexists. An unkeyederrorrecord there makes the headCache-Control: no-storeand dropsCDN-Cache-Control,<CDN>-CDN-Cache-Control(RFC 9213) andSurrogate-Control. This is 19 lines inframe-sink.ts, with no new export, header or hook.{ error }value keeps the author's headers. The author chose that value and the headers that travel with it.Public API
The only change is a removal from the experimental
@solidjs/web/frames/serverentry:renderServerComponent,renderToFrameStream,serverComponentResponseandcreateFrameSink. It keepsframeTransformResult,frameTransformFlightResult,frameTransformDirectResult,ServerComponentPlugin,SERVER_COMPONENT_BOOTSTRAP, the frame header constants,isFrameStreamResponseand the slot types. Nothing in the repo or its examples imports the removed four from the public entry, and@solidjs/vite-plugin(3.0.0-next.35 and next.49) uses only the transform hooks andServerComponentPlugin.The first revision of this PR added an internal mark header (
X-Solid-Failed-Value), re-exportedFAILED_VALUE_HEADER, and changed the transport finalizer. All of that was reverted.Test plan
frame-error-not-cached.spec.tsx(5 cases, all through the server-function handler): a sync throw is 200 andno-storewith the error record; it drops the CDN fields and keepsVary; success keepspublic, max-age=60and the CDN fields;respond({ error }, cacheable)keeps its headers. The throw cases fail withnext'sframe-sink.ts.renderServerComponentfrom the public entry (dist-frames-server-instance.spec.ts,ssr-error-sanitization.fixture.mjs) now reach frames throughhandleServerFunctionRequestwithframeTransformResult, the way an app does. Both pass in every tier, with the same sanitization findings as before.nextbecause of the Node version.test-typesis clean.🤖 Generated with Claude Code
https://claude.ai/code/session_018LY7F9czoRqdMEvk2QwPr1