Skip to content

Commit

Permalink
use-subscription tearing fix (#16623)
Browse files Browse the repository at this point in the history
* Add (failing) subscription tearing test and bugfix
* Added more inline comments to test
* Simplified tearing test case slightly
  • Loading branch information
Brian Vaughn authored Sep 5, 2019
1 parent 79e46b6 commit d96f478
Show file tree
Hide file tree
Showing 2 changed files with 70 additions and 1 deletion.
Original file line number Diff line number Diff line change
Expand Up @@ -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');
Expand Down Expand Up @@ -560,4 +563,65 @@ describe('useSubscription', () => {
act(() => renderer.update(<Subscription subscription={subscription2} />));
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(
<React.Fragment>
<Subscriber id="first" />
<Subscriber id="second" />
</React.Fragment>,
{unstable_isConcurrent: true},
);
expect(Scheduler).toFlushAndYield(['render:first:A', 'render:second:A']);

// 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).toFlushAndYield(['render:first:B', 'render:second:B']);

// No more pending updates
jest.runAllTimers();

// 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).toFlushAndYield([
'render:second:C',
'render:first:D',
'render:second:D',
]);

// No more pending updates
jest.runAllTimers();
});
});
});
7 changes: 6 additions & 1 deletion packages/use-subscription/src/useSubscription.js
Original file line number Diff line number Diff line change
Expand Up @@ -80,6 +80,12 @@ export function useSubscription<Value>({
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,
Expand All @@ -95,7 +101,6 @@ export function useSubscription<Value>({
// 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;
}
Expand Down

0 comments on commit d96f478

Please sign in to comment.