Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

useMutableSource: bugfix for new getSnapshot with mutation #18297

Merged
merged 5 commits into from
Apr 3, 2020
Merged

useMutableSource: bugfix for new getSnapshot with mutation #18297

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
9bee11d
useMutableSource: bugfix for new getSnapshot with mutation
Mar 31, 2020
4d25d4e
Split mutable source pending expiration times into first and last
Apr 2, 2020
1a73af5
Fixed remaining tearing cases
Apr 2, 2020
db511f4
Swapped first/last logic in setPendingExpirationTime and removed no l…
Apr 3, 2020
86da5ce
Recreate queue and dispatch when source/subscribe/getSnapshot change
Apr 3, 2020
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
133 changes: 47 additions & 86 deletions packages/react-reconciler/src/ReactFiberHooks.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -999,56 +999,44 @@ function useMutableSource<Source, Snapshot>(
subscribe,
}: MutableSourceMemoizedState<Source, Snapshot>);

// Sync the values needed by our subscribe function after each commit.
// Sync the values needed by our subscription handler after each commit.
dispatcher.useEffect(() => {
const didGetSnapshotChange = !is(refs.getSnapshot, getSnapshot);
refs.getSnapshot = getSnapshot;

// Normally the dispatch function for a state hook never changes,
// but in the case of this hook, it will change if getSnapshot changes.
// In that case, the subscription below will have cloesd over the previous function,
// so we use a ref to ensure that handleChange() always has the latest version.
// but this hook recreates the queue in certain cases to avoid updates from stale sources.
// handleChange() below needs to reference the dispatch function without re-subscribing,
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think if you go with this approach then you don't have to track getSnapshot or setSnapshot at all. Can use the closed over values, since getSnapshot is now a dep, and setSnapshot only changes when one of the other deps does.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Edit: Got confused, thought you had removed the getSnapshot optimization. Never mind.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No worries. This is something we can further optimize when we replace the two composed effects with our own pushed effect. Then we can just use one single effect and selectively unsubscribe/subscribe only when necessary.

Sounds like that will be a fun little cleanup refactor anyway. 😄

// so we use a ref to ensure that it always has the latest version.
refs.setSnapshot = setSnapshot;

// This effect runs on mount, even though getSnapshot hasn't changed.
// In that case we can avoid the additional checks for a changed snapshot,
// because the subscription effect below will cover this.
if (didGetSnapshotChange) {
// Because getSnapshot is shared with subscriptions via a ref,
// we don't resubscribe when getSnapshot changes.
// This means that we also don't check for any missed mutations
// between the render and the passive commit though.
// So we need to check here, just like when we newly subscribe.
const maybeNewVersion = getVersion(source._source);
if (!is(version, maybeNewVersion)) {
const maybeNewSnapshot = getSnapshot(source._source);
if (!is(snapshot, maybeNewSnapshot)) {
setSnapshot(maybeNewSnapshot);

const currentTime = requestCurrentTimeForUpdate();
const suspenseConfig = requestCurrentSuspenseConfig();
const expirationTime = computeExpirationForFiber(
currentTime,
fiber,
suspenseConfig,
);
setPendingExpirationTime(root, expirationTime);

// If the source mutated between render and now,
// there may be state updates already scheduled from the old getSnapshot.
// Those updates should not commit without this value.
// There is no mechanism currently to associate these updates though,
// so for now we fall back to synchronously flushing all pending updates.
// TODO: Improve this later.
markRootExpiredAtTime(root, getLastPendingExpirationTime(root));
}
// Check for a possible change between when we last rendered now.
const maybeNewVersion = getVersion(source._source);
if (!is(version, maybeNewVersion)) {
const maybeNewSnapshot = getSnapshot(source._source);
if (!is(snapshot, maybeNewSnapshot)) {
setSnapshot(maybeNewSnapshot);

const currentTime = requestCurrentTimeForUpdate();
const suspenseConfig = requestCurrentSuspenseConfig();
const expirationTime = computeExpirationForFiber(
currentTime,
fiber,
suspenseConfig,
);
setPendingExpirationTime(root, expirationTime);

// If the source mutated between render and now,
// there may be state updates already scheduled from the old getSnapshot.
// Those updates should not commit without this value.
// There is no mechanism currently to associate these updates though,
// so for now we fall back to synchronously flushing all pending updates.
// TODO: Improve this later.
markRootExpiredAtTime(root, getLastPendingExpirationTime(root));
}
}
}, [getSnapshot]);
}, [getSnapshot, source, subscribe]);

