Skip to content

Commit 8011e22

Browse files
alunyovfacebook-github-bot
authored andcommitted
Add areEqualOwners to check for structural equality of fragment owners. (#4500)
Summary: There is a bug in the experimental hooks implementation. useRefetchableFragment will return a new data object every time the refetch function is called, even if the data has not changed. This is because refetch creates a new fragment owner with createOperationDescriptor and refetch variables, and passes it to useFragmentInternal as part of the fragment key. Later in useFragmentInternal, we call areEqualSelectors to check if the fragment selector has changed. The owner is part of the selector, but we only check if the owner is the same object. However, it's possible that the owner is the same but just a new instance of the same object. Proposed fix: * Add areEqualOwners to check that owners are equivalent. This would prevent an endless loop of refetch/setState between components using useRefetchableFragment and the refetch call in the useEffect. Pull Request resolved: #4500 Test Plan: * New unit test-case to useRefetchableFragmentNode-test. * Updated tests for RelayModernSelector. Reviewed By: voideanvalue Differential Revision: D50524883 Pulled By: alunyov fbshipit-source-id: 6d9df3abc80ab997a59bf961e7a721da6ad769e3
1 parent d35e27b commit 8011e22

7 files changed

+417
-10
lines changed

packages/react-relay/relay-hooks/__tests__/__generated__/useRefetchableFragmentNodeTestIdentityTestFragment.graphql.js

+113
Some generated files are not rendered by default. Learn more about customizing how changed files appear on GitHub.

packages/react-relay/relay-hooks/__tests__/__generated__/useRefetchableFragmentNodeTestIdentityTestFragmentRefetchQuery.graphql.js

+178
Some generated files are not rendered by default. Learn more about customizing how changed files appear on GitHub.

packages/react-relay/relay-hooks/__tests__/useRefetchableFragmentNode-test.js

+72-2
Original file line numberDiff line numberDiff line change
@@ -40,6 +40,7 @@ import type {Query} from 'relay-runtime/util/RelayRuntimeTypes';
4040

4141
const {useTrackLoadQueryInRender} = require('../loadQuery');
4242
const useRefetchableFragmentInternal_REACT_CACHE = require('../react-cache/useRefetchableFragmentInternal_REACT_CACHE');
43+
const RelayEnvironmentProvider = require('../RelayEnvironmentProvider');
4344
const useRefetchableFragmentNode_LEGACY = require('../useRefetchableFragmentNode');
4445
const invariant = require('invariant');
4546
const React = require('react');
@@ -1393,7 +1394,7 @@ describe.each([
13931394
]);
13941395
});
13951396

1396-
it('warns if data retured has different __typename', () => {
1397+
it('warns if data returned has different __typename', () => {
13971398
const warning = require('warning');
13981399
// $FlowFixMe[prop-missing]
13991400
warning.mockClear();
@@ -1985,7 +1986,7 @@ describe.each([
19851986
{force: true},
19861987
);
19871988

1988-
// Assert we suspend on intial refetch request
1989+
// Assert we suspend on initial refetch request
19891990
expectFragmentIsRefetching(renderer, {
19901991
refetchQuery: refetchQuery1,
19911992
refetchVariables: refetchVariables1,
@@ -2154,6 +2155,75 @@ describe.each([
21542155

21552156
expect(fetchSpy).toBeCalledTimes(4);
21562157
});
2158+
2159+
it('preserves referential equality after refetch if data & variables have not changed', async () => {
2160+
let refetchCount = 0;
2161+
const ComponentWithUseEffectRefetch = (props: {
2162+
fragmentKey: any,
2163+
}): null => {
2164+
const {fragmentData, refetch} = useRefetchableFragmentNode(
2165+
graphql`
2166+
fragment useRefetchableFragmentNodeTestIdentityTestFragment on User
2167+
@refetchable(
2168+
queryName: "useRefetchableFragmentNodeTestIdentityTestFragmentRefetchQuery"
2169+
) {
2170+
id
2171+
name
2172+
profile_picture(scale: $scale) {
2173+
uri
2174+
}
2175+
}
2176+
`,
2177+
props.fragmentKey,
2178+
);
2179+
if (refetchCount > 2) {
2180+
throw new Error('Detected refetch loop.');
2181+
}
2182+
useEffect(() => {
2183+
refetchCount++;
2184+
refetch(fragmentData.id);
2185+
}, [fragmentData, refetch]);
2186+
2187+
return null;
2188+
};
2189+
const variables = {id: '1', scale: 16};
2190+
const query = createOperationDescriptor(
2191+
gqlRefetchQuery,
2192+
variables,
2193+
{},
2194+
);
2195+
environment.commitPayload(query, {
2196+
node: {
2197+
__typename: 'User',
2198+
id: '1',
2199+
name: 'Alice',
2200+
profile_picture: null,
2201+
},
2202+
});
2203+
let renderer;
2204+
TestRenderer.act(() => {
2205+
renderer = TestRenderer.create(
2206+
<ErrorBoundary fallback={({error}) => `Error: ${error.message}`}>
2207+
<React.Suspense fallback={'Loading'}>
2208+
<RelayEnvironmentProvider environment={environment}>
2209+
<ComponentWithUseEffectRefetch
2210+
fragmentKey={createFragmentRef(
2211+
'1',
2212+
query,
2213+
'useRefetchableFragmentNodeTestIdentityTestFragment',
2214+
)}
2215+
/>
2216+
</RelayEnvironmentProvider>
2217+
</React.Suspense>
2218+
</ErrorBoundary>,
2219+
// $FlowFixMe[prop-missing] - error revealed when flow-typing ReactTestRenderer
2220+
{unstable_isConcurrent: true},
2221+
);
2222+
jest.runAllImmediates();
2223+
});
2224+
expect(refetchCount).toBe(2);
2225+
expect(renderer?.toJSON()).toBe(null);
2226+
});
21572227
});
21582228

21592229
describe('fetchPolicy', () => {

0 commit comments

Comments
 (0)