-
Notifications
You must be signed in to change notification settings - Fork 373
Test React on Rails 17.0.0.rc.7 #775
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -1,9 +1,10 @@ | ||
| 'use client'; | ||
|
|
||
| import React, { useState, Suspense } from 'react'; | ||
| import React, { Suspense, useRef, useState } from 'react'; | ||
| import { ErrorBoundary } from 'react-error-boundary'; | ||
| import RSCRoute from 'react-on-rails-pro/RSCRoute'; | ||
| import { useRSC } from 'react-on-rails-pro/RSCProvider'; | ||
| import { ServerComponentFetchError } from 'react-on-rails-pro/ServerComponentFetchError'; | ||
|
|
||
| // Same shape and dimensions as the rendered LiveActivity card. Local Suspense | ||
| // fallback prevents the RSCRoute suspension from bubbling to an outer | ||
|
|
@@ -23,33 +24,102 @@ function ActivityCardSkeleton() { | |
| ); | ||
| } | ||
|
|
||
| function ActivityErrorFallback({ error, onRetry }) { | ||
| return ( | ||
| <div className="bg-rose-50 border border-rose-200 rounded-lg p-4"> | ||
| <p className="text-rose-700 font-semibold mb-1">Server component fetch failed</p> | ||
| <p className="text-rose-600 text-sm font-mono mb-3">{error.message}</p> | ||
| <button | ||
| type="button" | ||
| onClick={onRetry} | ||
| className="px-3 py-1.5 text-sm bg-rose-600 text-white rounded hover:bg-rose-700" | ||
| > | ||
| Retry | ||
| </button> | ||
| </div> | ||
| ); | ||
| } | ||
|
|
||
| function toServerComponentFetchError(error, componentProps) { | ||
| if (error instanceof ServerComponentFetchError) { | ||
| return error; | ||
| } | ||
|
|
||
| const originalError = error instanceof Error ? error : new Error(String(error)); | ||
| return new ServerComponentFetchError(originalError.message, 'LiveActivity', componentProps, originalError); | ||
| } | ||
|
|
||
| function ThrowActivityError({ error }) { | ||
| throw error; | ||
| } | ||
|
|
||
| function LiveActivityRefresher() { | ||
| const [refreshKey, setRefreshKey] = useState(0); | ||
| const [simulateError, setSimulateError] = useState(false); | ||
| const [refreshCount, setRefreshCount] = useState(0); | ||
| const [routeRefreshKey, setRouteRefreshKey] = useState(0); | ||
| const [fetchError, setFetchError] = useState(null); | ||
| const latestRequestIdRef = useRef(0); | ||
| const { refetchComponent } = useRSC(); | ||
|
|
||
| const nextRequestId = () => { | ||
| latestRequestIdRef.current += 1; | ||
| return latestRequestIdRef.current; | ||
| }; | ||
|
|
||
| const refetchLiveActivity = (componentProps) => | ||
| refetchComponent('LiveActivity', componentProps, true).then((payload) => { | ||
| if (payload instanceof Error) { | ||
| throw payload; | ||
| } | ||
| }); | ||
|
Comment on lines
+61
to
+66
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win 🧩 Analysis chain🌐 Web query:
💡 Result: Based on the current React on Rails Pro documentation and implementation, here are the answers to your questions: 1. Does refetchComponent accept a third boolean parameter? No. The refetchComponent(name, props) method accepts only two parameters: the component name and the component props [1][2][3]. 2. Can its returned promise resolve with an Error instead of rejecting? The behavior of the returned promise depends on the nature of the failure: - Promise Rejection: The underlying RSC fetch typically results in a rejected promise when a network, renderer, or deployment failure occurs [4]. Recent updates have ensured that these failures consistently reject, and the internal cache is updated to evict failed promises so that subsequent retries can succeed rather than returning a "sticky" failed state [4][5]. - Error as a Value: Technical documentation notes that payloads which explicitly resolve with an Error value (rather than throwing/rejecting) are intentionally left in the cache [4]. This is distinct from a failed fetch that causes a rejection [4]. In summary, refetchComponent is designed for error-retry flows where you provide the component name and props; it does not take a third boolean parameter [1][6], and it expects standard promise rejection patterns for fetch failures [4][5]. Citations:
🏁 Script executed: #!/bin/bash
set -euo pipefail
printf 'FILES:\n'
git ls-files | rg 'LiveActivityRefresher\.jsx|refetchComponent|useRSC|react_on_rails|server-components'
printf '\nUSAGES OF refetchComponent:\n'
rg -n "refetchComponent\(" client app . -g '!node_modules' -g '!dist' -g '!build' || true
printf '\nDEFINITIONS/EXPORTS AROUND useRSC:\n'
rg -n "useRSC|refetchComponent" client app . -g '!node_modules' -g '!dist' -g '!build' || trueRepository: shakacode/react-webpack-rails-tutorial Length of output: 2370 🏁 Script executed: #!/bin/bash
set -euo pipefail
printf 'LIVEACTIVITYREFRESHER CONTEXT:\n'
cat -n client/app/bundles/server-components/components/LiveActivityRefresher.jsx | sed -n '1,140p'
printf '\nREADME CONTEXT:\n'
sed -n '210,250p' README.md
printf '\nREACT-ON-RAILS-PRO INITIALIZER:\n'
cat -n config/initializers/react_on_rails_pro.rbRepository: shakacode/react-webpack-rails-tutorial Length of output: 9589 🏁 Script executed: #!/bin/bash
set -euo pipefail
printf 'PACKAGE/VERSION HINTS:\n'
rg -n "react-on-rails-pro|17\.0\.0-rc\.7|refetchComponent" package.json Gemfile Gemfile.lock yarn.lock pnpm-lock.yaml package-lock.json README.md config client -g '!node_modules' -g '!dist' -g '!build' || true
printf '\nRELEVANT DOCS SEARCH:\n'
rg -n "useRSC\(\)\.refetchComponent|refetchComponent\(name, props|refetchComponent" README.md docs client config -g '!node_modules' -g '!dist' -g '!build' || trueRepository: shakacode/react-webpack-rails-tutorial Length of output: 5982 Drop the extra 🤖 Prompt for AI Agents |
||
|
|
||
| const handleRefresh = () => { | ||
| setSimulateError(false); | ||
| setRefreshKey((k) => k + 1); | ||
| const nextKey = refreshCount + 1; | ||
|
|
||
| nextRequestId(); | ||
| setFetchError(null); | ||
| setRefreshCount(nextKey); | ||
| setRouteRefreshKey(nextKey); | ||
| }; | ||
|
|
||
| const handleSimulateError = () => { | ||
| setSimulateError(true); | ||
| setRefreshKey((k) => k + 1); | ||
| const requestId = nextRequestId(); | ||
| const nextKey = refreshCount + 1; | ||
| const errorProps = { simulateError: true, refreshKey: nextKey }; | ||
|
|
||
| setFetchError(null); | ||
| setRefreshCount(nextKey); | ||
| refetchLiveActivity(errorProps).catch((error) => { | ||
| if (latestRequestIdRef.current === requestId) { | ||
| setFetchError(toServerComponentFetchError(error, errorProps)); | ||
| } | ||
| }); | ||
| }; | ||
|
Comment on lines
84
to
96
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Redundant fetch on "Simulate Error"
Since So every "Simulate Error" click triggers two RSC round-trips: one real fetch that's thrown away, and one error-only fetch used to derive the failure. Net effect (besides the wasted request) is a possible flicker where real stats render for a moment before the error fallback replaces them. Consider not bumping the display
Comment on lines
84
to
96
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
|
||
|
|
||
| // refetchComponent primes the cache with corrected props before resetting | ||
| // the boundary, so the post-reset render hits cache instead of re-fetching. | ||
| const buildRetry = (resetErrorBoundary) => () => { | ||
| const newKey = refreshKey + 1; | ||
| setSimulateError(false); | ||
| setRefreshKey(newKey); | ||
| refetchComponent('LiveActivity', { simulateError: false, refreshKey: newKey }) | ||
| // eslint-disable-next-line no-console | ||
| .catch((err) => console.error('Retry refetch failed:', err)) | ||
| .finally(() => resetErrorBoundary()); | ||
| const buildBoundaryRetry = (resetErrorBoundary) => () => { | ||
| const requestId = nextRequestId(); | ||
| const newKey = refreshCount + 1; | ||
| const correctedProps = { simulateError: false, refreshKey: newKey }; | ||
|
|
||
| setFetchError(null); | ||
| setRefreshCount(newKey); | ||
| refetchLiveActivity(correctedProps) | ||
| .then(() => { | ||
| if (latestRequestIdRef.current === requestId) { | ||
| setRouteRefreshKey(newKey); | ||
| resetErrorBoundary(); | ||
| } | ||
| }) | ||
| .catch((err) => { | ||
| if (latestRequestIdRef.current === requestId) { | ||
| // eslint-disable-next-line no-console | ||
| console.error('Retry refetch failed:', err); | ||
| setFetchError(toServerComponentFetchError(err, correctedProps)); | ||
| } | ||
| }); | ||
| }; | ||
|
|
||
| const componentProps = { simulateError: false, refreshKey: routeRefreshKey }; | ||
|
|
||
| return ( | ||
| <div className="space-y-3"> | ||
| <div className="flex items-center gap-2"> | ||
|
|
@@ -67,30 +137,24 @@ function LiveActivityRefresher() { | |
| > | ||
| Simulate Error | ||
| </button> | ||
| <span className="text-xs text-slate-500 ml-2">Refresh count: {refreshKey}</span> | ||
| <span className="text-xs text-slate-500 ml-2">Refresh count: {refreshCount}</span> | ||
| </div> | ||
| <ErrorBoundary | ||
| // react-error-boundary's fallbackRender is a render-prop API by design; | ||
| // the closure captures buildRetry which depends on parent state. | ||
| // the closure captures retry state from this component. | ||
| // eslint-disable-next-line react/no-unstable-nested-components | ||
| fallbackRender={({ error, resetErrorBoundary }) => ( | ||
| <div className="bg-rose-50 border border-rose-200 rounded-lg p-4"> | ||
| <p className="text-rose-700 font-semibold mb-1">Server component fetch failed</p> | ||
| <p className="text-rose-600 text-sm font-mono mb-3">{error.message}</p> | ||
| <button | ||
| type="button" | ||
| onClick={buildRetry(resetErrorBoundary)} | ||
| className="px-3 py-1.5 text-sm bg-rose-600 text-white rounded hover:bg-rose-700" | ||
| > | ||
| Retry | ||
| </button> | ||
| </div> | ||
| <ActivityErrorFallback error={error} onRetry={buildBoundaryRetry(resetErrorBoundary)} /> | ||
| )} | ||
| resetKeys={[refreshKey]} | ||
| resetKeys={[routeRefreshKey]} | ||
| > | ||
| <Suspense fallback={<ActivityCardSkeleton />}> | ||
| <RSCRoute componentName="LiveActivity" componentProps={{ simulateError, refreshKey }} /> | ||
| </Suspense> | ||
| {fetchError ? ( | ||
| <ThrowActivityError error={fetchError} /> | ||
| ) : ( | ||
| <Suspense fallback={<ActivityCardSkeleton />}> | ||
| <RSCRoute componentName="LiveActivity" componentProps={componentProps} /> | ||
| </Suspense> | ||
| )} | ||
| </ErrorBoundary> | ||
| </div> | ||
| ); | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -32,7 +32,7 @@ | |
| expect(page).to have_content("Refresh count: 1") | ||
| end | ||
|
|
||
| it "shows the ErrorBoundary fallback when Simulate Error is clicked, then recovers on Retry" do | ||
| it "shows the error recovery fallback when Simulate Error is clicked, then recovers on Retry" do | ||
|
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Per the PR description, this exact spec ( Merging this PR as-is means
|
||
| click_button "Simulate Error" | ||
| expect(page).to have_content("Server component fetch failed") | ||
|
|
||
|
|
||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Version bump to
17.0.0.rc.7— per the PR description, this lane is reported as BLOCKED: one system spec (spec/system/server_components_demo_spec.rb:35) regresses under rc.7 vs. the rc.6 baseline. Given this, and that the RC report explicitly says "Do not count this hard-gate lane as green... until the rc.7 RSC error-boundary regression is fixed, explained, or explicitly waived," this PR shouldn't be merged as-is — worth confirming CI is green after theLiveActivityRefresherworkaround before merging, and that the workaround doesn't just paper over the underlying regression for other (non-simulated) error paths.