From 508dca3b96ce11f8c06b3f989d92e804fd6ae312 Mon Sep 17 00:00:00 2001 From: Andrey Lunyov Date: Thu, 5 Oct 2023 05:07:30 -0700 Subject: [PATCH] Add handleMissedUpdates to subscription callback Summary: I've spent quite some time trying to come up with a unit test for the issue we observed in one of our apps. The problem was with the `setState` function in the RelayStore subscription callback. In specific conditions, such as a specific server response for the refetch query, batched updates in React, and GC runs from Relay, and something else that I couldn't fully figure out, the `useFragment` may return stale data. Unfortunately, I don't have a unit test yet. So, I propose adding a logger function to detect how often we encounter these cases, and maybe there is a specific pattern we can identify that would help us find the root cause of the issue. To detect these cases, we can use the `handleMissedUpdates` function, which will return a new state with the updated snapshot. We would log how often we have missed updates (this also needs to be integrated with the internal logger). I believe that this change will fix the issue, as we would be returning fresh data from the subscription hook. Reviewed By: tyao1 Differential Revision: D49925999 fbshipit-source-id: 07944bb7209ed9f5550261b0549a7287d4e68909 --- .../__tests__/useFragmentNode-test.js | 127 ++++++++++++++++++ .../useFragmentInternal_REACT_CACHE.js | 24 +++- .../relay-runtime/store/RelayStoreTypes.js | 8 +- 3 files changed, 156 insertions(+), 3 deletions(-) diff --git a/packages/react-relay/relay-hooks/__tests__/useFragmentNode-test.js b/packages/react-relay/relay-hooks/__tests__/useFragmentNode-test.js index 86e20e2410a51..9e6ac83cb3407 100644 --- a/packages/react-relay/relay-hooks/__tests__/useFragmentNode-test.js +++ b/packages/react-relay/relay-hooks/__tests__/useFragmentNode-test.js @@ -975,6 +975,133 @@ describe.each([ }); }); + it('should ignore updates to initially rendered data when fragment pointers change, but still handle updates to the new data', () => { + const Scheduler = require('scheduler'); + const YieldChild = (props: any) => { + // NOTE the unstable_yield method will move to the static renderer. + // When React sync runs we need to update this. + Scheduler.log(props.children); + return props.children; + }; + const YieldyUserComponent = ({user}: any) => ( + <> + Hey user, + {user.name} + with id {user.id}! + + ); + + // Assert initial render + // $FlowFixMe[incompatible-type] + SingularRenderer = YieldyUserComponent; + internalAct(() => { + renderSingularFragment({isConcurrent: true}); + }); + expectSchedulerToHaveYielded([ + 'Hey user,', + 'Alice', + ['with id ', '1', '!'], + ]); + assertFragmentResults([ + { + data: { + id: '1', + name: 'Alice', + profile_picture: null, + ...createFragmentRef('1', singularQuery), + }, + }, + ]); + + const newVariables = {...singularVariables, id: '200'}; + const newQuery = createOperationDescriptor( + gqlSingularQuery, + newVariables, + ); + internalAct(() => { + environment.commitPayload(newQuery, { + node: { + __typename: 'User', + id: '200', + name: 'Foo', + username: 'userfoo', + profile_picture: null, + }, + }); + }); + + internalAct(() => { + // Pass new fragment ref that points to new ID 200 + setSingularOwner(newQuery); + + // Flush some of the changes, but don't commit + expectSchedulerToFlushAndYieldThrough(['Hey user,', 'Foo']); + + // Trigger an update for initially rendered data and for the new data + // while second render is in progress + environment.commitUpdate(store => { + store.get('1')?.setValue('Alice in Wonderland', 'name'); + store.get('200')?.setValue('Foo Bar', 'name'); + }); + + // Assert the component renders the data from newQuery/newVariables, + // ignoring any updates triggered while render was in progress. + const expectedData = { + data: { + id: '200', + name: 'Foo', + profile_picture: null, + ...createFragmentRef('200', newQuery), + }, + }; + expectSchedulerToFlushAndYield([ + ['with id ', '200', '!'], + 'Hey user,', + 'Foo Bar', + ['with id ', '200', '!'], + ]); + assertFragmentResults([ + expectedData, + { + data: { + id: '200', + name: 'Foo Bar', + profile_picture: null, + ...createFragmentRef('200', newQuery), + }, + }, + ]); + + // Update latest rendered data + environment.commitPayload(newQuery, { + node: { + __typename: 'User', + id: '200', + // Update name + name: 'Foo Updated', + username: 'userfoo', + profile_picture: null, + }, + }); + expectSchedulerToFlushAndYield([ + 'Hey user,', + 'Foo Updated', + ['with id ', '200', '!'], + ]); + assertFragmentResults([ + { + data: { + id: '200', + // Assert name is updated + name: 'Foo Updated', + profile_picture: null, + ...createFragmentRef('200', newQuery), + }, + }, + ]); + }); + }); + it('should re-read and resubscribe to fragment when variables change', () => { renderSingularFragment(); assertFragmentResults([ diff --git a/packages/react-relay/relay-hooks/react-cache/useFragmentInternal_REACT_CACHE.js b/packages/react-relay/relay-hooks/react-cache/useFragmentInternal_REACT_CACHE.js index 2837bd65b9524..9d1adf182ab22 100644 --- a/packages/react-relay/relay-hooks/react-cache/useFragmentInternal_REACT_CACHE.js +++ b/packages/react-relay/relay-hooks/react-cache/useFragmentInternal_REACT_CACHE.js @@ -257,7 +257,17 @@ function subscribeToSnapshot( prevState.kind !== 'singular' || prevState.snapshot.selector !== latestSnapshot.selector ) { - return prevState; + const updates = handleMissedUpdates(environment, prevState); + if (updates != null) { + const [dataChanged, nextState] = updates; + environment.__log({ + name: 'useFragment.subscription.missedUpdates', + hasDataChanges: dataChanged, + }); + return dataChanged ? nextState : prevState; + } else { + return prevState; + } } return { kind: 'singular', @@ -280,7 +290,17 @@ function subscribeToSnapshot( prevState.kind !== 'plural' || prevState.snapshots[index]?.selector !== latestSnapshot.selector ) { - return prevState; + const updates = handleMissedUpdates(environment, prevState); + if (updates != null) { + const [dataChanged, nextState] = updates; + environment.__log({ + name: 'useFragment.subscription.missedUpdates', + hasDataChanges: dataChanged, + }); + return dataChanged ? nextState : prevState; + } else { + return prevState; + } } const updated = [...prevState.snapshots]; updated[index] = latestSnapshot; diff --git a/packages/relay-runtime/store/RelayStoreTypes.js b/packages/relay-runtime/store/RelayStoreTypes.js index dec08202fdba1..506330b28d1bd 100644 --- a/packages/relay-runtime/store/RelayStoreTypes.js +++ b/packages/relay-runtime/store/RelayStoreTypes.js @@ -714,6 +714,11 @@ export type LiveResolverBatchEndLogEvent = { +name: 'liveresolver.batch.end', }; +export type UseFragmentSubscriptionMissedUpdates = { + +name: 'useFragment.subscription.missedUpdates', + +hasDataChanges: boolean, +}; + export type LogEvent = | SuspenseFragmentLogEvent | SuspenseQueryLogEvent @@ -741,7 +746,8 @@ export type LogEvent = | StoreNotifySubscriptionLogEvent | EntrypointRootConsumeLogEvent | LiveResolverBatchStartLogEvent - | LiveResolverBatchEndLogEvent; + | LiveResolverBatchEndLogEvent + | UseFragmentSubscriptionMissedUpdates; export type LogFunction = LogEvent => void; export type LogRequestInfoFunction = mixed => void;