Skip to content
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.

Sign up for GitHub

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

Jump to bottom

Move error logging to update callback #21737

Merged
merged 2 commits into from Jun 24, 2021
Merged

Move error logging to update callback #21737

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
5dc3d50
Move error logging to update callback
gaearon Jun 23, 2021
c0f639c
Add an explanation for the rest of duplicates
gaearon Jun 23, 2021
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
26 changes: 9 additions & 17 deletions packages/react-dom/src/__tests__/ReactDOMConsoleErrorReporting-test.js
View file
Open in desktop
Expand Up @@ -166,7 +166,8 @@ describe('ReactDOMConsoleErrorReporting', () => {
}),
],
[
// TODO: This is duplicated only with createRoot. Why?
// This is only duplicated with createRoot
// because it retries once with a sync render.
expect.objectContaining({
message: 'Boom',
}),
Expand All @@ -181,7 +182,8 @@ describe('ReactDOMConsoleErrorReporting', () => {
}),
],
[
// TODO: This is duplicated only with createRoot. Why?
// This is only duplicated with createRoot
// because it retries once with a sync render.
expect.stringContaining('Error: Uncaught [Error: Boom]'),
expect.objectContaining({
message: 'Boom',
Expand Down Expand Up @@ -246,7 +248,8 @@ describe('ReactDOMConsoleErrorReporting', () => {
}),
],
[
// TODO: This is duplicated only with createRoot. Why?
// This is only duplicated with createRoot
// because it retries once with a sync render.
expect.objectContaining({
message: 'Boom',
}),
Expand All @@ -261,20 +264,15 @@ describe('ReactDOMConsoleErrorReporting', () => {
}),
],
[
// Addendum by React:
expect.stringContaining(
'The above error occurred in the <Foo> component',
),
],
[
// TODO: This is duplicated only with createRoot. Why?
// This is only duplicated with createRoot
// because it retries once with a sync render.
expect.stringContaining('Error: Uncaught [Error: Boom]'),
expect.objectContaining({
message: 'Boom',
}),
],
[
// TODO: This is duplicated only with createRoot. Why?
// Addendum by React:
expect.stringContaining(
'The above error occurred in the <Foo> component',
),
Expand All @@ -291,12 +289,6 @@ describe('ReactDOMConsoleErrorReporting', () => {
message: 'Boom',
}),
],
[
// TODO: This is duplicated only with createRoot. Why?
expect.objectContaining({
message: 'Boom',
}),
],
]);
}

Expand Down
15 changes: 7 additions & 8 deletions packages/react-reconciler/src/ReactFiberThrow.new.js
View file
Open in desktop
Expand Up @@ -108,9 +108,14 @@ function createClassErrorUpdate(
if (typeof getDerivedStateFromError === 'function') {
const error = errorInfo.value;
update.payload = () => {
logCapturedError(fiber, errorInfo);
return getDerivedStateFromError(error);
};
update.callback = () => {
if (__DEV__) {
markFailedErrorBoundaryForHotReloading(fiber);
}
logCapturedError(fiber, errorInfo);
};
}

const inst = fiber.stateNode;
Expand All @@ -119,16 +124,14 @@ function createClassErrorUpdate(
if (__DEV__) {
markFailedErrorBoundaryForHotReloading(fiber);
}
logCapturedError(fiber, errorInfo);
if (typeof getDerivedStateFromError !== 'function') {
// To preserve the preexisting retry behavior of error boundaries,
// we keep track of which ones already failed during this batch.
// This gets reset before we yield back to the browser.
// TODO: Warn in strict mode if getDerivedStateFromError is
// not defined.
markLegacyErrorBoundaryAsFailed(this);

// Only log here if componentDidCatch is the only error boundary method defined
logCapturedError(fiber, errorInfo);
}
const error = errorInfo.value;
const stack = errorInfo.stack;
Expand All @@ -150,10 +153,6 @@ function createClassErrorUpdate(
}
}
};
} else if (__DEV__) {
update.callback = () => {
markFailedErrorBoundaryForHotReloading(fiber);
};
}
return update;
}
Expand Down
15 changes: 7 additions & 8 deletions packages/react-reconciler/src/ReactFiberThrow.old.js
View file
Open in desktop
Expand Up @@ -108,9 +108,14 @@ function createClassErrorUpdate(
if (typeof getDerivedStateFromError === 'function') {
const error = errorInfo.value;
update.payload = () => {
logCapturedError(fiber, errorInfo);
return getDerivedStateFromError(error);
};
update.callback = () => {
if (__DEV__) {
markFailedErrorBoundaryForHotReloading(fiber);
}
logCapturedError(fiber, errorInfo);
};
}

const inst = fiber.stateNode;
Expand All @@ -119,16 +124,14 @@ function createClassErrorUpdate(
if (__DEV__) {
markFailedErrorBoundaryForHotReloading(fiber);
}
logCapturedError(fiber, errorInfo);
if (typeof getDerivedStateFromError !== 'function') {
// To preserve the preexisting retry behavior of error boundaries,
// we keep track of which ones already failed during this batch.
// This gets reset before we yield back to the browser.
// TODO: Warn in strict mode if getDerivedStateFromError is
// not defined.
markLegacyErrorBoundaryAsFailed(this);

// Only log here if componentDidCatch is the only error boundary method defined
logCapturedError(fiber, errorInfo);
}
const error = errorInfo.value;
const stack = errorInfo.stack;
Expand All @@ -150,10 +153,6 @@ function createClassErrorUpdate(
}
}
};
} else if (__DEV__) {
update.callback = () => {
markFailedErrorBoundaryForHotReloading(fiber);
};
}
return update;
}
Expand Down