use-subscription tearing fix (#16623)
* Add (failing) subscription tearing test and bugfix * Added more inline comments to test * Simplified tearing test case slightly
Brian Vaughn committed
Sep 5, 2019 at 11:12 UTC
d96f478f8a79da3125f6842c16efbc2ae8bcd3bf
2 files changed
+70
-1
packages/use-subscription/src/__tests__/useSubscription-test.internal.js
+64
@@ -22,6 +22,9 @@ describe('useSubscription', () => {
22
jest.resetModules();
23
jest.mock('scheduler', () => require('scheduler/unstable_mock'));
24
25
+ const ReactFeatureFlags = require('shared/ReactFeatureFlags');
26
+ ReactFeatureFlags.debugRenderPhaseSideEffectsForStrictMode = false;
27
+
28
useSubscription = require('use-subscription').useSubscription;
29
React = require('react');
30
ReactTestRenderer = require('react-test-renderer');
@@ -560,4 +563,65 @@ describe('useSubscription', () => {
563
act(() => renderer.update(<Subscription subscription={subscription2} />));
564
Scheduler.unstable_flushAll();
565
});
566
+
567
+ it('should not tear if a mutation occurs during a concurrent update', () => {
568
+ const input = document.createElement('input');
569
+
570
+ const mutate = value => {
571
+ input.value = value;
572
+ input.dispatchEvent(new Event('change'));
573
+ };
574
+
575
+ const subscription = {
576
+ getCurrentValue: () => input.value,
577
+ subscribe: callback => {
578
+ input.addEventListener('change', callback);
579
+ return () => input.removeEventListener('change', callback);
580
+ },
581
+ };
582
+
583
+ const Subscriber = ({id}) => {
584
+ const value = useSubscription(subscription);
585
+ Scheduler.unstable_yieldValue(`render:${id}:${value}`);
586
+ return value;
587
+ };
588
+
589
+ act(() => {
590
+ // Initial render of "A"
591
+ mutate('A');
592
+ ReactTestRenderer.create(
593
+ <React.Fragment>
594
+ <Subscriber id="first" />
595
+ <Subscriber id="second" />
596
+ </React.Fragment>,
597
+ {unstable_isConcurrent: true},
598
+ );
599
+ expect(Scheduler).toFlushAndYield(['render:first:A', 'render:second:A']);
600
+
601
+ // Update state "A" -> "B"
602
+ // This update will be eagerly evaluated,
603
+ // so the tearing case this test is guarding against would not happen.
604
+ mutate('B');
605
+ expect(Scheduler).toFlushAndYield(['render:first:B', 'render:second:B']);
606
+
607
+ // No more pending updates
608
+ jest.runAllTimers();
609
+
610
+ // Partial update "B" -> "C"
611
+ // Interrupt with a second mutation "C" -> "D".
612
+ // This update will not be eagerly evaluated,
613
+ // but useSubscription() should eagerly close over the updated value to avoid tearing.
614
+ mutate('C');
615
+ expect(Scheduler).toFlushAndYieldThrough(['render:first:C']);
616
+ mutate('D');
617
+ expect(Scheduler).toFlushAndYield([
618
+ 'render:second:C',
619
+ 'render:first:D',
620
+ 'render:second:D',
621
+ ]);
622
+
623
+ // No more pending updates
624
+ jest.runAllTimers();
625
+ });
626
+ });
627
});
packages/use-subscription/src/useSubscription.js
+6
-1
@@ -80,6 +80,12 @@ export function useSubscription<Value>({
80
return;
81
}
82
83
+ // We use a state updater function to avoid scheduling work for a stale source.
84
+ // However it's important to eagerly read the currently value,
85
+ // so that all scheduled work shares the same value (in the event of multiple subscriptions).
86
+ // This avoids visual "tearing" when a mutation happens during a (concurrent) render.
87
+ const value = getCurrentValue();
88
+
89
setState(prevState => {
90
// Ignore values from stale sources!
91
// Since we subscribe an unsubscribe in a passive effect,
@@ -95,7 +101,6 @@ export function useSubscription<Value>({
101
// Some subscriptions will auto-invoke the handler, even if the value hasn't changed.
102
// If the value hasn't changed, no update is needed.
103
// Return state as-is so React can bail out and avoid an unnecessary render.
98
- const value = getCurrentValue();
104
if (prevState.value === value) {
105
return prevState;
106
}