test(react-start): cover SPA shell hydration in the spa-mode e2e app - #8474
Open
christian-apollo wants to merge 1 commit into
Open
christian-apollo wants to merge 1 commit into
christian-apollo wants to merge 1 commit into
Conversation
The spa-mode app is served in its e2e run by `serve dist/client`, which answers an unknown address with its own 404 page. The shell is therefore never loaded at an address it was not prerendered for, which is the normal case for a deployed SPA, and nothing in the suite listens for uncaught errors, so the React TanStack#418 that `/` already throws goes unnoticed. `spa-server.mjs` serves the build the way a static host does: an existing file wins, a directory's index.html wins, anything else falls back to the shell with a 200. That keeps the prerendered /posts/1 reachable, which `serve --single` would not, and makes the fallback reachable, which plain `serve` does not. The new spec asserts that a page load throws nothing uncaught. The prerendered page passes. The two shell cases are marked `test.fail`, since both throw React TanStack#418 today (issue TanStack#8473); removing the marker is how a fix for that issue proves itself.
Contributor
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Repository: TanStack/router/.coderabbit.yaml Review profile: CHILL Plan: Advanced Run ID: 📒 Files selected for processing (3)
Included review availability: Your plan provides up to 10 included reviews per hour; 9 remain after this review. 📝 WalkthroughWalkthroughThe SPA test setup now launches a custom Node server instead of ChangesSPA server and hydration
Priority: ⬇️ Low Estimated code review effort: 2 (Simple) | ~10 minutes Change: Other Sequence Diagram(s)sequenceDiagram
participant Browser
participant spa-server.mjs
participant Filesystem
participant hydration.spec.ts
Browser->>spa-server.mjs: Request SPA route
spa-server.mjs->>Filesystem: Resolve route file or index.html
Filesystem-->>spa-server.mjs: Return file path
spa-server.mjs-->>Browser: Stream response
hydration.spec.ts->>Browser: Navigate and capture page errors
Browser-->>hydration.spec.ts: Root heading and error messages
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
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
1 participant
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.
🎯 Changes
Test-only. Adds the missing coverage for hydrating the SPA shell, which is how #8473 stayed invisible.
The
spa-modee2e app is served byserve dist/client, which answers an unknown address with its own 404 page, so the shell is never loaded at an address it was not prerendered for: the normal case for a deployed SPA. Nothing in the suite listens for uncaught errors either, so the React #418 that/already throws goes unnoticed.spa-server.mjsserves the build the way a static host does: an existing file wins, a directory'sindex.htmlwins, anything else falls back to the shell with a 200 (try_files $uri $uri/index.html /index.html). Plainservecannot reach the fallback andserve --singlecannot reach the prerendered/posts/1, so the two cases need this ~40 line server. It also drops annpx servedownload from the e2e run.tests/hydration.spec.tsasserts a page load throws nothing uncaught, for three addresses: the prerendered/posts/1, which passes; and the shell at/and at/no-such-address, which are markedtest.failbecause both throwMinified React error #418today.I marked the two rather than leaving CI red: they pass as expected-failures now, and a fix for #8473 makes them pass unexpectedly, which fails the run and is the prompt to drop the marker. Happy to change that to skips, or to drop them entirely and keep only the passing control, if you would rather the bug were tracked only in the issue.
The existing two tests pass unchanged against the new server.
✅ Checklist
🚀 Release Impact
Summary by CodeRabbit
New Features
Tests