@samitouri / QOS-React-1 / commits / 0fba3ecf73

[Fizz] Reset error component stack and fix error messages (#27456)

The way we collect component stacks right now are pretty fragile. We expect that we'll call captureBoundaryErrorDetailsDev whenever an error happens. That resets lastBoundaryErrorComponentStackDev to null but if we don't, it just lingers and we don't set it to anything new then which leaks the previous component stack into the next time we have an error. So we need to reset it in a bunch of places. This is still broken with erroredReplay because it has the inverse problem that abortRemainingReplayNodes can call captureBoundaryErrorDetailsDev more than one time. So the second boundary won't get a stack. We probably should try to figure out an alternative way to carry along the stack. Perhaps WeakMap keyed by the error object. This also fixes an issue where we weren't invoking the onShellReady event if we error a replay. That event is a bit weird for resuming because we're probably really supposed to just invoke it immediately if we have already flushed the shell in the prerender which is always atm. Right now, it gets invoked later than necessary because you could have a resumed hole ready before a sibling in the shell is ready and that's blocked.

Sebastian Markbåge committed Oct 4, 2023 at 16:48 UTC 0fba3ecf73900a1b54ed6d3b0617462ac92d2fef

3 files changed +188 -19

packages/react-dom/src/__tests__/ReactDOMFizzStaticBrowser-test.js

+145

@@ -981,4 +981,149 @@ describe('ReactDOMFizzStaticBrowser', () => {

981

982 expect(getVisibleChildren(container)).toEqual(<div>Hello</div>);

983 });

984 +

985 + // @gate enablePostpone

986 + it('errors if the replay does not line up', async () => {

987 + let prerendering = true;

988 + function Postpone() {

989 + if (prerendering) {

990 + React.unstable_postpone();

991 + }

992 + return 'Hello';

993 + }

994 +

995 + function Wrapper({children}) {

996 + return children;

997 + }

998 +

999 + const lazySpan = React.lazy(async () => {

1000 + await 0;

1001 + return {default: <span />};

1002 + });

1003 +

1004 + function App() {

1005 + const children = (

1006 + <Suspense fallback="Loading...">

1007 + <Postpone />

1008 + </Suspense>

1009 + );

1010 + return (

1011 + <>

1012 + <div>{prerendering ? <Wrapper>{children}</Wrapper> : children}</div>

1013 + <div>

1014 + {prerendering ? (

1015 + <Suspense fallback="Loading...">

1016 + <div>

1017 + <Postpone />

1018 + </div>

1019 + </Suspense>

1020 + ) : (

1021 + lazySpan

1022 + )}

1023 + </div>

1024 + </>

1025 + );

1026 + }

1027 +

1028 + const prerendered = await ReactDOMFizzStatic.prerender(<App />);

1029 + expect(prerendered.postponed).not.toBe(null);

1030 +

1031 + await readIntoContainer(prerendered.prelude);

1032 +

1033 + expect(getVisibleChildren(container)).toEqual([

1034 + <div>Loading...</div>,

1035 + <div>Loading...</div>,

1036 + ]);

1037 +

1038 + prerendering = false;

1039 +

1040 + const errors = [];

1041 + const resumed = await ReactDOMFizzServer.resume(

1042 + <App />,

1043 + JSON.parse(JSON.stringify(prerendered.postponed)),

1044 + {

1045 + onError(x) {

1046 + errors.push(x.message);

1047 + },

1048 + },

1049 + );

1050 +

1051 + expect(errors).toEqual([

1052 + 'Expected the resume to render <Wrapper> in this slot but instead it rendered <Suspense>. ' +

1053 + "The tree doesn't match so React will fallback to client rendering.",

1054 + 'Expected the resume to render <Suspense> in this slot but instead it rendered <span>. ' +

1055 + "The tree doesn't match so React will fallback to client rendering.",

1056 + ]);

1057 +

1058 + // TODO: Test the component stack but we don't expose it to the server yet.

1059 +

1060 + await readIntoContainer(resumed);

1061 +

1062 + // Client rendered

1063 + expect(getVisibleChildren(container)).toEqual([

1064 + <div>Loading...</div>,

1065 + <div>Loading...</div>,

1066 + ]);

1067 + });

1068 +

1069 + // @gate enablePostpone

1070 + it('can abort the resume', async () => {

1071 + let prerendering = true;

1072 + const infinitePromise = new Promise(() => {});

1073 + function Postpone() {

1074 + if (prerendering) {

1075 + React.unstable_postpone();

1076 + }

1077 + return 'Hello';

1078 + }

1079 +

1080 + function App() {

1081 + if (!prerendering) {

1082 + React.use(infinitePromise);

1083 + }

1084 + return (

1085 + <div>

1086 + <Suspense fallback="Loading...">

1087 + <Postpone />

1088 + </Suspense>

1089 + </div>

1090 + );

1091 + }

1092 +

1093 + const prerendered = await ReactDOMFizzStatic.prerender(<App />);

1094 + expect(prerendered.postponed).not.toBe(null);

1095 +

1096 + await readIntoContainer(prerendered.prelude);

1097 +

1098 + expect(getVisibleChildren(container)).toEqual(<div>Loading...</div>);

1099 +

1100 + prerendering = false;

1101 +

1102 + const controller = new AbortController();

1103 +

1104 + const errors = [];

1105 +

1106 + const resumedPromise = ReactDOMFizzServer.resume(

1107 + <App />,

1108 + JSON.parse(JSON.stringify(prerendered.postponed)),

1109 + {

1110 + signal: controller.signal,

1111 + onError(x) {

1112 + errors.push(x);

1113 + },

1114 + },

1115 + );

1116 +

1117 + controller.abort('abort');

1118 +

1119 + const resumed = await resumedPromise;

1120 + await resumed.allReady;

1121 +

1122 + expect(errors).toEqual(['abort']);

1123 +

1124 + await readIntoContainer(resumed);

1125 +

1126 + // Client rendered

1127 + expect(getVisibleChildren(container)).toEqual(<div>Loading...</div>);

1128 + });

1129 });

