Skip to content

Commit

Permalink
useMutableSource: bugfix for new getSnapshot with mutation
Browse files Browse the repository at this point in the history
  • Loading branch information
Brian Vaughn committed Mar 13, 2020
1 parent 885ed46 commit e07253b
Show file tree
Hide file tree
Showing 2 changed files with 69 additions and 0 deletions.
11 changes: 11 additions & 0 deletions packages/react-reconciler/src/ReactFiberHooks.js
Expand Up @@ -991,6 +991,17 @@ function useMutableSource<Source, Snapshot>(
// Sync the values needed by our subscribe function after each commit.
dispatcher.useEffect(() => {
refs.getSnapshot = getSnapshot;

// Because getSnapshot is shared with subscriptions via a ref,
// we won't check for missed mutations when getSnapshot is the only thing that's changed.
// This means we need to check for possible changes here just like when we re-subscribe.
const maybeNewVersion = getVersion(source._source);
if (!is(version, maybeNewVersion)) {
const maybeNewSnapshot = getSnapshot(source._source);
if (!is(snapshot, maybeNewSnapshot)) {
setSnapshot(maybeNewSnapshot);
}
}
}, [getSnapshot]);

// If we got a new source or subscribe function,
Expand Down
Expand Up @@ -1265,6 +1265,64 @@ describe('useMutableSource', () => {
expect(Scheduler).toHaveYielded(['x: bar, y: bar']);
});

it('getSnapshot changes and then source is mutated in between paint and passive effect phase', async () => {
const source = createSource({
a: 'foo',
b: 'bar',
});
const mutableSource = createMutableSource(source);

function mutateB(newB) {
source.value = {
...source.value,
b: newB,
};
}

const getSnapshotA = () => source.value.a;
const getSnapshotB = () => source.value.b;

function App({getSnapshot}) {
const value = useMutableSource(
mutableSource,
getSnapshot,
defaultSubscribe,
);

Scheduler.unstable_yieldValue('Render: ' + value);
React.useEffect(() => {
Scheduler.unstable_yieldValue('Commit: ' + value);
}, [value]);

return value;
}

const root = ReactNoop.createRoot();
await act(async () => {
root.render(<App getSnapshot={getSnapshotA} />);
});
expect(Scheduler).toHaveYielded(['Render: foo', 'Commit: foo']);

await act(async () => {
// Switch getSnapshot to read from B instead
root.render(<App getSnapshot={getSnapshotB} />);
// Render and finish the tree, but yield right after paint, before
// the passive effects have fired.
expect(Scheduler).toFlushUntilNextPaint(['Render: bar']);
// Then mutate B.
mutateB('baz');
});
expect(Scheduler).toHaveYielded([
// Fires the effect from the previous render
'Commit: bar',
// During that effect, it should detect that the snapshot has changed
// and re-render.
'Render: baz',
'Commit: baz',
]);
expect(root).toMatchRenderedOutput('baz');
});

if (__DEV__) {
describe('dev warnings', () => {
it('should warn if the subscribe function does not return an unsubscribe function', () => {
Expand Down

0 comments on commit e07253b

Please sign in to comment.