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.

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

Recover from errors with a boundary in completion phase #14104

Merged
merged 7 commits into from
Nov 6, 2018
Merged
Show file tree
Hide file tree
Changes from 3 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
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',
);
});
});
19 changes: 16 additions & 3 deletions packages/react-reconciler/src/ReactFiberScheduler.js
Original file line number Diff line number Diff line change
Expand Up @@ -263,6 +263,7 @@ let nextRenderDidError: boolean = false;
let nextEffect: Fiber | null = null;

let isCommitting: boolean = false;
let isCompleting: boolean = false;
let rootWithPendingPassiveEffects: FiberRoot | null = null;
let passiveEffectCallbackHandle: * = null;
let passiveEffectCallback: * = null;
Expand Down Expand Up @@ -953,22 +954,30 @@ function completeUnitOfWork(workInProgress: Fiber): Fiber | null {
startProfilerTimer(workInProgress);
}

// Remember we're completing this unit so we can find a boundary if it fails.
isCompleting = true;
nextUnitOfWork = workInProgress;
nextUnitOfWork = completeWork(
current,
workInProgress,
nextRenderExpirationTime,
);
isCompleting = false;

if (workInProgress.mode & ProfileMode) {
// Update render duration assuming we didn't error.
stopProfilerTimerIfRunningAndRecordDelta(workInProgress, false);
}
} else {
// Remember we're completing this unit so we can find a boundary if it fails.
isCompleting = true;
nextUnitOfWork = workInProgress;
nextUnitOfWork = completeWork(
current,
workInProgress,
nextRenderExpirationTime,
);
isCompleting = false;
}
stopWorkTimer(workInProgress);
resetChildExpirationTime(workInProgress, nextRenderExpirationTime);
Expand Down Expand Up @@ -1277,6 +1286,8 @@ function renderRoot(root: FiberRoot, isYieldy: boolean): void {
resetContextDependences();
resetHooks();

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

const failedUnitOfWork: Fiber = nextUnitOfWork;
if (__DEV__ && replayFailedUnitOfWorkWithInvokeGuardedCallback) {
replayUnitOfWork(failedUnitOfWork, thrownValue, isYieldy);
if (!wasCompleting) {
const failedUnitOfWork: Fiber = nextUnitOfWork;
if (__DEV__ && replayFailedUnitOfWorkWithInvokeGuardedCallback) {
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

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