From ef8c95d3f87ffcf94ceb9d88d0446829766f07d7 Mon Sep 17 00:00:00 2001 From: Joe Savona Date: Tue, 7 Jun 2022 15:55:56 -0700 Subject: [PATCH] Fix to always render w latest state Summary: Fixes the new Cache-friendly `useFragment()` implementation to handle the case of props changing to point to a different entity. The use-case is: * Render user 1. This subscribes for updates on that user. * In a batch, delete user 1 and enqueue an update to render user 2 instead. * This should avoid incorrectly attempting to render the (deleted) user 1. Currently we render once w the stale data and then render again with the correct data. This is now fixed: in addition to enqueuing a setState when the entity changes we also eagerly render w the new entity. Due to useFragmentInternal()'s use of state we can't avoid a double render in this case, but we can avoid rendering the stale data. Reviewed By: davidmccabe Differential Revision: D36956052 fbshipit-source-id: 9e54a8de7c2358cb35fa9f39ba2848dae1e9dfdf --- .../__tests__/useFragmentNode-test.js | 51 ++++++++++++++++--- .../useFragmentInternal_REACT_CACHE.js | 14 +++-- 2 files changed, 56 insertions(+), 9 deletions(-) diff --git a/packages/react-relay/relay-hooks/__tests__/useFragmentNode-test.js b/packages/react-relay/relay-hooks/__tests__/useFragmentNode-test.js index 2a8595d965727..7a507a8ee2a80 100644 --- a/packages/react-relay/relay-hooks/__tests__/useFragmentNode-test.js +++ b/packages/react-relay/relay-hooks/__tests__/useFragmentNode-test.js @@ -154,6 +154,7 @@ describe.each([ let renderPluralFragment; let forceSingularUpdate; let commitSpy; + let renderSpy; let SingularRenderer; let PluralRenderer; @@ -180,6 +181,7 @@ describe.each([ useEffect(() => { commitSpy(data); }); + renderSpy(data); return [data]; } @@ -199,6 +201,32 @@ describe.each([ commitSpy.mockClear(); } + /// Asserts that a single rendering *batch* occurred, with possibly multiple render + /// calls and a single commit. `expectedCalls` describes the expected result as follows: + /// * items 0..length-1 (for length > 1) are calls expected to be rendered, but not committed + /// * item length-1 is expected to be rendered and committed + function assertRenderBatch( + expectedCalls: $ReadOnlyArray<{|data: $FlowFixMe|}>, + ) { + expect(expectedCalls.length >= 1).toBeTruthy(); // must expect at least one value + + // the issue is that the initial miss-updates-on-subscribe thing is + // only on the second runAllImmediates here. + // This ensures that useEffect runs + internalAct(() => jest.runAllImmediates()); + expect(renderSpy).toBeCalledTimes(expectedCalls.length); + expectedCalls.forEach((expected, idx) => { + const [actualData] = renderSpy.mock.calls[idx]; + expect(actualData).toEqual(expected.data); + }); + renderSpy.mockClear(); + + expect(commitSpy).toBeCalledTimes(1); + const [actualData] = commitSpy.mock.calls[0]; + expect(actualData).toEqual(expectedCalls[expectedCalls.length - 1].data); + commitSpy.mockClear(); + } + function createFragmentRef(id: string, owner: OperationDescriptor) { return { [ID_KEY]: id, @@ -215,6 +243,7 @@ describe.each([ return jest.requireActual('scheduler/unstable_mock'); }); commitSpy = jest.fn(); + renderSpy = jest.fn(); // Set up environment and base data environment = createMockEnvironment(); @@ -434,6 +463,7 @@ describe.each([ flushScheduler(); environment.mockClear(); commitSpy.mockClear(); + renderSpy.mockClear(); }); it('should render singular fragment without error when data is available', () => { @@ -651,7 +681,7 @@ describe.each([ it('should re-read and resubscribe to fragment when fragment pointers change', () => { renderSingularFragment(); - assertFragmentResults([ + assertRenderBatch([ { data: { id: '1', @@ -677,7 +707,10 @@ describe.each([ }, }); - internalAct(() => { + TestRenderer.act(() => { + environment.commitUpdate(store => { + store.delete('1'); + }); setSingularOwner(newQuery); }); @@ -689,9 +722,15 @@ describe.each([ // Assert that ref now points to newQuery owner ...createFragmentRef('200', newQuery), }; - assertFragmentResults([{data: expectedUser}]); - - internalAct(() => { + if (isUsingReactCacheImplementation) { + // React Cache renders twice (because it has to update state for derived data), + // but avoids rendering with stale data on the initial update + assertRenderBatch([{data: expectedUser}, {data: expectedUser}]); + } else { + assertRenderBatch([{data: expectedUser}]); + } + + TestRenderer.act(() => { environment.commitPayload(newQuery, { node: { __typename: 'User', @@ -703,7 +742,7 @@ describe.each([ }, }); }); - assertFragmentResults([ + assertRenderBatch([ { data: { id: '200', 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 08b59add5cabf..04f71179ab0a4 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 @@ -360,14 +360,14 @@ function useFragmentInternal_REACT_CACHE( return state; } }; - const state = stateFromRawState(rawState); + let state = stateFromRawState(rawState); // This copy of the state we only update when something requires us to // unsubscribe and re-subscribe, namely a changed environment or // fragment selector. const [rawSubscribedState, setSubscribedState] = useState(state); // FIXME since this is used as an effect dependency, it needs to be memoized. - const subscribedState = stateFromRawState(rawSubscribedState); + let subscribedState = stateFromRawState(rawSubscribedState); const [previousFragmentSelector, setPreviousFragmentSelector] = useState(fragmentSelector); @@ -376,11 +376,19 @@ function useFragmentInternal_REACT_CACHE( !areEqualSelectors(fragmentSelector, previousFragmentSelector) || environment !== previousEnvironment ) { + // Enqueue setState to record the new selector and state setPreviousFragmentSelector(fragmentSelector); setPreviousEnvironment(environment); - const newState = getFragmentState(environment, fragmentSelector, isPlural); + const newState = stateFromRawState( + getFragmentState(environment, fragmentSelector, isPlural), + ); setState(newState); setSubscribedState(newState); // This causes us to form a new subscription + // But render with the latest state w/o waiting for the setState. Otherwise + // the component would render the wrong information temporarily (including + // possibly incorrectly triggering some warnings below). + state = newState; + subscribedState = newState; } // Handle the queries for any missing client edges; this may suspend.