enable `react-hooks` lint rules #11511

phryneas · 2024-01-22T14:34:21Z

This PR adds the react-hooks linter rules and addresses some of the issues it raises - it's not meant for immediate merging, but mostly as a place where we can discuss the different issues. We will probably spin off a few individual PRs from this and then merge this in at the end when everything that needs addressing has been addressed.

changeset-bot · 2024-01-22T14:34:25Z

🦋 Changeset detected

Latest commit: 619b188

The changes in this PR will be included in the next version bump.

This PR includes changesets to release 1 package

Name	Type
@apollo/client	Patch

Not sure what this means? Click here to learn what changesets are.

Click here if you're a maintainer who wants to add another changeset to this PR

github-actions · 2024-01-22T14:35:53Z

size-limit report 📦

Path	Size
dist/apollo-client.min.cjs	38.66 KB (+0.04% 🔺)
import { ApolloClient, InMemoryCache, HttpLink } from "dist/main.cjs"	47.44 KB (+0.04% 🔺)
import { ApolloClient, InMemoryCache, HttpLink } from "dist/main.cjs" (production)	45.04 KB (+0.13% 🔺)
import { ApolloClient, InMemoryCache, HttpLink } from "dist/index.js"	34.2 KB (0%)
import { ApolloClient, InMemoryCache, HttpLink } from "dist/index.js" (production)	32.05 KB (0%)
import { ApolloProvider } from "dist/react/index.js"	1.23 KB (0%)
import { ApolloProvider } from "dist/react/index.js" (production)	1.22 KB (0%)
import { useQuery } from "dist/react/index.js"	5.28 KB (0%)
import { useQuery } from "dist/react/index.js" (production)	4.36 KB (0%)
import { useLazyQuery } from "dist/react/index.js"	5.52 KB (0%)
import { useLazyQuery } from "dist/react/index.js" (production)	4.59 KB (0%)
import { useMutation } from "dist/react/index.js"	3.52 KB (0%)
import { useMutation } from "dist/react/index.js" (production)	2.74 KB (0%)
import { useSubscription } from "dist/react/index.js"	3.51 KB (+9.45% 🔺)
import { useSubscription } from "dist/react/index.js" (production)	2.67 KB (+11.35% 🔺)
import { useSuspenseQuery } from "dist/react/index.js"	5.44 KB (0%)
import { useSuspenseQuery } from "dist/react/index.js" (production)	4.1 KB (0%)
import { useBackgroundQuery } from "dist/react/index.js"	4.96 KB (0%)
import { useBackgroundQuery } from "dist/react/index.js" (production)	3.61 KB (0%)
import { useLoadableQuery } from "dist/react/index.js"	5.07 KB (-0.04% 🔽)
import { useLoadableQuery } from "dist/react/index.js" (production)	3.72 KB (0%)
import { useReadQuery } from "dist/react/index.js"	3.33 KB (+0.03% 🔺)
import { useReadQuery } from "dist/react/index.js" (production)	3.27 KB (+0.03% 🔺)
import { useFragment } from "dist/react/index.js"	2.29 KB (0%)
import { useFragment } from "dist/react/index.js" (production)	2.23 KB (0%)

phryneas · 2024-01-22T14:36:27Z

patches/eslint-plugin-react-hooks+4.6.0.patch

