From f766e8305d9f2dbde59a61b8e70c99c4b2b67d55 Mon Sep 17 00:00:00 2001 From: Lenz Weber-Tronic Date: Fri, 28 Jul 2023 09:42:33 +0200 Subject: [PATCH] Adjust relative timing of `useFragment` and `useQuery` (#11083) * work on useFragment timing * changeset * all tests green * remove most `IS_REACT_18` checks * changeset * Update src/react/hooks/useQuery.ts * Update .changeset/honest-ads-act.md Co-authored-by: Jerel Miller * feedback from code review --------- Co-authored-by: Jerel Miller --- .changeset/honest-ads-act.md | 5 + .size-limit.cjs | 2 +- .../__tests__/client/Mutation.test.tsx | 27 +- .../__tests__/client/Query.test.tsx | 57 ++--- .../__tests__/client/Subscription.test.tsx | 18 +- .../hoc/__tests__/mutations/queries.test.tsx | 29 +-- .../mutations/recycled-queries.test.tsx | 130 +++++----- .../hoc/__tests__/queries/errors.test.tsx | 30 +-- .../hoc/__tests__/queries/lifecycle.test.tsx | 22 +- .../hoc/__tests__/queries/loading.test.tsx | 61 +++-- src/react/hoc/__tests__/queries/skip.test.tsx | 19 +- .../hooks/__tests__/useFragment.test.tsx | 237 +++++++++++++++++- src/react/hooks/useFragment.ts | 9 +- src/react/hooks/useLazyQuery.ts | 2 +- src/react/hooks/useQuery.ts | 29 ++- 15 files changed, 426 insertions(+), 251 deletions(-) create mode 100644 .changeset/honest-ads-act.md diff --git a/.changeset/honest-ads-act.md b/.changeset/honest-ads-act.md new file mode 100644 index 00000000000..35a2d2bc27d --- /dev/null +++ b/.changeset/honest-ads-act.md @@ -0,0 +1,5 @@ +--- +'@apollo/client': patch +--- + +Adjust the rerender timing of `useQuery` to more closely align with `useFragment`. This means that cache updates delivered to both hooks should trigger renders at relatively the same time. Previously, the `useFragment` might rerender much faster leading to some confusion. diff --git a/.size-limit.cjs b/.size-limit.cjs index 54cacceb903..653df42d84e 100644 --- a/.size-limit.cjs +++ b/.size-limit.cjs @@ -1,7 +1,7 @@ const checks = [ { path: "dist/apollo-client.min.cjs", - limit: "38042" + limit: "38056" }, { path: "dist/main.cjs", diff --git a/src/react/components/__tests__/client/Mutation.test.tsx b/src/react/components/__tests__/client/Mutation.test.tsx index 3cc17e57fd3..e0e08f83e4f 100644 --- a/src/react/components/__tests__/client/Mutation.test.tsx +++ b/src/react/components/__tests__/client/Mutation.test.tsx @@ -17,8 +17,6 @@ import { import { Query } from '../../Query'; import { Mutation } from '../../Mutation'; -const IS_REACT_18 = React.version.startsWith('18'); - const mutation = gql` mutation createTodo($text: String!) { createTodo { @@ -978,7 +976,7 @@ describe('General Mutation testing', () => { ); }); - it('allows a refetchQueries prop as string and variables have updated', async () => new Promise((resolve, reject) => { + it('allows a refetchQueries prop as string and variables have updated', async () => { const query = gql` query people($first: Int) { allPeople(first: $first) { @@ -1026,6 +1024,7 @@ describe('General Mutation testing', () => { const refetchQueries = ['people']; let count = 0; + let testFailures: any[] = []; const Component: React.FC>> = props => { const [variables, setVariables] = useState(props.variables); @@ -1055,19 +1054,20 @@ describe('General Mutation testing', () => { // mutation loading expect(resultMutation.loading).toBe(true); } else if (count === 5) { - // mutation loaded - expect(resultMutation.loading).toBe(false); + // query refetched or mutation loaded + // or both finished batched together + // hard to make assumptions here } else if (count === 6) { - // query refetched + // both loaded expect(resultQuery.loading).toBe(false); expect(resultMutation.loading).toBe(false); expect(resultQuery.data).toEqual(peopleData3); } else { - reject(`Too many renders (${count})`); + throw new Error(`Too many renders (${count})`); } count++; } catch (err) { - reject(err); + testFailures.push(err); } return null; }} @@ -1083,10 +1083,13 @@ describe('General Mutation testing', () => { ); - waitFor(() => { - expect(count).toEqual(IS_REACT_18 ? 6 : 7); - }).then(resolve, reject); - })); + await waitFor(() => { + if (testFailures.length > 0) { + throw testFailures[0]; + } + expect(count).toEqual(7); + }); + }); it('allows refetchQueries to be passed to the mutate function', () => new Promise((resolve, reject) => { const query = gql` diff --git a/src/react/components/__tests__/client/Query.test.tsx b/src/react/components/__tests__/client/Query.test.tsx index 5e8ebdd1415..ea18591712f 100644 --- a/src/react/components/__tests__/client/Query.test.tsx +++ b/src/react/components/__tests__/client/Query.test.tsx @@ -11,8 +11,6 @@ import { ApolloProvider } from '../../../context'; import { itAsync, MockedProvider, mockSingleLink } from '../../../../testing'; import { Query } from '../../Query'; -const IS_REACT_18 = React.version.startsWith('18'); - const allPeopleQuery: DocumentNode = gql` query people { allPeople(first: 1) { @@ -293,11 +291,7 @@ describe('Query component', () => { } if (count === 3) { // second data - if (IS_REACT_18) { - expect(data).toEqual(data3); - } else { - expect(data).toEqual(data2); - } + expect(data).toEqual(data2); } if (count === 5) { // third data @@ -338,11 +332,7 @@ describe('Query component', () => { ); waitFor(() => { - if (IS_REACT_18) { - expect(count).toBe(4); - } else { - expect(count).toBe(6); - } + expect(count).toBe(6); }).then(resolve, reject); }); @@ -688,11 +678,7 @@ describe('Query component', () => { }); } if (count === 2) { - if (IS_REACT_18) { - expect(result.loading).toBeFalsy(); - } else { - expect(result.loading).toBeTruthy(); - } + expect(result.loading).toBeTruthy(); } if (count === 3) { expect(result.loading).toBeFalsy(); @@ -714,11 +700,7 @@ describe('Query component', () => { ); waitFor(() => { - if (IS_REACT_18) { - expect(count).toBe(3) - } else { - expect(count).toBe(4) - } + expect(count).toBe(4) }).then(resolve, reject); }); @@ -1378,11 +1360,7 @@ describe('Query component', () => { break; case 3: // Second response loading - if (IS_REACT_18) { - expect(props.loading).toBe(false); - } else { - expect(props.loading).toBe(true); - } + expect(props.loading).toBe(true); break; case 4: // Second response received, fire another refetch @@ -1416,11 +1394,7 @@ describe('Query component', () => { render(); waitFor(() => { - if (IS_REACT_18) { - expect(count).toBe(4) - } else { - expect(count).toBe(7) - } + expect(count).toBe(7) }).then(resolve, reject); }); }); @@ -1502,6 +1476,7 @@ describe('Query component', () => { }); let count = 0; + let testFailures: any[] = []; const noop = () => null; const AllPeopleQuery2 = Query; @@ -1530,11 +1505,7 @@ describe('Query component', () => { break; case 2: // Waiting for the second result to load - if (IS_REACT_18) { - expect(result.loading).toBe(false); - } else { - expect(result.loading).toBe(true); - } + expect(result.loading).toBe(true); break; case 3: setTimeout(() => { @@ -1561,7 +1532,10 @@ describe('Query component', () => { throw new Error('Unexpected fall through'); } } catch (e) { - fail(e); + // if we throw the error inside the component, + // we will get more rerenders in the test, but the `expect` error + // might not propagate anyways + testFailures.push(e); } return null; }} @@ -1576,11 +1550,10 @@ describe('Query component', () => { ); await waitFor(() => { - if (IS_REACT_18) { - expect(count).toBe(3) - } else { - expect(count).toBe(6) + if (testFailures.length > 0) { + throw testFailures[0]; } + expect(count).toBe(6); }); }); diff --git a/src/react/components/__tests__/client/Subscription.test.tsx b/src/react/components/__tests__/client/Subscription.test.tsx index c5cec1e4d52..7fd0ccd95e0 100644 --- a/src/react/components/__tests__/client/Subscription.test.tsx +++ b/src/react/components/__tests__/client/Subscription.test.tsx @@ -415,7 +415,7 @@ itAsync('renders an error', (resolve, reject) => { }); describe('should update', () => { - itAsync('if the client changes', (resolve, reject) => { + it('if the client changes', async () => { const link2 = new MockSubscriptionLink(); const client2 = new ApolloClient({ link: link2, @@ -423,6 +423,7 @@ describe('should update', () => { }); let count = 0; + let testFailures: any[] = []; class Component extends React.Component { state = { @@ -436,7 +437,7 @@ describe('should update', () => { {(result: any) => { const { loading, data } = result; try { - switch (count) { + switch (count++) { case 0: expect(loading).toBeTruthy(); expect(data).toBeUndefined(); @@ -465,12 +466,12 @@ describe('should update', () => { expect(loading).toBeFalsy(); expect(data).toEqual(results[1].result.data); break; + default: + throw new Error("too many rerenders"); } } catch (error) { - reject(error); + testFailures.push(error); } - - count++; return null; }} @@ -483,7 +484,12 @@ describe('should update', () => { link.simulateResult(results[0]); - waitFor(() => expect(count).toBe(5)).then(resolve, reject); + await waitFor(() => { + if (testFailures.length > 0) { + throw testFailures[0]; + } + expect(count).toBe(5); + }); }); itAsync('if the query changes', (resolve, reject) => { diff --git a/src/react/hoc/__tests__/mutations/queries.test.tsx b/src/react/hoc/__tests__/mutations/queries.test.tsx index 408e960ba36..d3b31512b41 100644 --- a/src/react/hoc/__tests__/mutations/queries.test.tsx +++ b/src/react/hoc/__tests__/mutations/queries.test.tsx @@ -14,8 +14,6 @@ import { import { graphql } from '../../graphql'; import { ChildProps } from '../../types'; -const IS_REACT_18 = React.version.startsWith('18'); - describe('graphql(mutation) query integration', () => { itAsync('allows for passing optimisticResponse for a mutation', (resolve, reject) => { const query: DocumentNode = gql` @@ -194,15 +192,6 @@ describe('graphql(mutation) query integration', () => { mutationData.createTodo ]); break; - case 3: - if (IS_REACT_18) { - expect(this.props.data.todo_list.tasks).toEqual([ - mutationData.createTodo - ]); - } else { - reject(`too many renders (${renderCount})`); - } - break; default: reject(`too many renders (${renderCount})`); } @@ -220,11 +209,7 @@ describe('graphql(mutation) query integration', () => { ); waitFor(() => { - if (IS_REACT_18) { - expect(renderCount).toBe(3); - } else { - expect(renderCount).toBe(2); - } + expect(renderCount).toBe(2); }).then(resolve, reject); }); @@ -325,11 +310,7 @@ describe('graphql(mutation) query integration', () => { class extends React.Component> { render() { if (count === 1) { - if (IS_REACT_18) { - expect(this.props.data!.mini).toEqual(mutationData.mini); - } else { - expect(this.props.data!.mini).toEqual(queryData.mini); - } + expect(this.props.data!.mini).toEqual(queryData.mini); } if (count === 2) { expect(this.props.data!.mini).toEqual( @@ -354,11 +335,7 @@ describe('graphql(mutation) query integration', () => { ); waitFor(() => { - if (IS_REACT_18) { - expect(count).toBe(2); - } else { - expect(count).toBe(3); - } + expect(count).toBe(3); }).then(resolve, reject); }); diff --git a/src/react/hoc/__tests__/mutations/recycled-queries.test.tsx b/src/react/hoc/__tests__/mutations/recycled-queries.test.tsx index 9ce625f7610..4b2eb96b78d 100644 --- a/src/react/hoc/__tests__/mutations/recycled-queries.test.tsx +++ b/src/react/hoc/__tests__/mutations/recycled-queries.test.tsx @@ -11,8 +11,6 @@ import { mockSingleLink } from '../../../../testing'; import { graphql } from '../../graphql'; import { ChildProps } from '../../types'; -const IS_REACT_18 = React.version.startsWith('18') - describe('graphql(mutation) update queries', () => { // This is a long test that keeps track of a lot of stuff. It is testing // whether or not the `options.update` reducers will run even when a given @@ -128,6 +126,7 @@ describe('graphql(mutation) update queries', () => { let queryMountCount = 0; let queryUnmountCount = 0; let queryRenderCount = 0; + const testFailures: any[] = []; const MyQuery = graphql<{}, QueryData>(query)( class extends React.Component> { @@ -140,55 +139,60 @@ describe('graphql(mutation) update queries', () => { } render() { - switch (queryRenderCount) { - case 0: - expect(this.props.data!.loading).toBeTruthy(); - expect(this.props.data!.todo_list).toBeFalsy(); - break; - case 1: - expect(this.props.data!.loading).toBeFalsy(); - if (!IS_REACT_18) { + try { + switch (queryRenderCount) { + case 0: + expect(this.props.data!.loading).toBeTruthy(); + expect(this.props.data!.todo_list).toBeFalsy(); + break; + case 1: + expect(this.props.data!.loading).toBeFalsy(); expect(this.props.data!.todo_list).toEqual({ id: '123', title: 'how to apollo', tasks: [] }); - } - break; - case 2: - expect(this.props.data!.loading).toBeFalsy(); - expect(queryMountCount).toBe(1); - expect(this.props.data!.todo_list).toEqual({ - id: '123', - title: 'how to apollo', - tasks: [ - { - id: '99', - text: 'This one was created with a mutation.', - completed: true - } - ] - }); - break; - case 3: - expect(this.props.data!.todo_list).toEqual({ - id: '123', - title: 'how to apollo', - tasks: [ - { - id: '99', - text: 'This one was created with a mutation.', - completed: true - }, - { - id: '99', - text: 'This one was created with a mutation.', - completed: true - } - ] - }); - break; - default: + break; + case 2: + expect(this.props.data!.loading).toBeFalsy(); + expect(queryMountCount).toBe(1); + expect(this.props.data!.todo_list).toEqual({ + id: '123', + title: 'how to apollo', + tasks: [ + { + id: '99', + text: 'This one was created with a mutation.', + completed: true + }, + ] + }); + break; + case 3: + expect(this.props.data!.loading).toBeFalsy(); + expect(queryMountCount).toBe(1); + expect(this.props.data!.todo_list).toEqual({ + id: '123', + title: 'how to apollo', + tasks: [ + { + id: '99', + text: 'This one was created with a mutation.', + completed: true + }, + { + id: '99', + text: 'This one was created with a mutation.', + completed: true + } + ] + }); + break; + default: + throw new Error("too many rerenders") + } + } catch (e) { + testFailures.push(e); } queryRenderCount += 1; @@ -209,35 +213,41 @@ describe('graphql(mutation) update queries', () => { ); - setTimeout(() => { + let resolveLastTimeout: () => void; + const allTimeoutsFinished = new Promise(r => { resolveLastTimeout = r }); + + const catchingSetTimeout = (cb: (args: void) => void, ms: number) => { + return setTimeout(() => { + try { cb() } catch (e) { testFailures.push(e) } + }, ms); + } + + catchingSetTimeout(() => { mutate(); - setTimeout(() => { - if (IS_REACT_18) { - expect(queryUnmountCount).toBe(1); - } else { - expect(queryUnmountCount).toBe(0); - } + catchingSetTimeout(() => { + expect(queryUnmountCount).toBe(0); query1Unmount(); expect(queryUnmountCount).toBe(1); - setTimeout(() => { + catchingSetTimeout(() => { mutate(); - setTimeout(() => { + catchingSetTimeout(() => { const { unmount: query2Unmount } = render( ); - setTimeout(() => { + catchingSetTimeout(() => { mutationUnmount(); query2Unmount(); expect(todoUpdateQueryCount).toBe(2); expect(queryMountCount).toBe(2); expect(queryUnmountCount).toBe(2); + resolveLastTimeout!(); }, 5); }, 5); }, 5); @@ -245,10 +255,12 @@ describe('graphql(mutation) update queries', () => { }, 5); await waitFor(() => { - if (!IS_REACT_18) { - expect(queryRenderCount).toBe(4); - } + expect(queryRenderCount).toBe(4); }); + await allTimeoutsFinished; + if (testFailures.length > 0) { + throw testFailures[0]; + } }); it('will run `refetchQueries` for a recycled queries', async () => { diff --git a/src/react/hoc/__tests__/queries/errors.test.tsx b/src/react/hoc/__tests__/queries/errors.test.tsx index 0ab2b2ca7df..bcf3822e2cd 100644 --- a/src/react/hoc/__tests__/queries/errors.test.tsx +++ b/src/react/hoc/__tests__/queries/errors.test.tsx @@ -13,8 +13,6 @@ import { Query } from '../../../components/Query'; import { graphql } from '../../graphql'; import { ChildProps, DataValue } from '../../types'; -const IS_REACT_18 = React.version.startsWith('18'); - describe('[queries] errors', () => { let error: typeof console.error; beforeEach(() => { @@ -376,11 +374,7 @@ describe('[queries] errors', () => { }); break; case 1: - if (IS_REACT_18) { - expect(props.data!.loading).toBeFalsy(); - } else { - expect(props.data!.loading).toBeTruthy(); - } + expect(props.data!.loading).toBeTruthy(); expect(props.data!.allPeople).toEqual( data.allPeople ); @@ -413,11 +407,7 @@ describe('[queries] errors', () => { ); waitFor(() => { - if (IS_REACT_18) { - expect(count).toBe(2); - } else { - expect(count).toBe(3) - } + expect(count).toBe(3) }).then(resolve, reject); }); @@ -464,11 +454,7 @@ describe('[queries] errors', () => { .catch(noop); break; case 1: - if (IS_REACT_18) { - expect(props.data!.loading).toBeFalsy(); - } else { - expect(props.data!.loading).toBeTruthy(); - } + expect(props.data!.loading).toBeTruthy(); break; case 2: expect(props.data!.loading).toBeFalsy(); @@ -511,11 +497,7 @@ describe('[queries] errors', () => { ); waitFor(() => { - if (IS_REACT_18) { - expect(count).toBe(2) - } else { - expect(count).toBe(5) - } + expect(count).toBe(5) }).then(resolve, reject); }); @@ -636,9 +618,7 @@ describe('[queries] errors', () => { ); waitFor(() => { - if (!IS_REACT_18) { - expect(done).toBeTruthy() - } + expect(done).toBeTruthy() }).then(resolve, reject); }); diff --git a/src/react/hoc/__tests__/queries/lifecycle.test.tsx b/src/react/hoc/__tests__/queries/lifecycle.test.tsx index 1198794bda6..2ecd58c5891 100644 --- a/src/react/hoc/__tests__/queries/lifecycle.test.tsx +++ b/src/react/hoc/__tests__/queries/lifecycle.test.tsx @@ -11,8 +11,6 @@ import { Query as QueryComponent } from '../../../components'; import { graphql } from '../../graphql'; import { ChildProps } from '../../types'; -const IS_REACT_18 = React.version.startsWith('18'); - describe('[queries] lifecycle', () => { // lifecycle it('reruns the query if it changes', async () => { @@ -512,9 +510,7 @@ describe('[queries] lifecycle', () => { rerender = render(app).rerender; await waitFor(() => { - if (!IS_REACT_18) { - expect(done).toBeTruthy() - } + expect(done).toBeTruthy() }); }); @@ -571,6 +567,7 @@ describe('[queries] lifecycle', () => { let switchClient: (client: ApolloClient) => void; let refetchQuery: () => void; let count = 0; + let testFailures: any[] = []; const Query = graphql<{}, Data>(query, { options: { notifyOnNetworkStatusChange: true } @@ -602,11 +599,7 @@ describe('[queries] lifecycle', () => { refetchQuery!(); break; case 3: - if (IS_REACT_18) { - expect({ loading }).toEqual({ loading: false }); - } else { - expect({ loading }).toEqual({ loading: true }); - } + expect({ loading }).toEqual({ loading: true }); expect({ a, b, c }).toEqual({ a: 1, b: 2, @@ -702,7 +695,7 @@ describe('[queries] lifecycle', () => { fail(`Unexpectedly many renders (${count})`); } } catch (err) { - fail(err); + testFailures.push(err); } return null; @@ -733,11 +726,10 @@ describe('[queries] lifecycle', () => { render(); await waitFor(() => { - if (IS_REACT_18) { - expect(count).toBe(3) - } else { - expect(count).toBe(12) + if (testFailures.length > 0) { + throw testFailures[0]; } + expect(count).toBe(12) }); }); diff --git a/src/react/hoc/__tests__/queries/loading.test.tsx b/src/react/hoc/__tests__/queries/loading.test.tsx index 678f5388942..819ab63390f 100644 --- a/src/react/hoc/__tests__/queries/loading.test.tsx +++ b/src/react/hoc/__tests__/queries/loading.test.tsx @@ -10,8 +10,6 @@ import { itAsync, mockSingleLink } from '../../../../testing'; import { graphql } from '../../graphql'; import { ChildProps } from '../../types'; -const IS_REACT_18 = React.version.startsWith('18'); - describe('[queries] loading', () => { // networkStatus / loading itAsync('exposes networkStatus as a part of the props api', (resolve, reject) => { @@ -300,13 +298,8 @@ describe('[queries] loading', () => { }); break; case 1: - if (IS_REACT_18) { - expect(data!.loading).toBeFalsy(); - expect(data!.networkStatus).toBe(NetworkStatus.ready); - } else { - expect(data!.loading).toBeTruthy(); - expect(data!.networkStatus).toBe(NetworkStatus.refetch); - } + expect(data!.loading).toBeTruthy(); + expect(data!.networkStatus).toBe(NetworkStatus.refetch); expect(data!.allPeople).toEqual(data!.allPeople); break; case 2: @@ -332,11 +325,7 @@ describe('[queries] loading', () => { ); waitFor(() => { - if (IS_REACT_18) { - expect(count).toBe(2) - } else { - expect(count).toBe(3) - } + expect(count).toBe(3) }).then(resolve, reject); }); @@ -474,6 +463,7 @@ describe('[queries] loading', () => { }); let renderFn: (num: number) => React.ReactElement, count = 0; + const testFailures: any[] = []; interface Props { first: number; @@ -483,25 +473,33 @@ describe('[queries] loading', () => { })( class extends React.Component> { componentDidUpdate() { - if (count === 0) { - // has data - unmount(); - setTimeout(() => { - render(renderFn(2)); - }, 5); - } + try { + if (count === 0) { + // has data + unmount(); + setTimeout(() => { + render(renderFn(2)); + }, 5); + } - if (count === 2) { - // remounted data after fetch - expect(this.props.data!.loading).toBeFalsy(); + if (count === 2) { + // remounted data after fetch + expect(this.props.data!.loading).toBeFalsy(); + } + count++; + } catch (e) { + testFailures.push(e); } - count++; } render() { - if (count === 1) { - expect(this.props.data!.loading).toBeTruthy(); // on remount - count++; + try { + if (count === 1) { + expect(this.props.data!.loading).toBeTruthy(); // on remount + count++; + } + } catch (e) { + testFailures.push(e); } return null; @@ -516,11 +514,10 @@ describe('[queries] loading', () => { ); const { unmount } = render(renderFn(1)); waitFor(() => { - if (IS_REACT_18) { - expect(count).toBe(0) - } else { - expect(count).toBe(3) + if (testFailures.length > 0) { + throw testFailures[0]; } + expect(count).toBe(3); }).then(resolve, reject); }); diff --git a/src/react/hoc/__tests__/queries/skip.test.tsx b/src/react/hoc/__tests__/queries/skip.test.tsx index a45751da2ec..4a25a770c73 100644 --- a/src/react/hoc/__tests__/queries/skip.test.tsx +++ b/src/react/hoc/__tests__/queries/skip.test.tsx @@ -11,8 +11,6 @@ import { itAsync, mockSingleLink } from '../../../../testing'; import { graphql } from '../../graphql'; import { ChildProps } from '../../types'; -const IS_REACT_18 = React.version.startsWith('18'); - describe('[queries] skip', () => { itAsync('allows you to skip a query without running it', (resolve, reject) => { const query: DocumentNode = gql` @@ -691,14 +689,9 @@ describe('[queries] skip', () => { break; case 6: expect(this.props.skip).toBe(false); - if (IS_REACT_18) { - expect(this.props.data!.loading).toBe(false); - expect(this.props.data.allPeople).toEqual(finalData.allPeople); - } else { - expect(ranQuery).toBe(3); - expect(this.props.data.allPeople).toEqual(nextData.allPeople); - expect(this.props.data!.loading).toBe(true); - } + expect(ranQuery).toBe(3); + expect(this.props.data.allPeople).toEqual(nextData.allPeople); + expect(this.props.data!.loading).toBe(true); break; case 7: // The next batch of data has loaded. @@ -738,11 +731,7 @@ describe('[queries] skip', () => { ); waitFor(() => { - if (IS_REACT_18) { - expect(count).toEqual(6) - } else { - expect(count).toEqual(7) - } + expect(count).toEqual(7) }).then(resolve, reject); })); diff --git a/src/react/hooks/__tests__/useFragment.test.tsx b/src/react/hooks/__tests__/useFragment.test.tsx index 789750c2e55..b12e8b4f758 100644 --- a/src/react/hooks/__tests__/useFragment.test.tsx +++ b/src/react/hooks/__tests__/useFragment.test.tsx @@ -1,5 +1,5 @@ import * as React from "react"; -import { render, waitFor, screen, renderHook } from "@testing-library/react"; +import { render, waitFor, screen, renderHook, within } from "@testing-library/react"; import userEvent from '@testing-library/user-event'; import { act } from "react-dom/test-utils"; @@ -16,11 +16,13 @@ import { ApolloLink, StoreObject, DocumentNode, + FetchResult, } from "../../../core"; import { useQuery } from "../useQuery"; import { concatPagination } from "../../../utilities"; import assert from "assert"; import { expectTypeOf } from 'expect-type' +import { SubscriptionObserver } from "zen-observable-ts"; describe("useFragment", () => { it("is importable and callable", () => { @@ -175,7 +177,7 @@ describe("useFragment", () => { act(() => { cache.modify({ fields: { - list(list: Reference[], { readField }) { + list(list: readonly Reference[], { readField }) { return [ ...list, cache.writeFragment({ @@ -309,6 +311,39 @@ describe("useFragment", () => { }); }); + it("returns data on first render", () => { + const ItemFragment: TypedDocumentNode = gql` + fragment ItemFragment on Item { + id + text + } + `; + const cache = new InMemoryCache(); + const item = { __typename: "Item", id: 1, text: "Item #1" }; + cache.writeFragment({ + fragment: ItemFragment, + data: item, + }) + const client = new ApolloClient({ + cache, + }) + function Component(){ + const { data } = useFragment({ + fragment: ItemFragment, + from: { __typename: "Item", id: 1 } + }) + return <>{data.text} + } + render( + + + , + ); + + // would throw if not present synchronously + screen.getByText(/Item #1/); + }) + it.each>([ // This query uses a basic field-level @nonreactive directive. gql` @@ -807,7 +842,7 @@ describe("useFragment", () => { act(() => { cache.modify({ fields: { - list(list: Reference[], { readField }) { + list(list: readonly Reference[], { readField }) { return [ ...list, cache.writeFragment({ @@ -1048,8 +1083,8 @@ describe("useFragment", () => { }); }); + await waitFor(() => expect(renderResult.current.data).toEqual(data125)); expect(renderResult.current.complete).toBe(false); - expect(renderResult.current.data).toEqual(data125); expect(renderResult.current.missing).toEqual({ list: { // Even though Query.list is actually an array in the data, data paths @@ -1083,8 +1118,8 @@ describe("useFragment", () => { }); }); + await waitFor(() => expect(renderResult.current.data).toEqual(data182WithText)); expect(renderResult.current.complete).toBe(true); - expect(renderResult.current.data).toEqual(data182WithText); expect(renderResult.current.missing).toBeUndefined(); checkHistory(3); @@ -1108,13 +1143,13 @@ describe("useFragment", () => { }, })); - expect(renderResult.current.complete).toBe(false); - expect(renderResult.current.data).toEqual({ + await waitFor(() => expect(renderResult.current.data).toEqual({ list: [ { __typename: "Item", id: 1, text: "oyez1" }, { __typename: "Item", id: 2 }, ], - }); + })); + expect(renderResult.current.complete).toBe(false); expect(renderResult.current.missing).toEqual({ // TODO Figure out why Item:8 is not represented here. Likely because of // auto-filtering of dangling references from arrays, but that should @@ -1416,6 +1451,192 @@ describe("useFragment", () => { }); }); +describe("has the same timing as `useQuery`", () => { + const itemFragment = gql` + fragment ItemFragment on Item { + id + title + } + `; + + it("both in same component", async () => { + const initialItem = { __typename: "Item", id: 1, title: "Item #initial" }; + const updatedItem = { __typename: "Item", id: 1, title: "Item #updated" }; + + const query = gql` + query { + item { + ...ItemFragment + } + } + ${itemFragment} + `; + let observer: SubscriptionObserver; + const cache = new InMemoryCache(); + const client = new ApolloClient({ + cache, + link: new ApolloLink(operation => new Observable(o => void (observer = o))), + }); + + function Component(){ + const { data: queryData } = useQuery(query, { returnPartialData: true }); + const { data: fragmentData, complete } = useFragment({ + fragment: itemFragment, + from: initialItem, + }); + + if (!queryData) { + expect(fragmentData).toStrictEqual({}); + } else { + expect({ item: fragmentData }).toStrictEqual(queryData); + } + return complete ? JSON.stringify(fragmentData) : 'loading'; + } + render(, { wrapper: ({ children }) => ( + {children} + )}); + await screen.findByText(/loading/); + assert(observer!) + observer.next({ data: { item: initialItem } }); + observer.complete(); + await screen.findByText(/Item #initial/) + cache.writeQuery({ query, data: { item: updatedItem } }); + await screen.findByText(/Item #updated/) + await new Promise(resolve => setTimeout(resolve, 50)) + }); + + it("`useQuery` in parent, `useFragment` in child", async () => { + const item1 = { __typename: "Item", id: 1, title: "Item #1" }; + const item2 = { __typename: "Item", id: 2, title: "Item #2" }; + const query: TypedDocumentNode<{items: Array}> = gql` + query { + items { + ...ItemFragment + } + } + ${itemFragment} + `; + const cache = new InMemoryCache(); + const client = new ApolloClient({ + cache + }); + cache.writeQuery({ query, data: { items: [ item1, item2 ] } }); + + const valuePairs: Array<[item: string, parentCount: number, childCount: number]> = [] + function captureDOMState(){ + const parent = screen.getByTestId("parent"); + const children = screen.getByTestId("children"); + valuePairs.push(['Item 1', within(parent).queryAllByText(/Item #1/).length, within(children).queryAllByText(/Item #1/).length]) + valuePairs.push(['Item 2', within(parent).queryAllByText(/Item #2/).length, within(children).queryAllByText(/Item #2/).length]) + } + + function Parent(){ + const { data } = useQuery(query); + if (!data) throw new Error("should never happen") + React.useEffect(captureDOMState); + return <> +

{JSON.stringify(data)}

+
{data.items.map((item, i) =>

)}
+ + } + function Child({ id }: { id: number }){ + const {data} = useFragment({ + fragment: itemFragment, + from: { __typename: "Item", id }, + }) + React.useEffect(captureDOMState); + return <>{JSON.stringify({item: data })} + } + + render(, { wrapper: ({ children }) => ( + {children} + )}); + cache.evict({ + id: cache.identify(item2) + }); + await waitFor(() => { + expect(() => screen.getByText(/Item #2/)).toThrow() + }) + + for (const [_item, parentChount, childCount] of valuePairs) { + expect(parentChount).toBe(childCount) + } + }); + + /** + * This would be optimal, but would only work if `useFragment` and + * `useQuery` had exactly the same timing, which is not the case with + * the current implementation. + * The best we can do is to make sure that `useFragment` is not + * faster than `useQuery` in reasonable cases (of course, `useQuery` + * could trigger a network request on cache update, which would be slower + * than `useFragment`, no matter how much we delay it). + * If we change the core implementation into a more synchronous one, + * we should try to get this test to work, too. + */ + it.failing("`useFragment` in parent, `useQuery` in child", async () => { + const item1 = { __typename: "Item", id: 1, title: "Item #1" }; + const item2 = { __typename: "Item", id: 2, title: "Item #2" }; + const query: TypedDocumentNode<{items: Array}> = gql` + query { + items { + ...ItemFragment + } + } + ${itemFragment} + `; + const cache = new InMemoryCache(); + const client = new ApolloClient({ + cache + }); + cache.writeQuery({ query, data: { items: [ item1, item2 ] } }); + + const valuePairs: Array<[item: string, parentCount: number, childCount: number]> = [] + function captureDOMState(){ + const parent = screen.getByTestId("parent"); + const children = screen.getByTestId("children"); + valuePairs.push(['Item 1', within(parent).queryAllByText(/Item #1/).length, within(children).queryAllByText(/Item #1/).length]) + valuePairs.push(['Item 2', within(parent).queryAllByText(/Item #2/).length, within(children).queryAllByText(/Item #2/).length]) + } + + function Parent(){ + const {data: data1} = useFragment({ + fragment: itemFragment, + from: { __typename: "Item", id: 1 }, + }) + const {data: data2} = useFragment({ + fragment: itemFragment, + from: { __typename: "Item", id: 2 }, + }) + React.useEffect(captureDOMState); + return <> +

{JSON.stringify(data1)}

{JSON.stringify(data2)}

+

+ + } + function Child(){ + const { data } = useQuery(query); + if (!data) throw new Error("should never happen") + React.useEffect(captureDOMState); + return <>{JSON.stringify(data)} + } + + render(, { wrapper: ({ children }) => ( + {children} + )}); + act(() => void cache.evict({ + id: cache.identify(item2) + })); + await waitFor(() => { + expect(() => screen.getByText(/Item #2/)).toThrow() + }) + + for (const [_item, parentChount, childCount] of valuePairs) { + expect(parentChount).toBe(childCount) + } + }); +}); + describe.skip("Type Tests", () => { test('NoInfer prevents adding arbitrary additional variables', () => { const typedNode = {} as TypedDocumentNode<{ foo: string}, { bar: number }> diff --git a/src/react/hooks/useFragment.ts b/src/react/hooks/useFragment.ts index aef7a50db8b..71d9f012466 100644 --- a/src/react/hooks/useFragment.ts +++ b/src/react/hooks/useFragment.ts @@ -83,16 +83,21 @@ export function useFragment< return useSyncExternalStore( (forceUpdate) => { - return cache.watch({ + let lastTimeout = 0; + const unsubcribe = cache.watch({ ...diffOptions, immediate: true, callback(diff) { if (!equal(diff, latestDiff)) { resultRef.current = diffToResult((latestDiff = diff)); - forceUpdate(); + lastTimeout = setTimeout(forceUpdate) as any; } }, }); + return () => { + unsubcribe(); + clearTimeout(lastTimeout); + } }, getSnapshot, getSnapshot diff --git a/src/react/hooks/useLazyQuery.ts b/src/react/hooks/useLazyQuery.ts index 700f05a7740..7f3c06ef0a7 100644 --- a/src/react/hooks/useLazyQuery.ts +++ b/src/react/hooks/useLazyQuery.ts @@ -68,7 +68,7 @@ export function useLazyQuery( // setTick function. Updating this state by calling state.forceUpdate is the // only way we trigger React component updates (no other useState calls within // the InternalState class). - const [_tick, setTick] = React.useState(0); - state.forceUpdate = () => { - setTick(tick => tick + 1); - }; + state.forceUpdateState = React.useReducer(tick => tick + 1, 0)[1]; return state; } @@ -94,11 +91,24 @@ class InternalState { } } - forceUpdate() { + /** + * Forces an update using local component state. + * As this is not batched with `useSyncExternalStore` updates, + * this is only used as a fallback if the `useSyncExternalStore` "force update" + * method is not registered at the moment. + * See https://github.com/facebook/react/issues/25191 + * */ + forceUpdateState() { // Replaced (in useInternalState) with a method that triggers an update. invariant.warn("Calling default no-op implementation of InternalState#forceUpdate"); } + /** + * Will be overwritten by the `useSyncExternalStore` "force update" method + * whenever it is available and reset to `forceUpdateState` when it isn't. + */ + forceUpdate = () => this.forceUpdateState(); + executeQuery(options: QueryHookOptions & { query?: DocumentNode; }) { @@ -160,11 +170,13 @@ class InternalState { const obsQuery = this.useObservableQuery(); const result = useSyncExternalStore( - React.useCallback(() => { + React.useCallback((handleStoreChange) => { if (this.renderPromises) { return () => {}; } + this.forceUpdate = handleStoreChange; + const onNext = () => { const previousResult = this.result; // We use `getCurrentResult()` instead of the onNext argument because @@ -227,7 +239,10 @@ class InternalState { // This way, an existing subscription can be reused without an additional // request if "unsubscribe" and "resubscribe" to the same ObservableQuery // happen in very fast succession. - return () => setTimeout(() => subscription.unsubscribe()); + return () => { + setTimeout(() => subscription.unsubscribe()); + this.forceUpdate = () => this.forceUpdateState(); + }; }, [ // We memoize the subscribe function using useCallback and the following // dependency keys, because the subscribe function reference is all that