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 <jerelmiller@gmail.com> * feedback from code review --------- Co-authored-by: Jerel Miller <jerelmiller@gmail.com>
apollographql · Jul 28, 2023 · f766e83 · f766e83
1 parent e3d611d
commit f766e83
Show file tree

Hide file tree

Showing 15 changed files with 426 additions and 251 deletions.
diff --git a/.changeset/honest-ads-act.md 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
@@ -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
@@ -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<PropsWithChildren<PropsWithChildren<any>>> = 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', () => {
       </MockedProvider>
     );
 
-    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
@@ -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(<SomeComponent />);
 
       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
@@ -415,14 +415,15 @@ 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,
       cache: new Cache({ addTypename: false })
     });
 
     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;
               }}
             </Subscription>
@@ -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
@@ -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<ChildProps<Variables, Data>> {
         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);
   });