enable react-hooks lint rules (#11511)

* enable `react-hooks` lint rules * Clean up Prettier, Size-limit, and Api-Extractor * update eslint rule * different type casting to not break lint rule parsing * fix up useMutation + lifecycle test * Clean up Prettier, Size-limit, and Api-Extractor * avoid reading/writing ref in render * fix up useLazyQuery * Clean up Prettier, Size-limit, and Api-Extractor * fix up another test doing a mutation in render * more useQuery cleanup * Clean up Prettier, Size-limit, and Api-Extractor * ignore rule of hook for context access * undo changes that were moved into separate PRs * Clean up Prettier, Size-limit, and Api-Extractor * almost full rewrite of the `useSubscription` hook * Clean up Prettier, Size-limit, and Api-Extractor * ignore any rule violation lint warnings in useSubscription and useQuery * changeset * rename patch * also add "eslint-plugin-react-compiler" rules --------- Co-authored-by: phryneas <phryneas@users.noreply.github.com>
apollographql · Jun 12, 2024 · 6536369 · 6536369
1 parent 08dbc02
commit 6536369
Show file tree

Hide file tree

Showing 13 changed files with 738 additions and 225 deletions.
diff --git a/.changeset/famous-camels-rescue.md b/.changeset/famous-camels-rescue.md
@@ -0,0 +1,5 @@
+---
+"@apollo/client": patch
+---
+
+`useLoadableQuery`: ensure that `loadQuery` is updated if the ApolloClient instance changes
diff --git a/.eslintrc b/.eslintrc
@@ -23,10 +23,13 @@
     {
       "files": ["**/*.ts", "**/*.tsx"],
       "excludedFiles": ["**/__tests__/**/*.*", "*.d.ts"],
+      "extends": ["plugin:react-hooks/recommended"],
       "parserOptions": {
         "project": "./tsconfig.json"
       },
+      "plugins": ["eslint-plugin-react-compiler"],
       "rules": {
+        "react-compiler/react-compiler": "error",
         "@typescript-eslint/consistent-type-imports": [
           "error",
           {

diff --git a/package-lock.json b/package-lock.json
diff --git a/package.json b/package.json
@@ -148,6 +148,8 @@
     "eslint-import-resolver-typescript": "3.6.1",
     "eslint-plugin-import": "npm:@phryneas/eslint-plugin-import@2.27.5-pr.2813.2817.199971c",
     "eslint-plugin-local-rules": "2.0.1",
+    "eslint-plugin-react-compiler": "0.0.0-experimental-c8b3f72-20240517",
+    "eslint-plugin-react-hooks": "4.6.2",
     "eslint-plugin-testing-library": "6.2.2",
     "expect-type": "0.19.0",
     "fetch-mock": "9.11.0",

diff --git a/patches/eslint-plugin-react-hooks+4.6.2.patch b/patches/eslint-plugin-react-hooks+4.6.2.patch
@@ -0,0 +1,28 @@
+diff --git a/node_modules/eslint-plugin-react-hooks/cjs/eslint-plugin-react-hooks.development.js b/node_modules/eslint-plugin-react-hooks/cjs/eslint-plugin-react-hooks.development.js
+index 441442f..d1ec5dc 100644
+--- a/node_modules/eslint-plugin-react-hooks/cjs/eslint-plugin-react-hooks.development.js
++++ b/node_modules/eslint-plugin-react-hooks/cjs/eslint-plugin-react-hooks.development.js
+@@ -905,7 +905,7 @@ var ExhaustiveDeps = {
+         var _callee = callee,
+             name = _callee.name;
+
+-        if (name === 'useRef' && id.type === 'Identifier') {
++        if ((name === 'useRef' || name === "useLazyRef") && id.type === 'Identifier') {
+           // useRef() return value is stable.
+           return true;
+         } else if (name === 'useState' || name === 'useReducer') {
+diff --git a/node_modules/eslint-plugin-react-hooks/index.js b/node_modules/eslint-plugin-react-hooks/index.js
+index 0e91baf..7e86d46 100644
+--- a/node_modules/eslint-plugin-react-hooks/index.js
++++ b/node_modules/eslint-plugin-react-hooks/index.js
+@@ -1,9 +1,3 @@
+ 'use strict';
+
+-// TODO: this doesn't make sense for an ESLint rule.
+-// We need to fix our build process to not create bundles for "raw" packages like this.
+-if (process.env.NODE_ENV === 'production') {
+-  module.exports = require('./cjs/eslint-plugin-react-hooks.production.min.js');
+-} else {
+-  module.exports = require('./cjs/eslint-plugin-react-hooks.development.js');
+-}
++module.exports = require('./cjs/eslint-plugin-react-hooks.development.js');
diff --git a/src/react/hooks/internal/useRenderGuard.ts b/src/react/hooks/internal/useRenderGuard.ts
@@ -12,6 +12,7 @@ Relay does this too, so we hope this is safe.
 https://github.com/facebook/relay/blob/8651fbca19adbfbb79af7a3bc40834d105fd7747/packages/react-relay/relay-hooks/loadQuery.js#L90-L98
 */
 export function useRenderGuard() {
+  // eslint-disable-next-line react-compiler/react-compiler
   RenderDispatcher = getRenderDispatcher();
 
   return React.useCallback(() => {

diff --git a/src/react/hooks/useLoadableQuery.ts b/src/react/hooks/useLoadableQuery.ts
@@ -245,12 +245,19 @@ export function useLoadableQuery<
 
       setQueryRef(wrapQueryRef(queryRef));
     },
-    [query, queryKey, suspenseCache, watchQueryOptions, calledDuringRender]
+    [
+      query,
+      queryKey,
+      suspenseCache,
+      watchQueryOptions,
+      calledDuringRender,
+      client,
+    ]
   );
 
   const reset: ResetFunction = React.useCallback(() => {
     setQueryRef(null);
-  }, [queryRef]);
+  }, []);
 
   return [loadQuery, queryRef, { fetchMore, refetch, reset }];
 }
diff --git a/src/react/hooks/useQuery.ts b/src/react/hooks/useQuery.ts
@@ -230,13 +230,16 @@ class InternalState<TData, TVariables extends OperationVariables> {
     // initialization, this.renderPromises is usually undefined (unless SSR is
     // happening), but that's fine as long as it has been initialized that way,
     // rather than left uninitialized.
+    // eslint-disable-next-line react-hooks/rules-of-hooks
     this.renderPromises = React.useContext(getApolloContext()).renderPromises;
 
     this.useOptions(options);
 
     const obsQuery = this.useObservableQuery();
 
+    // eslint-disable-next-line react-hooks/rules-of-hooks
     const result = useSyncExternalStore(
+      // eslint-disable-next-line react-hooks/rules-of-hooks
       React.useCallback(
         (handleStoreChange) => {
           if (this.renderPromises) {
@@ -307,7 +310,9 @@ class InternalState<TData, TVariables extends OperationVariables> {
           // effectively passing this dependency array to that useEffect buried
           // inside useSyncExternalStore, as desired.
           obsQuery,
+          // eslint-disable-next-line react-hooks/exhaustive-deps
           this.renderPromises,
+          // eslint-disable-next-line react-hooks/exhaustive-deps
           this.client.disableNetworkFetches,
         ]
       ),
@@ -533,6 +538,7 @@ class InternalState<TData, TVariables extends OperationVariables> {
       this.observable || // Reuse this.observable if possible (and not SSR)
       this.client.watchQuery(this.getObsQueryOptions()));
 
+    // eslint-disable-next-line react-hooks/rules-of-hooks
     this.obsQueryFields = React.useMemo(
       () => ({
         refetch: obsQuery.refetch.bind(obsQuery),

diff --git a/src/react/hooks/useQueryRefHandlers.ts b/src/react/hooks/useQueryRefHandlers.ts
@@ -59,6 +59,10 @@ export function useQueryRefHandlers<
       // client that's available to us at the current position in the React tree
       // that ApolloClient will then have the job to recreate a real queryRef from
       // the transported object
+      // This is just a context read - it's fine to do this conditionally.
+      // This hook wrapper also shouldn't be optimized by React Compiler.
+      // eslint-disable-next-line react-compiler/react-compiler
+      // eslint-disable-next-line react-hooks/rules-of-hooks
     : useApolloClient()
   )(queryRef);
 }

diff --git a/src/react/hooks/useReadQuery.ts b/src/react/hooks/useReadQuery.ts
@@ -52,6 +52,10 @@ export function useReadQuery<TData>(
       // client that's available to us at the current position in the React tree
       // that ApolloClient will then have the job to recreate a real queryRef from
       // the transported object
+      // This is just a context read - it's fine to do this conditionally.
+      // This hook wrapper also shouldn't be optimized by React Compiler.
+      // eslint-disable-next-line react-compiler/react-compiler
+      // eslint-disable-next-line react-hooks/rules-of-hooks
     : useApolloClient()
   )(queryRef);
 }
@@ -85,7 +89,7 @@ function _useReadQuery<TData>(
           forceUpdate();
         });
       },
-      [internalQueryRef]
+      [internalQueryRef, queryRef]
     ),
     getPromise,
     getPromise

diff --git a/src/react/hooks/useSubscription.ts b/src/react/hooks/useSubscription.ts
@@ -204,6 +204,8 @@ export function useSubscription<
     }
 
     Object.assign(ref.current, { client, subscription, options });
+    // eslint-disable-next-line react-compiler/react-compiler
+    // eslint-disable-next-line react-hooks/exhaustive-deps
   }, [client, subscription, options, canResetObservableRef.current]);
 
   React.useEffect(() => {
@@ -271,6 +273,8 @@ export function useSubscription<
         subscription.unsubscribe();
       });
     };
+    // eslint-disable-next-line react-compiler/react-compiler
+    // eslint-disable-next-line react-hooks/exhaustive-deps
   }, [observable]);
 
   return result;

diff --git a/src/react/hooks/useSuspenseQuery.ts b/src/react/hooks/useSuspenseQuery.ts
@@ -251,18 +251,18 @@ function _useSuspenseQuery<
   }, [queryRef.result]);
 
   const result = fetchPolicy === "standby" ? skipResult : __use(promise);
