Skip to content

Commit

Permalink
[Bugfix] Check tag before calling hook effects (#16215)
Browse files Browse the repository at this point in the history
* Add failing test for #16215

Next commit fixes it.

* [Bugfix] Check tag before calling hook effects

TODO: Test that triggers this
  • Loading branch information
acdlite authored Jul 26, 2019
1 parent 858c842 commit ed57bf8
Show file tree
Hide file tree
Showing 2 changed files with 82 additions and 2 deletions.
17 changes: 15 additions & 2 deletions packages/react-reconciler/src/ReactFiberCommitWork.js
Original file line number Diff line number Diff line change
Expand Up @@ -55,10 +55,12 @@ import {
clearCaughtError,
} from 'shared/ReactErrorUtils';
import {
NoEffect,
ContentReset,
Placement,
Snapshot,
Update,
Passive,
} from 'shared/ReactSideEffectTags';
import getComponentName from 'shared/getComponentName';
import invariant from 'shared/invariant';
Expand Down Expand Up @@ -383,8 +385,19 @@ function commitHookEffectList(
}

export function commitPassiveHookEffects(finishedWork: Fiber): void {
commitHookEffectList(UnmountPassive, NoHookEffect, finishedWork);
commitHookEffectList(NoHookEffect, MountPassive, finishedWork);
if ((finishedWork.effectTag & Passive) !== NoEffect) {
switch (finishedWork.tag) {
case FunctionComponent:
case ForwardRef:
case SimpleMemoComponent: {
commitHookEffectList(UnmountPassive, NoHookEffect, finishedWork);
commitHookEffectList(NoHookEffect, MountPassive, finishedWork);
break;
}
default:
break;
}
}
}

function commitLifeCycles(
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -238,4 +238,71 @@ describe('ReactSuspense', () => {
expect(ops1).toEqual([]);
expect(ops2).toEqual([]);
});

if (__DEV__) {
it('regression test for #16215 that relies on implementation details', async () => {
// Regression test for https://github.com/facebook/react/pull/16215.
// The bug only happens if there's an error earlier in the commit phase.
// The first error is the one that gets thrown, so to observe the later
// error, I've mocked the ReactErrorUtils module.
//
// If this test starts failing because the implementation details change,
// you can probably just delete it. It's not worth the hassle.
jest.resetModules();

let errors = [];
let hasCaughtError = false;
jest.mock('shared/ReactErrorUtils', () => ({
invokeGuardedCallback(name, fn, context, ...args) {
try {
return fn.call(context, ...args);
} catch (error) {
hasCaughtError = true;
errors.push(error);
}
},
hasCaughtError() {
return hasCaughtError;
},
clearCaughtError() {
hasCaughtError = false;
return errors[errors.length - 1];
},
}));

ReactFeatureFlags = require('shared/ReactFeatureFlags');
ReactFeatureFlags.enableSuspenseCallback = true;

React = require('react');
ReactNoop = require('react-noop-renderer');
Scheduler = require('scheduler');

const {useEffect} = React;
const {PromiseComp} = createThenable();
function App() {
useEffect(() => {
Scheduler.unstable_yieldValue('Passive Effect');
});
return (
<React.Suspense
suspenseCallback={() => {
throw Error('Oops!');
}}
fallback="Loading...">
<PromiseComp />
</React.Suspense>
);
}
const root = ReactNoop.createRoot();
await ReactNoop.act(async () => {
root.render(<App />);
expect(Scheduler).toFlushAndThrow('Oops!');
});

// Should have only received a single error. Before the bug fix, there was
// also a second error related to the Suspense update queue.
expect(errors.length).toBe(1);
expect(errors[0].message).toEqual('Oops!');
});
}
});

0 comments on commit ed57bf8

Please sign in to comment.