+             name = _callee.name;
+
+-        if (name === 'useRef' && id.type === 'Identifier') {
+        if ((name === 'useRef' || name === "useLazyRef") && id.type === 'Identifier') {


This cannot be configured, but we already have patch-package in the repo, soooo.. 😏

Out of curiosity, would we need this with the changes in #11403?

Yup - useLazyRef is used in useFragment.

phryneas · 2024-01-22T14:37:28Z

src/react/hooks/useLazyQuery.ts

-  const result: QueryResult<TData, TVariables> = Object.assign(useQueryResult, {
-    called: !!execOptionsRef.current,
-  });
+  useQueryResult.called = !!execOptionsRef.current;


This was introducing a second variable, result with a reference to useQueryResult - so the code after this treated both these variables as different objects while in reality they were the same object.

I've eliminated result here.

Alternatively, this might have been a mistake in the first place, and this should have been Object.assign({}, useQueryResult, { called }), but I don't think so.

Let's take the changes over in #11403 for anything useLazyQuery. Feel free to add additional comments in that PR for any other changes you'd like to see from this PR.

That implementation now properly uses deps and tests for stale closures, so hopefully that hook is "fixed" in this regard in that PR.

phryneas · 2024-01-22T14:42:13Z

src/react/hooks/useLazyQuery.ts

@@ -112,8 +112,8 @@ export function useLazyQuery<

      return promise;
    },
-    []
+    [eagerMethods, initialFetchPolicy, internalState]


eagerMethods and internalState are stable, but I believe initialFetchPolicy could be modified at some point by updating the options of the underlying observableQuery, which would not be picked up by execute up until now.

phryneas · 2024-01-22T14:43:35Z

src/react/hooks/useLoadableQuery.ts

+      watchQueryOptions,
+      calledDuringRender,
+      client,
+    ]


Up until now, loadQuery would not follow a new client instance.

@jerelmiller this might be something we should still take into account for 3.9, or at least 3.9.1?

Good call! I'm ok with doing a fast follow with 3.9.1 here if we want to keep with the code freeze. Thanks for finding!

phryneas · 2024-01-22T14:45:35Z

src/react/hooks/useMutation.ts

@@ -43,7 +43,7 @@ export function useMutation<
    client,
  });

-  const ref = React.useRef({
+  const { current } = React.useRef({


The lint rules had problems following deep access on ref.current.something, and since current is never reassigned I've destructured it here.

Is it even worth using a useRef here anymore? Based on the number of changes I'm seeing here, it might be worth looking into more of a full refactor. Then again, I haven't run through these changes locally to best understand them, so perhaps this makes the most sense.

We need something to hold a reference between renders. It could also be a useState where we'd never call the setter.

phryneas · 2024-01-22T14:52:27Z

src/react/hooks/useQuery.ts

@@ -251,8 +259,7 @@ class InternalState<TData, TVariables extends OperationVariables> {
          // effectively passing this dependency array to that useEffect buried
          // inside useSyncExternalStore, as desired.
          obsQuery,
-          this.renderPromises,
-          this.client.disableNetworkFetches,


this.client.disableNetworkFetches is never referenced within the callback, so the lint rules suggest to remove it. I assume that that's a good thing - ObservableQuery doesn't have a concept of disableNetworkFetches (it's a client property) and unsubscribing/resubscribing will happen so fast that it doesn't trigger a new attempt to make a request anyways.

Am I missing anything here?

phryneas · 2024-01-22T14:53:35Z

src/react/hooks/useReadQuery.ts

@@ -64,7 +64,7 @@ export function useReadQuery<TData>(
          forceUpdate();
        });
      },
-      [internalQueryRef]
+      [internalQueryRef, queryRef]


If queryRef changed, internalQueryRef should have changed anyways, but it doesn't hurt to have it here to stick to the rules.

phryneas · 2024-01-22T14:56:03Z

src/react/hooks/useSubscription.ts

@@ -120,7 +120,7 @@ export function useSubscription<
    }

    Object.assign(ref.current, { client, subscription, options });
-  }, [client, subscription, options, canResetObservableRef.current]);


canResetObservableRef is a ref and it's current value should not be used to trigger an effect, as changing it doesn't trigger a rerender and the resulting effect would be postponed to some random point in time in the future.

This might be a change in behaviour though - we need to be careful here.

TIL we might have a tendency to overuse refs 😅

phryneas · 2024-01-22T14:56:13Z

src/react/hooks/useSubscription.ts

+    // This seems like a valid complaint and we should investigate that.
+    // Generally: why is this a second `useEffect` and not part of the first one?


See comment 🙃

phryneas · 2024-01-22T14:56:51Z

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.


Tbh., I wouldn't touch our implementation of uSES for now and remove it completely in 4.0.

Agreed. To be honest, I don't completely understand why we didn't just use the official polyfill to begin with, but it is what it is! Totally fine leaving this one as-is.

jerelmiller · 2024-02-06T00:44:27Z

src/react/hooks/useLoadableQuery.ts

  );

  const reset: ResetFunction = React.useCallback(() => {
    setQueryRef(null);
-  }, [queryRef]);
+  }, []);