packages/react-server/src/ReactFizzServer.js

+41 -17

@@ -1110,7 +1110,6 @@ function replaySuspenseBoundary(

1110 // on preparing fallbacks if we don't have any more main content to task on.

1111 request.pingedTasks.push(suspendedFallbackTask);

1112

1113 - // TODO: Should this be in the finally?

1113 popComponentStackInDEV(task);

1114 }

1115

@@ -1953,17 +1952,19 @@ function replayElement(

1952 if (keyOrIndex !== node[1]) {

1953 continue;

1954 }

1956 - // Let's double check that the component name matches as a precaution.

1957 - if (name !== null && name !== node[0]) {

1958 - throw new Error(

1959 - 'Expected to see a component of type "' +

1960 - name +

1961 - '" in this slot. ' +

1962 - "The tree doesn't match so React will fallback to client rendering.",

1963 - );

1964 - }

1955 if (node.length === 4) {

1956 // Matched a replayable path.

1957 + // Let's double check that the component name matches as a precaution.

1958 + if (name !== null && name !== node[0]) {

1959 + throw new Error(

1960 + 'Expected the resume to render <' +

1961 + (node[0]: any) +

1962 + '> in this slot but instead it rendered <' +

1963 + name +

1964 + '>. ' +

1965 + "The tree doesn't match so React will fallback to client rendering.",

1966 + );

1967 + }

1968 const childNodes = node[2];

1969 const childSlots = node[3];

1970 task.replay = {nodes: childNodes, slots: childSlots, pendingTasks: 1};

@@ -2009,8 +2010,13 @@ function replayElement(

2010 } else {

2011 // Let's double check that the component type matches.

2012 if (type !== REACT_SUSPENSE_TYPE) {

2013 + const expectedType = 'Suspense';

2014 throw new Error(

2013 - 'Expected to see a Suspense boundary in this slot. ' +

2015 + 'Expected the resume to render <' +

2016 + expectedType +

2017 + '> in this slot but instead it rendered <' +

2018 + (getComponentNameFromType(type) || 'Unknown') +

2019 + '>. ' +

2020 "The tree doesn't match so React will fallback to client rendering.",

2021 );

2022 }

@@ -2378,6 +2384,7 @@ function replayFragment(

2384 // in the original prerender. What's unable to complete is the child

2385 // replay nodes which might be Suspense boundaries which are able to

2386 // absorb the error and we can still continue with siblings.

2387 + // This is an error, stash the component stack if it is null.

2388 erroredReplay(request, task.blockedBoundary, x, childNodes, childSlots);

2389 } finally {

2390 task.replay.pendingTasks--;

@@ -2849,6 +2856,7 @@ function renderNode(

2856 if (__DEV__) {

2857 task.componentStack = previousComponentStack;

2858 }

2859 + lastBoundaryErrorComponentStackDev = null;

2860 return;

2861 }

2862 }

@@ -2930,6 +2938,7 @@ function erroredTask(

2938 errorDigest = logRecoverableError(request, error);

2939 }

2940 if (boundary === null) {

2941 + lastBoundaryErrorComponentStackDev = null;

2942 fatalError(request, error);

2943 } else {

2944 boundary.pendingTasks--;

@@ -2949,6 +2958,8 @@ function erroredTask(

2958 // We reuse the same queue for errors.

2959 request.clientRenderedBoundaries.push(boundary);

2960 }

2961 + } else {

2962 + lastBoundaryErrorComponentStackDev = null;

2963 }

2964 }

2965

@@ -3077,7 +3088,6 @@ function abortTask(task: Task, request: Request, error: mixed): void {

3088 }

3089

3090 if (boundary === null) {

3080 - request.allPendingTasks--;

3091 if (request.status !== CLOSING && request.status !== CLOSED) {

3092 const replay: null | ReplaySet = task.replay;

3093 if (replay === null) {

@@ -3085,6 +3095,7 @@ function abortTask(task: Task, request: Request, error: mixed): void {

3095 // the request;

3096 logRecoverableError(request, error);

3097 fatalError(request, error);

3098 + return;

3099 } else {

3100 // If the shell aborts during a replay, that's not a fatal error. Instead

3101 // we should be able to recover by client rendering all the root boundaries in

@@ -3101,6 +3112,12 @@ function abortTask(task: Task, request: Request, error: mixed): void {

3112 errorDigest,

3113 );

3114 }

3115 + request.pendingRootTasks--;

3116 + if (request.pendingRootTasks === 0) {

3117 + request.onShellError = noop;

3118 + const onShellReady = request.onShellReady;

3119 + onShellReady();

3120 + }

3121 }

3122 }

3123 } else {

@@ -3137,12 +3154,12 @@ function abortTask(task: Task, request: Request, error: mixed): void {

3154 abortTask(fallbackTask, request, error),

3155 );

3156 boundary.fallbackAbortableTasks.clear();

3157 + }

3158

3141 - request.allPendingTasks--;

3142 - if (request.allPendingTasks === 0) {

3143 - const onAllReady = request.onAllReady;

3144 - onAllReady();

3145 - }

3159 + request.allPendingTasks--;

3160 + if (request.allPendingTasks === 0) {

3161 + const onAllReady = request.onAllReady;

3162 + onAllReady();

3163 }

3164 }

3165

@@ -3365,6 +3382,7 @@ function retryRenderTask(

3382 logPostpone(request, postponeInstance.message);

3383 trackPostpone(request, trackedPostpones, task, segment);

3384 finishedTask(request, task.blockedBoundary, segment);

3385 + lastBoundaryErrorComponentStackDev = null;

3386 return;

3387 }

3388 }

