Fix useMutableSource tearing bug (#18912)
* Failing useMutableSource test If a source is mutated after initial read but before subscription is set up, it should still entangle all pending mutations even if snapshot of new subscription happens to match. Test case illustrates how not doing this can lead to tearing. * Fix useMutableSource tearing bug Fix is to move the entanglement call outside of the block that checks if the snapshot has changed.
Andrew Clark committed
May 13, 2020 at 14:28 UTC
fdb641629e82df6588c16e158978ac57d9aff9ba
3 files changed
+106
-15
packages/react-reconciler/src/ReactFiberHooks.new.js
+4
-7
@@ -997,14 +997,11 @@ function useMutableSource<Source, Snapshot>(
997
const suspenseConfig = requestCurrentSuspenseConfig();
998
const lane = requestUpdateLane(fiber, suspenseConfig);
999
markRootMutableRead(root, lane);
1000
-
1001
- // If the source mutated between render and now,
1002
- // there may be state updates already scheduled from the old source.
1003
- // Entangle the updates so that they render in the same batch.
1004
- // TODO: I think we need to entangle even if the snapshot matches,
1005
- // because there could have been an update to a different hook.
1006
- markRootEntangled(root, root.mutableReadLanes);
1000
}
1001
+ // If the source mutated between render and now,
1002
+ // there may be state updates already scheduled from the old source.
1003
+ // Entangle the updates so that they render in the same batch.
1004
+ markRootEntangled(root, root.mutableReadLanes);
1005
}
1006
}, [getSnapshot, source, subscribe]);
1007
packages/react-reconciler/src/ReactFiberHooks.old.js
+7
-8
@@ -986,15 +986,14 @@ function useMutableSource<Source, Snapshot>(
986
suspenseConfig,
987
);
988
setPendingExpirationTime(root, expirationTime);
989
-
990
- // If the source mutated between render and now,
991
- // there may be state updates already scheduled from the old getSnapshot.
992
- // Those updates should not commit without this value.
993
- // There is no mechanism currently to associate these updates though,
994
- // so for now we fall back to synchronously flushing all pending updates.
995
- // TODO: Improve this later.
996
- markRootExpiredAtTime(root, getLastPendingExpirationTime(root));
989
}
990
+ // If the source mutated between render and now,
991
+ // there may be state updates already scheduled from the old getSnapshot.
992
+ // Those updates should not commit without this value.
993
+ // There is no mechanism currently to associate these updates though,
994
+ // so for now we fall back to synchronously flushing all pending updates.
995
+ // TODO: Improve this later.
996
+ markRootExpiredAtTime(root, getLastPendingExpirationTime(root));
997
}
998
}, [getSnapshot, source, subscribe]);
999
packages/react-reconciler/src/__tests__/useMutableSource-test.internal.js
+95
@@ -1398,6 +1398,101 @@ describe('useMutableSource', () => {
1398
expect(root.getChildrenAsJSX()).toEqual('first: a1, second: a1');
1399
});
1400
1401
+ // @gate experimental
1402
+ it(
1403
+ 'if source is mutated after initial read but before subscription is set ' +
1404
+ 'up, should still entangle all pending mutations even if snapshot of ' +
1405
+ 'new subscription happens to match',
1406
+ async () => {
1407
+ const source = createSource({
1408
+ a: 'a0',
1409
+ b: 'b0',
1410
+ });
1411
+ const mutableSource = createMutableSource(source);
1412
+
1413
+ const getSnapshotA = () => source.value.a;
1414
+ const getSnapshotB = () => source.value.b;
1415
+
1416
+ function mutateA(newA) {
1417
+ source.value = {
1418
+ ...source.value,
1419
+ a: newA,
1420
+ };
1421
+ }
1422
+
1423
+ function mutateB(newB) {
1424
+ source.value = {
1425
+ ...source.value,
1426
+ b: newB,
1427
+ };
1428
+ }
1429
+
1430
+ function Read({getSnapshot}) {
1431
+ const value = useMutableSource(
1432
+ mutableSource,
1433
+ getSnapshot,
1434
+ defaultSubscribe,
1435
+ );
1436
+ Scheduler.unstable_yieldValue(value);
1437
+ return value;
1438
+ }
1439
+
1440
+ function Text({text}) {
1441
+ Scheduler.unstable_yieldValue(text);
1442
+ return text;
1443
+ }
1444
+
1445
+ const root = ReactNoop.createRoot();
1446
+ await act(async () => {
1447
+ root.render(
1448
+ <>
1449
+ <Read getSnapshot={getSnapshotA} />
1450
+ </>,
1451
+ );
1452
+ });
1453
+ expect(Scheduler).toHaveYielded(['a0']);
1454
+ expect(root).toMatchRenderedOutput('a0');
1455
+
1456
+ await act(async () => {
1457
+ root.render(
1458
+ <>
1459
+ <Read getSnapshot={getSnapshotA} />
1460
+ <Read getSnapshot={getSnapshotB} />
1461
+ <Text text="c" />
1462
+ </>,
1463
+ );
1464
+
1465
+ expect(Scheduler).toFlushAndYieldThrough(['a0', 'b0']);
1466
+ // Mutate in an event. This schedules a subscription update on a, which
1467
+ // already mounted, but not b, which hasn't subscribed yet.
1468
+ mutateA('a1');
1469
+ mutateB('b1');
1470
+
1471
+ // Mutate again at lower priority. This will schedule another subscription
1472
+ // update on a, but not b. When b mounts and subscriptions, the value it
1473
+ // read during render will happen to match the latest value. But it should
1474
+ // still entangle the updates to prevent the previous update (a1) from
1475
+ // rendering by itself.
1476
+ Scheduler.unstable_runWithPriority(
1477
+ Scheduler.unstable_IdlePriority,
1478
+ () => {
1479
+ mutateA('a0');
1480
+ mutateB('b0');
1481
+ },
1482
+ );
1483
+ // Finish the current render
1484
+ expect(Scheduler).toFlushUntilNextPaint(['c']);
1485
+ // a0 will re-render because of the mutation update. But it should show
1486
+ // the latest value, not the intermediate one, to avoid tearing with b.
1487
+ expect(Scheduler).toFlushUntilNextPaint(['a0']);
1488
+ expect(root).toMatchRenderedOutput('a0b0c');
1489
+ // We should be done.
1490
+ expect(Scheduler).toFlushAndYield([]);
1491
+ expect(root).toMatchRenderedOutput('a0b0c');
1492
+ });
1493
+ },
1494
+ );
1495
+
1496
// @gate experimental
1497
it('getSnapshot changes and then source is mutated during interleaved event', async () => {
1498
const {useEffect} = React;