Skip to content

Commit

Permalink
Use a Wrapper Error for onRecoverableError with a "cause" Field for t…
Browse files Browse the repository at this point in the history 
…he real Error (facebook#28736)

We basically have four kinds of recoverable errors:

- Hydration mismatches.
- Server errored but client didn't.
- Hydration render errored but client render didn't (in Root or Suspense
boundary).
- Concurrent render errored but synchronous render didn't.

For the first three we log an additional error that the root or Suspense
boundary didn't error. This provides some context about what happened.
However, the problem is that for hydration mismatches that's unnecessary
extra context that is confusing. We also don't log any additional
context for concurrent render errors that could recover. This used to be
the only recoverable error so it didn't need extra context but now we
need to distinguish them. When we log these to `reportError` it's
confusing to just see the error because you didn't see anything error on
the page. It's also hard to group them together as one.

In this PR, I remove the unnecessary context for hydration mismatches.

For hydration and concurrent errors, I now wrap them in an error that
describes that what happened but then use the new `cause` field to link
the original error so we can keep that as the cause. The error that
happened was that hydration client rendered or you deopted to sync
render, the cause of that error is some other error.

For server errors, we control the Error object so I already had added
some context to that error object's message. Since we hide the message
in prod, it's nice not to have the raw message in DEV neither. We could
potentially split these into two errors for parity though.
  • Loading branch information
@sebmarkbage @AndyPengc12
sebmarkbage authored and AndyPengc12 committed Apr 15, 2024
1 parent 948608d commit 6eacde8
Show file tree
Hide file tree
Showing 16 changed files with 437 additions and 286 deletions.
170 changes: 97 additions & 73 deletions packages/react-dom/src/__tests__/ReactDOMFizzServer-test.js
View file

Large diffs are not rendered by default.

13 changes: 11 additions & 2 deletions packages/react-dom/src/__tests__/ReactDOMFizzShellHydration-test.js
View file
Original file line number Diff line number Diff line change
Expand Up @@ -397,6 +397,9 @@ describe('ReactDOMFizzShellHydration', () => {
},
onRecoverableError(error) {
Scheduler.log('onRecoverableError: ' + error.message);
if (error.cause) {
Scheduler.log('Cause: ' + error.cause.message);
}
},
});
});
Expand Down Expand Up @@ -462,6 +465,9 @@ describe('ReactDOMFizzShellHydration', () => {
},
onRecoverableError(error) {
Scheduler.log('onRecoverableError: ' + error.message);
if (error.cause) {
Scheduler.log('Cause: ' + error.cause.message);
}
},
});
});
Expand Down Expand Up @@ -529,13 +535,16 @@ describe('ReactDOMFizzShellHydration', () => {
},
onRecoverableError(error) {
Scheduler.log('onRecoverableError: ' + error.message);
if (error.cause) {
Scheduler.log('Cause: ' + error.cause.message);
}
},
});
});