Oops, I wasn't careful enough in my own code review when we released this to catch this. Let's definitely get this change in a patch 🙂

jerelmiller

I'm generally in agreement with these changes. This is much needed!

For some of these, I think we'll just want to communicate some of the downstream effects in case we have users relying on the identity of some of these things that may now change more often (for good reasons). See this comment for an example.

phryneas

Comment on the wrong PR and I cannot delete this 🤦

alessbell

Also in favor of these changes, thanks for putting this together Lenz!

netlify · 2024-05-16T14:38:34Z

✅ Deploy Preview for apollo-client-docs ready!

Name	Link
🔨 Latest commit	`913ea1f`
🔍 Latest deploy log	https://app.netlify.com/sites/apollo-client-docs/deploys/66461939d694ec0008d75b8f
😎 Deploy Preview	https://deploy-preview-11511--apollo-client-docs.netlify.app
📱 Preview on mobile	Toggle QR Code... Use your smartphone camera to open QR code link.

To edit notification comments on pull requests, go to your Netlify site configuration.

netlify · 2024-05-16T14:44:03Z

✅ Deploy Preview for apollo-client-docs ready!

Name	Link
🔨 Latest commit	`e9c247b`
🔍 Latest deploy log	https://app.netlify.com/sites/apollo-client-docs/deploys/664e05e595d57d000808a510
😎 Deploy Preview	https://deploy-preview-11511--apollo-client-docs.netlify.app
📱 Preview on mobile	Toggle QR Code... Use your smartphone camera to open QR code link.

To edit notification comments on pull requests, go to your Netlify site configuration.

phryneas · 2024-05-17T13:22:57Z

I moved a bunch of changes out into #11851, #11852 and #11853 so this is more granular to review.

phryneas · 2024-05-17T13:24:57Z

src/react/hooks/useSuspenseQuery.ts

