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

Recover from errors with a boundary in completion phase #14104

Merged
merged 7 commits into from
Nov 6, 2018
Merged

Recover from errors with a boundary in completion phase #14104

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
7 commits
Select commit Hold shift + click to select a range
0aaf5e8
Recover from errors with a boundary in completion phase
gaearon Nov 5, 2018
9b82c9f
Use a separate field for completing unit of work
gaearon Nov 5, 2018
f644cb2
Use a simpler fix with one boolean
gaearon Nov 5, 2018
0fb8440
Reoder conditions
gaearon Nov 6, 2018
ac7c3ab
Clarify which paths are DEV-only
gaearon Nov 6, 2018
3b93b2e
Move duplicated line out
gaearon Nov 6, 2018
22158a5
Make it clearer this code is DEV-only
gaearon Nov 6, 2018
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
15 changes: 15 additions & 0 deletions packages/react-dom/src/__tests__/ReactErrorBoundaries-test.internal.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2149,4 +2149,19 @@ describe('ReactErrorBoundaries', () => {
expect(componentDidCatchError).toBe(thrownError);
expect(getDerivedStateFromErrorError).toBe(thrownError);
});

it('should catch errors from invariants in completion phase', () => {
const container = document.createElement('div');
ReactDOM.render(
<ErrorBoundary>
<input>
<div />
</input>
</ErrorBoundary>,
container,
);
expect(container.textContent).toContain(
'Caught an error: input is a void element tag',
);
});
});
28 changes: 24 additions & 4 deletions packages/react-reconciler/src/ReactFiberScheduler.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -274,11 +274,13 @@ let interruptedBy: Fiber | null = null;

let stashedWorkInProgressProperties;
let replayUnitOfWork;
let mayReplayFailedUnitOfWork;
let isReplayingFailedUnitOfWork;
let originalReplayError;
let rethrowOriginalError;
if (__DEV__ && replayFailedUnitOfWorkWithInvokeGuardedCallback) {
stashedWorkInProgressProperties = null;
mayReplayFailedUnitOfWork = true;
isReplayingFailedUnitOfWork = false;
originalReplayError = null;
replayUnitOfWork = (
Expand Down Expand Up @@ -947,18 +949,22 @@ function completeUnitOfWork(workInProgress: Fiber): Fiber | null {
const siblingFiber = workInProgress.sibling;

if ((workInProgress.effectTag & Incomplete) === NoEffect) {
if (__DEV__ && replayFailedUnitOfWorkWithInvokeGuardedCallback) {
// Don't replay if it fails during completion phase.
mayReplayFailedUnitOfWork = false;
}
// This fiber completed.
// Remember we're completing this unit so we can find a boundary if it fails.
nextUnitOfWork = workInProgress;
if (enableProfilerTimer) {
if (workInProgress.mode & ProfileMode) {
startProfilerTimer(workInProgress);
}

nextUnitOfWork = completeWork(
current,
workInProgress,
nextRenderExpirationTime,
);

if (workInProgress.mode & ProfileMode) {
// Update render duration assuming we didn't error.
stopProfilerTimerIfRunningAndRecordDelta(workInProgress, false);
Expand All @@ -970,6 +976,10 @@ function completeUnitOfWork(workInProgress: Fiber): Fiber | null {
nextRenderExpirationTime,
);
}
if (__DEV__ && replayFailedUnitOfWorkWithInvokeGuardedCallback) {
// We're out of completion phase so replaying is fine now.
mayReplayFailedUnitOfWork = true;
}
stopWorkTimer(workInProgress);
resetChildExpirationTime(workInProgress, nextRenderExpirationTime);
if (__DEV__) {
Expand Down Expand Up @@ -1277,6 +1287,14 @@ function renderRoot(root: FiberRoot, isYieldy: boolean): void {
resetContextDependences();
resetHooks();

// Reset in case completion throws.
// This is only used in DEV and when replaying is on.
let mayReplay;
if (__DEV__ && replayFailedUnitOfWorkWithInvokeGuardedCallback) {
mayReplay = mayReplayFailedUnitOfWork;
mayReplayFailedUnitOfWork = true;
}

if (nextUnitOfWork === null) {
// This is a fatal error.
didFatal = true;
Expand All @@ -1288,9 +1306,11 @@ function renderRoot(root: FiberRoot, isYieldy: boolean): void {
(resetCurrentlyProcessingQueue: any)();
}

const failedUnitOfWork: Fiber = nextUnitOfWork;
if (__DEV__ && replayFailedUnitOfWorkWithInvokeGuardedCallback) {
replayUnitOfWork(failedUnitOfWork, thrownValue, isYieldy);
if (mayReplay) {
const failedUnitOfWork: Fiber = nextUnitOfWork;
replayUnitOfWork(failedUnitOfWork, thrownValue, isYieldy);
}
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Does this mean that the bug only happens when in DEV?

Also, couldn't this code be combined into the following block to avoid implying that the conditions/variables have any effect in production?

        if (__DEV__) {
          // Reset global debug state
          // We assume this is defined in DEV
          (resetCurrentlyProcessingQueue: any)();

          if (!wasCompleting && replayFailedUnitOfWorkWithInvokeGuardedCallback) {
            const failedUnitOfWork: Fiber = nextUnitOfWork;
            replayUnitOfWork(failedUnitOfWork, thrownValue, isYieldy);
          }
        }

Copy link
Collaborator Author

@gaearon gaearon Nov 6, 2018 •
edited

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks. I pushed another commit that clarifies which parts of the fix are DEV-only.

}

// TODO: we already know this isn't true in some cases.
Expand Down