Apply suggestions from code review

Co-authored-by: Jerel Miller <jerelmiller@gmail.com>
apollographql · Jul 4, 2024 · dfda94e · dfda94e
1 parent a69327c
commit dfda94e
Show file tree

Hide file tree

Showing 2 changed files with 31 additions and 45 deletions.
diff --git a/src/react/hooks/useLazyQuery.ts b/src/react/hooks/useLazyQuery.ts
@@ -25,6 +25,7 @@ import {
   toQueryResult,
   useQueryInternals,
 } from "./useQuery.js";
+import { useIsomorphicLayoutEffect } from "./internal/useIsomorphicLayoutEffect.js";
 
 // The following methods, when called will execute the query, regardless of
 // whether the useLazyQuery execute function was called before.
@@ -188,7 +189,7 @@ export function useLazyQuery<
   );
 
   const executeRef = React.useRef(execute);
-  React.useLayoutEffect(() => {
+  useIsomorphicLayoutEffect(() => {
     executeRef.current = execute;
   });
 

diff --git a/src/react/hooks/useQuery.ts b/src/react/hooks/useQuery.ts
@@ -193,12 +193,7 @@ function useInternalState<
         (renderPromises &&
           renderPromises.getSSRObservable(makeWatchQueryOptions())) ||
         client.watchQuery(
-          getObsQueryOptions(
-            undefined,
-            client,
-            options,
-            makeWatchQueryOptions()
-          )
+          getObsQueryOptions(void 0, client, options, makeWatchQueryOptions())
         ),
       resultData: {
         // Reuse previousData from previous InternalState (if any) to provide
@@ -267,7 +262,7 @@ export function useQueryInternals<
   const renderPromises = React.useContext(getApolloContext()).renderPromises;
   const isSyncSSR = !!renderPromises;
   const disableNetworkFetches = client.disableNetworkFetches;
-  const ssrAllowed = !(options.ssr === false || options.skip);
+  const ssrAllowed = options.ssr !== false && !options.skip;
   const partialRefetch = options.partialRefetch;
 
   const makeWatchQueryOptions = createMakeWatchQueryOptions(
@@ -310,11 +305,7 @@ export function useQueryInternals<
     isSyncSSR
   );
 
-  useRegisterSSRObservable<TData, TVariables>(
-    observable,
-    renderPromises,
-    ssrAllowed
-  );
+  useRegisterSSRObservable(observable, renderPromises, ssrAllowed);
 
   const result = useObservableSubscriptionResult<TData, TVariables>(
     resultData,
@@ -349,7 +340,6 @@ function useObservableSubscriptionResult<
   disableNetworkFetches: boolean,
   partialRefetch: boolean | undefined,
   skipSubscribing: boolean,
-
   callbacks: {
     onCompleted: (data: TData) => void;
     onError: (error: ApolloError) => void;
@@ -483,11 +473,8 @@ function useObservableSubscriptionResult<
   );
 }
 
-function useRegisterSSRObservable<
-  TData = any,
-  TVariables extends OperationVariables = OperationVariables,
->(
-  observable: ObsQueryWithMeta<TData, TVariables>,
+function useRegisterSSRObservable(
+  observable: ObsQueryWithMeta<any, any>,
   renderPromises: RenderPromises | undefined,
   ssrAllowed: boolean
 ) {
@@ -569,32 +556,30 @@ function useResubscribeIfNecessary<
   options: QueryHookOptions<NoInfer<TData>, NoInfer<TVariables>>,
   watchQueryOptions: Readonly<WatchQueryOptions<TVariables, TData>>
 ) {
-  {
-    if (
-      observable[lastWatchOptions] &&
-      !equal(observable[lastWatchOptions], watchQueryOptions)
-    ) {
-      // Though it might be tempting to postpone this reobserve call to the
-      // useEffect block, we need getCurrentResult to return an appropriate
-      // loading:true result synchronously (later within the same call to
-      // useQuery). Since we already have this.observable here (not true for
-      // the very first call to useQuery), we are not initiating any new
-      // subscriptions, though it does feel less than ideal that reobserve
-      // (potentially) kicks off a network request (for example, when the
-      // variables have changed), which is technically a side-effect.
-      observable.reobserve(
-        getObsQueryOptions(observable, client, options, watchQueryOptions)
-      );
-
-      // Make sure getCurrentResult returns a fresh ApolloQueryResult<TData>,
-      // but save the current data as this.previousData, just like setResult
-      // usually does.
-      resultData.previousData =
-        resultData.current?.data || resultData.previousData;
-      resultData.current = void 0;
-    }
-    observable[lastWatchOptions] = watchQueryOptions;
+  if (
+    observable[lastWatchOptions] &&
+    !equal(observable[lastWatchOptions], watchQueryOptions)
+  ) {
+    // Though it might be tempting to postpone this reobserve call to the
+    // useEffect block, we need getCurrentResult to return an appropriate
+    // loading:true result synchronously (later within the same call to
+    // useQuery). Since we already have this.observable here (not true for
+    // the very first call to useQuery), we are not initiating any new
+    // subscriptions, though it does feel less than ideal that reobserve
+    // (potentially) kicks off a network request (for example, when the
+    // variables have changed), which is technically a side-effect.
+    observable.reobserve(
+      getObsQueryOptions(observable, client, options, watchQueryOptions)
+    );
+
+    // Make sure getCurrentResult returns a fresh ApolloQueryResult<TData>,
+    // but save the current data as this.previousData, just like setResult
+    // usually does.
+    resultData.previousData =
+      resultData.current?.data || resultData.previousData;
+    resultData.current = void 0;
   }
+  observable[lastWatchOptions] = watchQueryOptions;
 }
 
 /*
@@ -724,7 +709,7 @@ function setResult<TData, TVariables extends OperationVariables>(
   );
 }
 
-function handleErrorOrCompleted<TData, TVariables extends OperationVariables>(
+function handleErrorOrCompleted<TData>(
   result: ApolloQueryResult<TData>,
   previousResult: ApolloQueryResult<TData> | undefined,
   callbacks: Callbacks<TData>