-  const fetchMore = React.useCallback(
-    ((options) => {
+
+  const fetchMore = React.useCallback<


The lint rule had massive problems parsing the satisfies ... as pattern, but this works.

enable react-hooks lint rules

2d5f02f

phryneas commented Jan 22, 2024

View reviewed changes

phryneas requested review from jerelmiller and alessbell January 22, 2024 14:57

jerelmiller reviewed Feb 6, 2024

View reviewed changes

jerelmiller approved these changes Feb 6, 2024

View reviewed changes

github-actions bot added the auto-cleanup 🤖 label Feb 6, 2024

phryneas commented Feb 7, 2024

View reviewed changes

phryneas mentioned this pull request Feb 7, 2024

Fix accidental double network call with useLazyQuery on some calls of the execute function #11403

Merged

alessbell approved these changes Mar 11, 2024

View reviewed changes

Merge remote-tracking branch 'origin/main' into pr/lint-hooks

913ea1f

phryneas changed the base branch from release-3.9 to main May 16, 2024 14:33

Clean up Prettier, Size-limit, and Api-Extractor

9e786aa

phryneas and others added 11 commits May 16, 2024 17:20

update eslint rule

d757781

different type casting to not break lint rule parsing

0e5e3cb

fix up useMutation + lifecycle test

6ad1b0e

Clean up Prettier, Size-limit, and Api-Extractor

172f26c

avoid reading/writing ref in render

c5a5d28

fix up useLazyQuery

6070945

Clean up Prettier, Size-limit, and Api-Extractor

9c78b5c

fix up another test doing a mutation in render

8a33815

more useQuery cleanup

1aeb9ff

Clean up Prettier, Size-limit, and Api-Extractor

c34a48c

ignore rule of hook for context access

34ef7d8

This was referenced May 17, 2024

useLazyQuery: fix rules of React violations #11851

Merged

useMutation: fix rules of React violations #11852

Merged

phryneas mentioned this pull request May 17, 2024

useQuery: adjust for rules of React linting #11853

Open

undo changes that were moved into separate PRs

c5bb497

Clean up Prettier, Size-limit, and Api-Extractor

8cdba15

phryneas commented May 17, 2024

View reviewed changes

phryneas and others added 2 commits May 17, 2024 18:30

almost full rewrite of the useSubscription hook

2e751e1

Clean up Prettier, Size-limit, and Api-Extractor

d70b45e

This was referenced May 17, 2024

Support extensions in useSubscription #11854

Open

useSubscription: fix rules of React violations #11863

Open

phryneas added 3 commits May 22, 2024 16:46

ignore any rule violation lint warnings in useSubscription and useQuery

0c813a1

changeset

e9c247b

rename patch

619b188

phryneas marked this pull request as ready for review May 22, 2024 14:53

phryneas requested a review from alessbell May 22, 2024 14:53

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

enable `react-hooks` lint rules #11511

enable `react-hooks` lint rules #11511

phryneas commented Jan 22, 2024 •

edited

changeset-bot bot commented Jan 22, 2024 •

edited

github-actions bot commented Jan 22, 2024 •

edited

phryneas Jan 22, 2024

jerelmiller Feb 6, 2024

phryneas Feb 6, 2024

phryneas Jan 22, 2024

jerelmiller Feb 6, 2024

phryneas Jan 22, 2024

phryneas Jan 22, 2024

jerelmiller Jan 22, 2024

phryneas Jan 22, 2024

jerelmiller Feb 6, 2024

phryneas Feb 6, 2024

phryneas Jan 22, 2024

phryneas Jan 22, 2024

jerelmiller Feb 6, 2024

phryneas Jan 22, 2024

jerelmiller Feb 6, 2024

phryneas Jan 22, 2024

phryneas Jan 22, 2024

jerelmiller Feb 6, 2024

jerelmiller Feb 6, 2024

jerelmiller left a comment

phryneas left a comment •

edited

alessbell left a comment

netlify bot commented May 16, 2024

netlify bot commented May 16, 2024 •

edited

phryneas commented May 17, 2024

phryneas May 17, 2024

		// This seems like a valid complaint and we should investigate that.
		// Generally: why is this a second `useEffect` and not part of the first one?

enable react-hooks lint rules #11511

Are you sure you want to change the base?

enable react-hooks lint rules #11511

Conversation

phryneas commented Jan 22, 2024 • edited

changeset-bot bot commented Jan 22, 2024 • edited

🦋 Changeset detected

github-actions bot commented Jan 22, 2024 • edited

size-limit report 📦

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jerelmiller left a comment

Choose a reason for hiding this comment

phryneas left a comment • edited

Choose a reason for hiding this comment

alessbell left a comment

Choose a reason for hiding this comment

netlify bot commented May 16, 2024

✅ Deploy Preview for apollo-client-docs ready!

netlify bot commented May 16, 2024 • edited

✅ Deploy Preview for apollo-client-docs ready!

phryneas commented May 17, 2024

Choose a reason for hiding this comment

enable `react-hooks` lint rules #11511

enable `react-hooks` lint rules #11511

phryneas commented Jan 22, 2024 •

edited

changeset-bot bot commented Jan 22, 2024 •

edited

github-actions bot commented Jan 22, 2024 •

edited

phryneas left a comment •

edited

netlify bot commented May 16, 2024 •

edited