assertLog([
'onRecoverableError: plain error',
'onRecoverableError: There was an error while hydrating. Because the error happened outside of a Suspense boundary, the entire root will switch to client rendering.',
'onRecoverableError: There was an error while hydrating but React was able to recover by instead client rendering the entire root.',
'Cause: plain error',
]);
expect(container.textContent).toBe('Hello world');
});
Expand Down
81 changes: 55 additions & 26 deletions packages/react-dom/src/__tests__/ReactDOMFizzSuppressHydrationWarning-test.js
View file
Original file line number Diff line number Diff line change
Expand Up @@ -165,7 +165,10 @@ describe('ReactDOMFizzServerHydrationWarning', () => {
ReactDOMClient.hydrateRoot(container, <App isClient={true} />, {
onRecoverableError(error) {
// Don't miss a hydration error. There should be none.
Scheduler.log(normalizeError(error.message));
Scheduler.log('onRecoverableError: ' + normalizeError(error.message));
if (error.cause) {
Scheduler.log('Cause: ' + normalizeError(error.cause.message));
}
},
});
await waitForAll([]);
Expand Down Expand Up @@ -205,7 +208,10 @@ describe('ReactDOMFizzServerHydrationWarning', () => {
);
ReactDOMClient.hydrateRoot(container, <App isClient={true} />, {
onRecoverableError(error) {
Scheduler.log(normalizeError(error.message));
Scheduler.log('onRecoverableError: ' + normalizeError(error.message));
if (error.cause) {
Scheduler.log('Cause: ' + normalizeError(error.cause.message));
}
},
});
await waitForAll([]);
Expand Down Expand Up @@ -246,12 +252,14 @@ describe('ReactDOMFizzServerHydrationWarning', () => {
);
ReactDOMClient.hydrateRoot(container, <App isClient={true} />, {
onRecoverableError(error) {
Scheduler.log(normalizeError(error.message));
Scheduler.log('onRecoverableError: ' + normalizeError(error.message));
if (error.cause) {
Scheduler.log('Cause: ' + normalizeError(error.cause.message));
}
},
});
await waitForAll([
"Hydration failed because the server rendered HTML didn't match the client.",
'There was an error while hydrating.',
"onRecoverableError: Hydration failed because the server rendered HTML didn't match the client.",
]);
expect(getVisibleChildren(container)).toEqual(
<div>
Expand Down Expand Up @@ -283,7 +291,10 @@ describe('ReactDOMFizzServerHydrationWarning', () => {
);
const root = ReactDOMClient.hydrateRoot(container, <App text="Client" />, {
onRecoverableError(error) {
Scheduler.log(normalizeError(error.message));
Scheduler.log('onRecoverableError: ' + normalizeError(error.message));
if (error.cause) {
Scheduler.log('Cause: ' + normalizeError(error.cause.message));
}
},
});
await waitForAll([]);
Expand Down Expand Up @@ -327,12 +338,14 @@ describe('ReactDOMFizzServerHydrationWarning', () => {
);
ReactDOMClient.hydrateRoot(container, <App isClient={true} />, {
onRecoverableError(error) {
Scheduler.log(normalizeError(error.message));
Scheduler.log('onRecoverableError: ' + normalizeError(error.message));
if (error.cause) {
Scheduler.log('Cause: ' + normalizeError(error.cause.message));
}
},
});
await waitForAll([
"Hydration failed because the server rendered HTML didn't match the client.",
'There was an error while hydrating.',
"onRecoverableError: Hydration failed because the server rendered HTML didn't match the client.",
]);
expect(getVisibleChildren(container)).toEqual(
<div>
Expand Down Expand Up @@ -367,12 +380,14 @@ describe('ReactDOMFizzServerHydrationWarning', () => {
);
ReactDOMClient.hydrateRoot(container, <App isClient={true} />, {
onRecoverableError(error) {
Scheduler.log(normalizeError(error.message));
Scheduler.log('onRecoverableError: ' + normalizeError(error.message));
if (error.cause) {
Scheduler.log('Cause: ' + normalizeError(error.cause.message));
}
},
});
await waitForAll([
"Hydration failed because the server rendered HTML didn't match the client.",
'There was an error while hydrating.',
"onRecoverableError: Hydration failed because the server rendered HTML didn't match the client.",
]);
expect(getVisibleChildren(container)).toEqual(
<div>
Expand Down Expand Up @@ -410,12 +425,14 @@ describe('ReactDOMFizzServerHydrationWarning', () => {
);
ReactDOMClient.hydrateRoot(container, <App isClient={true} />, {
onRecoverableError(error) {
Scheduler.log(normalizeError(error.message));
Scheduler.log('onRecoverableError: ' + normalizeError(error.message));
if (error.cause) {
Scheduler.log('Cause: ' + normalizeError(error.cause.message));
}
},
});
await waitForAll([
"Hydration failed because the server rendered HTML didn't match the client.",
'There was an error while hydrating.',
"onRecoverableError: Hydration failed because the server rendered HTML didn't match the client.",
]);
expect(getVisibleChildren(container)).toEqual(
<div>
Expand Down Expand Up @@ -451,12 +468,14 @@ describe('ReactDOMFizzServerHydrationWarning', () => {
);
ReactDOMClient.hydrateRoot(container, <App isClient={true} />, {
onRecoverableError(error) {
Scheduler.log(normalizeError(error.message));
Scheduler.log('onRecoverableError: ' + normalizeError(error.message));
if (error.cause) {
Scheduler.log('Cause: ' + normalizeError(error.cause.message));
}
},
});
await waitForAll([
"Hydration failed because the server rendered HTML didn't match the client.",
'There was an error while hydrating.',
"onRecoverableError: Hydration failed because the server rendered HTML didn't match the client.",
]);
expect(getVisibleChildren(container)).toEqual(
<div>
Expand Down Expand Up @@ -496,7 +515,10 @@ describe('ReactDOMFizzServerHydrationWarning', () => {
);
ReactDOMClient.hydrateRoot(container, <App isClient={true} />, {
onRecoverableError(error) {
Scheduler.log(normalizeError(error.message));
Scheduler.log('onRecoverableError: ' + normalizeError(error.message));
if (error.cause) {
Scheduler.log('Cause: ' + normalizeError(error.cause.message));
}
},
});
await waitForAll([]);
Expand Down Expand Up @@ -533,7 +555,10 @@ describe('ReactDOMFizzServerHydrationWarning', () => {
);
ReactDOMClient.hydrateRoot(container, <App isClient={true} />, {
onRecoverableError(error) {
Scheduler.log(normalizeError(error.message));
Scheduler.log('onRecoverableError: ' + normalizeError(error.message));
if (error.cause) {
Scheduler.log('Cause: ' + normalizeError(error.cause.message));
}
},
});
await waitForAll([]);
Expand Down Expand Up @@ -566,12 +591,14 @@ describe('ReactDOMFizzServerHydrationWarning', () => {
);
ReactDOMClient.hydrateRoot(container, <App isClient={true} />, {
onRecoverableError(error) {
Scheduler.log(normalizeError(error.message));
Scheduler.log('onRecoverableError: ' + normalizeError(error.message));
if (error.cause) {
Scheduler.log('Cause: ' + normalizeError(error.cause.message));
}
},
});
await waitForAll([
"Hydration failed because the server rendered HTML didn't match the client.",
'There was an error while hydrating.',
"onRecoverableError: Hydration failed because the server rendered HTML didn't match the client.",
]);
expect(getVisibleChildren(container)).toEqual(
<div>
Expand Down Expand Up @@ -604,12 +631,14 @@ describe('ReactDOMFizzServerHydrationWarning', () => {
);
ReactDOMClient.hydrateRoot(container, <App isClient={true} />, {
onRecoverableError(error) {
Scheduler.log(normalizeError(error.message));
Scheduler.log('onRecoverableError: ' + normalizeError(error.message));
if (error.cause) {
Scheduler.log('Cause: ' + normalizeError(error.cause.message));
}
},
});
await waitForAll([
"Hydration failed because the server rendered HTML didn't match the client.",
'There was an error while hydrating.',
"onRecoverableError: Hydration failed because the server rendered HTML didn't match the client.",
]);
expect(getVisibleChildren(container)).toEqual(
<div>
Expand Down
Loading

0 comments on commit 6eacde8

Please sign in to comment.