[Bugfix] Check tag before calling hook effects (#16215)
* Add failing test for #16215 Next commit fixes it. * [Bugfix] Check tag before calling hook effects TODO: Test that triggers this
Andrew Clark committed
Jul 26, 2019 at 14:28 UTC
ed57bf8ed4591fc64b44326132e97b8888e4df5a
2 files changed
+82
-2
packages/react-reconciler/src/ReactFiberCommitWork.js
+15
-2
@@ -55,10 +55,12 @@ import {
55
clearCaughtError,
56
} from 'shared/ReactErrorUtils';
57
import {
58
+ NoEffect,
59
ContentReset,
60
Placement,
61
Snapshot,
62
Update,
63
+ Passive,
64
} from 'shared/ReactSideEffectTags';
65
import getComponentName from 'shared/getComponentName';
66
import invariant from 'shared/invariant';
@@ -383,8 +385,19 @@ function commitHookEffectList(
385
}
386
387
export function commitPassiveHookEffects(finishedWork: Fiber): void {
386
- commitHookEffectList(UnmountPassive, NoHookEffect, finishedWork);
387
- commitHookEffectList(NoHookEffect, MountPassive, finishedWork);
388
+ if ((finishedWork.effectTag & Passive) !== NoEffect) {
389
+ switch (finishedWork.tag) {
390
+ case FunctionComponent:
391
+ case ForwardRef:
392
+ case SimpleMemoComponent: {
393
+ commitHookEffectList(UnmountPassive, NoHookEffect, finishedWork);
394
+ commitHookEffectList(NoHookEffect, MountPassive, finishedWork);
395
+ break;
396
+ }
397
+ default:
398
+ break;
399
+ }
400
+ }
401
}
402
403
function commitLifeCycles(
packages/react-reconciler/src/__tests__/ReactSuspenseCallback-test.internal.js
+67
@@ -238,4 +238,71 @@ describe('ReactSuspense', () => {
238
expect(ops1).toEqual([]);
239
expect(ops2).toEqual([]);
240
});
241
+
242
+ if (__DEV__) {
243
+ it('regression test for #16215 that relies on implementation details', async () => {
244
+ // Regression test for https://github.com/facebook/react/pull/16215.
245
+ // The bug only happens if there's an error earlier in the commit phase.
246
+ // The first error is the one that gets thrown, so to observe the later
247
+ // error, I've mocked the ReactErrorUtils module.
248
+ //
249
+ // If this test starts failing because the implementation details change,
250
+ // you can probably just delete it. It's not worth the hassle.
251
+ jest.resetModules();
252
+
253
+ let errors = [];
254
+ let hasCaughtError = false;
255
+ jest.mock('shared/ReactErrorUtils', () => ({
256
+ invokeGuardedCallback(name, fn, context, ...args) {
257
+ try {
258
+ return fn.call(context, ...args);
259
+ } catch (error) {
260
+ hasCaughtError = true;
261
+ errors.push(error);
262
+ }
263
+ },
264
+ hasCaughtError() {
265
+ return hasCaughtError;
266
+ },
267
+ clearCaughtError() {
268
+ hasCaughtError = false;
269
+ return errors[errors.length - 1];
270
+ },
271
+ }));
272
+
273
+ ReactFeatureFlags = require('shared/ReactFeatureFlags');
274
+ ReactFeatureFlags.enableSuspenseCallback = true;
275
+
276
+ React = require('react');
277
+ ReactNoop = require('react-noop-renderer');
278
+ Scheduler = require('scheduler');
279
+
280
+ const {useEffect} = React;
281
+ const {PromiseComp} = createThenable();
282
+ function App() {
283
+ useEffect(() => {
284
+ Scheduler.unstable_yieldValue('Passive Effect');
285
+ });
286
+ return (
287
+ <React.Suspense
288
+ suspenseCallback={() => {
289
+ throw Error('Oops!');
290
+ }}
291
+ fallback="Loading...">
292
+ <PromiseComp />
293
+ </React.Suspense>
294
+ );
295
+ }
296
+ const root = ReactNoop.createRoot();
297
+ await ReactNoop.act(async () => {
298
+ root.render(<App />);
299
+ expect(Scheduler).toFlushAndThrow('Oops!');
300
+ });
301
+
302
+ // Should have only received a single error. Before the bug fix, there was
303
+ // also a second error related to the Suspense update queue.
304
+ expect(errors.length).toBe(1);
305
+ expect(errors[0].message).toEqual('Oops!');
306
+ });
307
+ }
308
});