// If we got a new source or subscribe function,
// we'll need to subscribe in a passive effect,
// and also check for any changes that fire between render and subscribe.
// If we got a new source or subscribe function, re-subscribe in a passive effect.
dispatcher.useEffect(() => {
const handleChange = () => {
const latestGetSnapshot = refs.getSnapshot;
Expand Down Expand Up @@ -1089,29 +1077,6 @@ function useMutableSource<Source, Snapshot>(
}
}

// Check for a possible change between when we last rendered and when we just subscribed.
const maybeNewVersion = getVersion(source._source);
if (!is(version, maybeNewVersion)) {
const maybeNewSnapshot = getSnapshot(source._source);
if (!is(snapshot, maybeNewSnapshot)) {
setSnapshot(maybeNewSnapshot);

const currentTime = requestCurrentTimeForUpdate();
const suspenseConfig = requestCurrentSuspenseConfig();
const expirationTime = computeExpirationForFiber(
currentTime,
fiber,
suspenseConfig,
);
setPendingExpirationTime(root, expirationTime);

// We missed a mutation before committing.
// It's possible that other components using this source also have pending updates scheduled.
// In that case, we should ensure they all commit together.
markRootExpiredAtTime(root, getLastPendingExpirationTime(root));
}
}

return unsubscribe;
}, [source, subscribe]);

Expand All @@ -1126,31 +1091,27 @@ function useMutableSource<Source, Snapshot>(
//
// In both cases, we need to throw away pending udpates (since they are no longer relevant)
// and treat reading from the source as we do in the mount case.
const didGetSnapshotChange = !is(prevGetSnapshot, getSnapshot);
if (
!is(prevGetSnapshot, getSnapshot) ||
!is(prevSource, source) ||
!is(prevSubscribe, subscribe) ||
didGetSnapshotChange
!is(prevSubscribe, subscribe)
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is neat that this is now the same condition that the effect hook uses when comparing deps. In the future we could "cheat" and read the deps off the effect hook instead of using a ref.

) {
if (didGetSnapshotChange) {
// Create a new queue and setState method,
// So if there are interleaved updates, they get pushed to the older queue.
// When this becomes current, the previous queue and dispatch method will be discarded,
// including any interleaving updates that occur.
const newQueue = {
pending: null,
dispatch: null,
lastRenderedReducer: basicStateReducer,
lastRenderedState: snapshot,
};
newQueue.dispatch = setSnapshot = (dispatchAction.bind(
null,
currentlyRenderingFiber,
newQueue,
): any);
stateHook.queue = newQueue;
}

// Create a new queue and setState method,
// So if there are interleaved updates, they get pushed to the older queue.
// When this becomes current, the previous queue and dispatch method will be discarded,
// including any interleaving updates that occur.
const newQueue = {
pending: null,
dispatch: null,
lastRenderedReducer: basicStateReducer,
lastRenderedState: snapshot,
};
newQueue.dispatch = setSnapshot = (dispatchAction.bind(
null,
currentlyRenderingFiber,
newQueue,
): any);
stateHook.queue = newQueue;
stateHook.baseQueue = null;
snapshot = readFromUnsubcribedMutableSource(root, source, getSnapshot);
stateHook.memoizedState = stateHook.baseState = snapshot;
Expand Down
2 changes: 1 addition & 1 deletion packages/react-reconciler/src/__tests__/useMutableSource-test.internal.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -302,7 +302,7 @@ describe('useMutableSource', () => {
/>,
() => Scheduler.unstable_yieldValue('Sync effect'),
);
expect(Scheduler).toFlushAndYieldThrough(['only:a-one', 'Sync effect']);
expect(Scheduler).toFlushAndYield(['only:a-one', 'Sync effect']);
ReactNoop.flushPassiveEffects();
expect(sourceA.listenerCount).toBe(1);

Expand Down