refactor(app-router): delegate RSC preload hint normalization#952
Conversation
commit: |
There was a problem hiding this comment.
Pull request overview
Refactors the App Router RSC virtual entry generation to delegate React Flight preload hint stream normalization to a dedicated, typed server helper, making the generated entry thinner and enabling direct unit testing of the stream behavior.
Changes:
- Added
normalizeReactFlightPreloadHints()stream transformer helper inpackages/vinext/src/server/rsc-stream-hints.ts. - Updated the generated App Router RSC entry template to import and delegate to the helper instead of inlining a
TransformStreamimplementation. - Added/updated tests and snapshots to assert delegation and to directly test chunk-splitting + flush behavior.
Reviewed changes
Copilot reviewed 6 out of 6 changed files in this pull request and generated no comments.
Show a summary per file
| File | Description |
|---|---|
| tests/entry-templates.test.ts | Adds an assertion that generateRscEntry() imports and delegates to the new helper (and no longer inlines the regex/transform). |
| tests/app-rsc-stream-hints.test.ts | New unit tests validating hint normalization across complete lines, split chunks, and final unterminated line flush. |
| tests/app-router.test.ts | Updates integration assertions to reflect delegation (removes inline-regex replication test). |
| tests/snapshots/entry-templates.test.ts.snap | Updates snapshots to match the new imported helper + one-line wrapper. |
| packages/vinext/src/server/rsc-stream-hints.ts | Introduces the stream-based normalization helper using a buffering TransformStream. |
| packages/vinext/src/entries/app-rsc-entry.ts | Updates the generated RSC entry template to import and call the helper. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
|
/bigbonk review |
![ask-bonk[bot]](https://avatars.githubusercontent.com/in/2454580?s=60&v=4){kind=small}
There was a problem hiding this comment.
Clean extraction — the generated entry reads much better as a one-line delegation, the helper is well-typed, and the new stream-level tests cover buffering semantics (cross-chunk splits, flush) that the old regex-only tests couldn't. CI is green.
One substantive issue: the old app-router.test.ts test had ~10 regex edge-case assertions that were deleted but only partially replaced. The new stream tests cover the happy path well, but several edge cases that guard against plausible regex regressions are now untested. Details below.
| @@ -0,0 +1,38 @@ | |||
| const REACT_FLIGHT_STYLESHEET_PRELOAD_HINT = /(\d*:HL\[.*?),"stylesheet"(\]|,)/g; | |||
There was a problem hiding this comment.
Nit: the module-level /g regex is safe here because String.prototype.replace resets lastIndex on each call — just noting for anyone reading later. Good pattern.
| if (text) controller.enqueue(encoder.encode(text.replace(_hlFixRe, '$1,"style"$2'))); | ||
| } | ||
| })); | ||
| return __normalizeReactFlightPreloadHints(_renderToReadableStream(model, options)); |
There was a problem hiding this comment.
The wrapper is nicely thin. One thing I noticed: the old inline code had const stream = _renderToReadableStream(model, options) as a separate variable, which made it clear the call happens exactly once. This one-liner is equivalent and arguably more readable — just confirming there are no cases where the return value of _renderToReadableStream is used elsewhere before being piped. Looks correct as-is.
|
Review posted. Here's a summary of what I flagged: Overall: Good refactoring — the generated entry is much thinner, the helper is well-typed and properly encapsulated, and the new stream-level tests add valuable coverage for cross-chunk buffering and flush semantics that the old regex-only tests couldn't cover. CI is fully green. One substantive issue: The old
Suggested adding one "realistic mixed content" test case that collapses all four gaps. Minor suggestions: Add a doc comment to |
There was a problem hiding this comment.
Pull request overview
This PR refactors the App Router RSC entry generation so React Flight preload hint (“HL”) normalization is delegated to a typed server helper, keeping generated entry code focused on wiring and improving direct testability of the stream behavior.
Changes:
- Added
normalizeReactFlightPreloadHints()stream helper to rewrite Flight HL"stylesheet"→"style"while buffering split lines safely. - Updated
generateRscEntry()output to import and delegate to the helper instead of embedding an inlineTransformStreamimplementation. - Added focused unit tests for the helper and updated existing template/assertion tests and snapshots accordingly.
Reviewed changes
Copilot reviewed 6 out of 6 changed files in this pull request and generated no comments.
Show a summary per file
| File | Description |
|---|---|
| tests/entry-templates.test.ts | Adds an assertion that generated RSC entry delegates hint normalization (and no longer inlines the regex/transform). |
| tests/app-rsc-stream-hints.test.ts | New unit tests covering hint rewriting, mixed content, split chunks, and unterminated final line flush behavior. |
| tests/app-router.test.ts | Updates the “RSC Flight hint fix” test to validate delegation/import rather than inline regex behavior. |
| tests/snapshots/entry-templates.test.ts.snap | Updates snapshots to reflect the delegated helper import + wrapper. |
| packages/vinext/src/server/rsc-stream-hints.ts | Introduces the stream normalization helper with buffering + transform logic. |
| packages/vinext/src/entries/app-rsc-entry.ts | Adds resolved import path for the helper and replaces inline transform with a one-line delegation wrapper. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
|
/bigbonk review |
There was a problem hiding this comment.
Clean refactor. The generated entry is much thinner, the helper is well-encapsulated with proper typing, and the stream-level tests cover the important behavioral properties (cross-chunk buffering, flush, mixed content, idempotency) that pure regex tests couldn't.
CI is fully green across all required checks.
The second commit addresses the previous review feedback correctly — the mixed-content test (covering non-HL stylesheet lines, multiple HL records, already-"style" hints, and no-prefix options) and the doc comment on normalizeReactFlightHintLine are both in place.
One minor note on the module-level /g regex: it's safe because String.prototype.replace resets lastIndex on each call, so there's no statefulness leak across transform/flush invocations. Verified.
LGTM — good to merge.
|
Approved. Here's a summary of the review: Verdict: APPROVE This is a clean, well-scoped refactor that moves the React Flight preload hint stream normalization from inline generated code into a typed helper module (
CI is fully green across all 20+ checks. The module-level |
The App Router RSC entry currently owns the React Flight preload hint stream transform inline. That keeps byte-stream buffering logic inside generated code, so the behavior is only indirectly covered by generated-entry snapshots. Move the stream normalization into a typed server helper and keep the generated RSC entry as a thin wrapper around renderToReadableStream. Add direct coverage for complete lines, split chunks, and final unterminated Flight hint lines.
NathanDrake2406
force-pushed
the
nathan/rsc-stream-hints
branch
from
47218d0 to
36e34fe
Compare
What this changes
The App Router RSC entry now delegates React Flight preload hint stream normalization to a typed server helper instead of carrying the TransformStream implementation inline in generated code.
Why
The generated RSC entry should mostly contain import tables, manifests, and thin runtime wiring. Keeping byte-stream buffering and hint rewriting inside the entry made the behavior harder to test directly and kept generated snapshots responsible for runtime stream semantics.
Approach
normalizeReactFlightPreloadHints()inpackages/vinext/src/server/rsc-stream-hints.ts.generateRscEntry()to import that helper and keeprenderToReadableStream()as a one-line delegation wrapper.Validation
vp test run tests/app-rsc-stream-hints.test.ts tests/entry-templates.test.tsvp test run tests/app-router.test.ts -t "RSC Flight hint fix"vp check tests/app-rsc-stream-hints.test.ts tests/entry-templates.test.ts tests/app-router.test.ts packages/vinext/src/server/rsc-stream-hints.ts packages/vinext/src/entries/app-rsc-entry.tstests/entry-templates.test.ts,vp check --fix, andknip --no-progressRisks / follow-ups
This is intentionally limited to the low-conflict stream hint helper slice. It avoids metadata, middleware, ISR, route-handler cache, server action, and boundary code paths that currently have active overlapping PRs.