@@ -3452,6 +3470,12 @@ function retryReplayTask(request: Request, task: ReplayTask): void {

3470 task.replay.nodes,

3471 task.replay.slots,

3472 );

3473 + request.pendingRootTasks--;

3474 + if (request.pendingRootTasks === 0) {

3475 + request.onShellError = noop;

3476 + const onShellReady = request.onShellReady;

3477 + onShellReady();

3478 + }

3479 request.allPendingTasks--;

3480 if (request.allPendingTasks === 0) {

3481 const onAllReady = request.onAllReady;

scripts/error-codes/codes.json

+2 -2

@@ -474,8 +474,8 @@

474 "486": "It should not be possible to postpone at the root. This is a bug in React.",

475 "487": "We should not have any resumable nodes in the shell. This is a bug in React.",

476 "488": "Couldn't find all resumable slots by key/index during replaying. The tree doesn't match so React will fallback to client rendering.",

477 - "489": "Expected to see a component of type \"%s\" in this slot. The tree doesn't match so React will fallback to client rendering.",

478 - "490": "Expected to see a Suspense boundary in this slot. The tree doesn't match so React will fallback to client rendering.",

477 + "489": "Expected the resume to render <%s> in this slot but instead it rendered <%s>. The tree doesn't match so React will fallback to client rendering.",

478 + "490": "Expected the resume to render <%s> in this slot but instead it rendered <%s>. The tree doesn't match so React will fallback to client rendering.",

479 "491": "It should not be possible to postpone both at the root of an element as well as a slot below. This is a bug in React.",

480 "492": "The \"react\" package in this environment is not configured correctly. The \"react-server\" condition must be enabled in any environment that runs React Server Components.",

481 "493": "To taint a value, a lifetime must be defined by passing an object that holds the value.",