From 58e6cbd601bede86279da414a3334823e63c8a9f Mon Sep 17 00:00:00 2001 From: Brian Vaughn Date: Fri, 30 Aug 2019 10:46:20 -0700 Subject: [PATCH 1/3] Add (failing) subscription tearing test and bugfix --- .../useSubscription-test.internal.js | 67 +++++++++++++++++++ .../use-subscription/src/useSubscription.js | 7 +- 2 files changed, 73 insertions(+), 1 deletion(-) diff --git a/packages/use-subscription/src/__tests__/useSubscription-test.internal.js b/packages/use-subscription/src/__tests__/useSubscription-test.internal.js index daacfb5cfe575..5cd372d9d673b 100644 --- a/packages/use-subscription/src/__tests__/useSubscription-test.internal.js +++ b/packages/use-subscription/src/__tests__/useSubscription-test.internal.js @@ -22,6 +22,9 @@ describe('useSubscription', () => { jest.resetModules(); jest.mock('scheduler', () => require('scheduler/unstable_mock')); + const ReactFeatureFlags = require('shared/ReactFeatureFlags'); + ReactFeatureFlags.debugRenderPhaseSideEffectsForStrictMode = false; + useSubscription = require('use-subscription').useSubscription; React = require('react'); ReactTestRenderer = require('react-test-renderer'); @@ -560,4 +563,68 @@ describe('useSubscription', () => { act(() => renderer.update()); Scheduler.unstable_flushAll(); }); + + it('should not tear if a mutation occurs during a concurrent update', () => { + const input = document.createElement('input'); + + const mutate = value => { + input.value = value; + input.dispatchEvent(new Event('change')); + }; + + const subscription = { + getCurrentValue: () => input.value, + subscribe: callback => { + input.addEventListener('change', callback); + return () => input.removeEventListener('change', callback); + }, + }; + + const Subscriber = ({id}) => { + const value = useSubscription(subscription); + Scheduler.unstable_yieldValue(`render:${id}:${value}`); + return value; + }; + + act(() => { + // Initial render of "A" + mutate('A'); + ReactTestRenderer.create( + + + + , + {unstable_isConcurrent: true}, + ); + expect(Scheduler).toFlushAndYield(['render:first:A', 'render:second:A']); + + // Partial update "A" -> "B" + // Interrupt with a second mutation "B" -> "C" but this should not cause tearing. + mutate('B'); + expect(Scheduler).toFlushAndYieldThrough(['render:first:B']); + mutate('C'); + expect(Scheduler).toFlushAndYield([ + 'render:second:B', + 'render:first:C', + 'render:second:C', + ]); + + // No more pending updates + jest.runAllTimers(); + + // Partial update "C" -> "D" + // Interrupt with a second mutation "D" -> "E" but this should not cause tearing. + mutate('D'); + expect(Scheduler).toFlushAndYieldThrough(['render:first:D']); + mutate('E'); + expect(Scheduler).toFlushAndYield([ + 'render:second:D', + 'render:first:E', + 'render:second:E', + ]); + + // No more pending updates + jest.runAllTimers(); + }); + }); }); diff --git a/packages/use-subscription/src/useSubscription.js b/packages/use-subscription/src/useSubscription.js index 1bb80e2d7b131..5d7b6500df995 100644 --- a/packages/use-subscription/src/useSubscription.js +++ b/packages/use-subscription/src/useSubscription.js @@ -80,6 +80,12 @@ export function useSubscription({ return; } + // We use a state updater function to avoid scheduling work for a stale source. + // However it's important to eagerly read the currently value, + // so that all scheduled work shares the same value (in the event of multiple subscriptions). + // This avoids visual "tearing" when a mutation happens during a (concurrent) render. + const value = getCurrentValue(); + setState(prevState => { // Ignore values from stale sources! // Since we subscribe an unsubscribe in a passive effect, @@ -95,7 +101,6 @@ export function useSubscription({ // Some subscriptions will auto-invoke the handler, even if the value hasn't changed. // If the value hasn't changed, no update is needed. // Return state as-is so React can bail out and avoid an unnecessary render. - const value = getCurrentValue(); if (prevState.value === value) { return prevState; } From 9815c0c9aa5c2c8bace582824e15a98c78eec23a Mon Sep 17 00:00:00 2001 From: Brian Vaughn Date: Fri, 30 Aug 2019 14:56:35 -0700 Subject: [PATCH 2/3] Added more inline comments to test --- .../src/__tests__/useSubscription-test.internal.js | 9 ++++++--- 1 file changed, 6 insertions(+), 3 deletions(-) diff --git a/packages/use-subscription/src/__tests__/useSubscription-test.internal.js b/packages/use-subscription/src/__tests__/useSubscription-test.internal.js index 5cd372d9d673b..0555fc55472e1 100644 --- a/packages/use-subscription/src/__tests__/useSubscription-test.internal.js +++ b/packages/use-subscription/src/__tests__/useSubscription-test.internal.js @@ -598,8 +598,9 @@ describe('useSubscription', () => { ); expect(Scheduler).toFlushAndYield(['render:first:A', 'render:second:A']); - // Partial update "A" -> "B" - // Interrupt with a second mutation "B" -> "C" but this should not cause tearing. + // Update state "A" -> "B" + // This update will be eagerly evaluated, + // so the tearing case this test is guarding against would not happen. mutate('B'); expect(Scheduler).toFlushAndYieldThrough(['render:first:B']); mutate('C'); @@ -613,7 +614,9 @@ describe('useSubscription', () => { jest.runAllTimers(); // Partial update "C" -> "D" - // Interrupt with a second mutation "D" -> "E" but this should not cause tearing. + // Interrupt with a second mutation "D" -> "E". + // This update will not be eagerly evaluated, + // but useSubscription() should eagerly close over the updated value to avoid tearing. mutate('D'); expect(Scheduler).toFlushAndYieldThrough(['render:first:D']); mutate('E'); From 5ba5309ca4dfafa687e78722580d27b33a0119e7 Mon Sep 17 00:00:00 2001 From: Brian Vaughn Date: Fri, 30 Aug 2019 14:59:22 -0700 Subject: [PATCH 3/3] Simplified tearing test case slightly --- .../useSubscription-test.internal.js | 20 +++++++------------ 1 file changed, 7 insertions(+), 13 deletions(-) diff --git a/packages/use-subscription/src/__tests__/useSubscription-test.internal.js b/packages/use-subscription/src/__tests__/useSubscription-test.internal.js index 0555fc55472e1..891a80ceb31d8 100644 --- a/packages/use-subscription/src/__tests__/useSubscription-test.internal.js +++ b/packages/use-subscription/src/__tests__/useSubscription-test.internal.js @@ -602,28 +602,22 @@ describe('useSubscription', () => { // This update will be eagerly evaluated, // so the tearing case this test is guarding against would not happen. mutate('B'); - expect(Scheduler).toFlushAndYieldThrough(['render:first:B']); - mutate('C'); - expect(Scheduler).toFlushAndYield([ - 'render:second:B', - 'render:first:C', - 'render:second:C', - ]); + expect(Scheduler).toFlushAndYield(['render:first:B', 'render:second:B']); // No more pending updates jest.runAllTimers(); - // Partial update "C" -> "D" - // Interrupt with a second mutation "D" -> "E". + // Partial update "B" -> "C" + // Interrupt with a second mutation "C" -> "D". // This update will not be eagerly evaluated, // but useSubscription() should eagerly close over the updated value to avoid tearing. + mutate('C'); + expect(Scheduler).toFlushAndYieldThrough(['render:first:C']); mutate('D'); - expect(Scheduler).toFlushAndYieldThrough(['render:first:D']); - mutate('E'); expect(Scheduler).toFlushAndYield([ + 'render:second:C', + 'render:first:D', 'render:second:D', - 'render:first:E', - 'render:second:E', ]); // No more pending updates