-  const fetchMore = React.useCallback(
-    ((options) => {
+
+  const fetchMore = React.useCallback<
+    FetchMoreFunction<unknown, OperationVariables>
+  >(
+    (options) => {
       const promise = queryRef.fetchMore(options);
       setPromise([queryRef.key, queryRef.promise]);
 
       return promise;
-    }) satisfies FetchMoreFunction<
-      unknown,
-      OperationVariables
-    > as FetchMoreFunction<TData | undefined, TVariables>,
+    },
     [queryRef]
-  );
+  ) as FetchMoreFunction<TData | undefined, TVariables>;
 
   const refetch: RefetchFunction<TData, TVariables> = React.useCallback(
     (variables) => {

diff --git a/src/react/hooks/useSyncExternalStore.ts b/src/react/hooks/useSyncExternalStore.ts
@@ -81,6 +81,8 @@ export const useSyncExternalStore: RealUseSESHookType =
           // Force a re-render.
           forceUpdate({ inst });
         }
+        // React Hook React.useLayoutEffect has a missing dependency: 'inst'. Either include it or remove the dependency array.
+        // eslint-disable-next-line react-hooks/exhaustive-deps
       }, [subscribe, value, getSnapshot]);
     } else {
       Object.assign(inst, { value, getSnapshot });
@@ -108,6 +110,8 @@ export const useSyncExternalStore: RealUseSESHookType =
           forceUpdate({ inst });
         }
       });
+      // React Hook React.useEffect has a missing dependency: 'inst'. Either include it or remove the dependency array.
+      // eslint-disable-next-line react-hooks/exhaustive-deps
     }, [subscribe]);
 
     return value;