Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

enable react-hooks lint rules #11511

Merged
merged 24 commits into from
Jun 12, 2024
Merged

enable react-hooks lint rules #11511

merged 24 commits into from
Jun 12, 2024

Conversation

phryneas
Copy link
Member

@phryneas phryneas commented Jan 22, 2024

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.

Copy link

changeset-bot bot commented Jan 22, 2024

🦋 Changeset detected

Latest commit: 9eaa883

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

Copy link
Contributor

github-actions bot commented Jan 22, 2024

size-limit report 📦

Path Size
dist/apollo-client.min.cjs 38.69 KB (0%)
import { ApolloClient, InMemoryCache, HttpLink } from "dist/main.cjs" 47.47 KB (0%)
import { ApolloClient, InMemoryCache, HttpLink } from "dist/main.cjs" (production) 45.04 KB (-0.01% 🔽)
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.29 KB (0%)
import { useQuery } from "dist/react/index.js" (production) 4.37 KB (0%)
import { useLazyQuery } from "dist/react/index.js" 5.57 KB (0%)
import { useLazyQuery } from "dist/react/index.js" (production) 4.64 KB (0%)
import { useMutation } from "dist/react/index.js" 3.54 KB (0%)
import { useMutation } from "dist/react/index.js" (production) 2.77 KB (0%)
import { useSubscription } from "dist/react/index.js" 3.21 KB (0%)
import { useSubscription } from "dist/react/index.js" (production) 2.4 KB (0%)
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%)

name = _callee.name;

- if (name === 'useRef' && id.type === 'Identifier') {
+ if ((name === 'useRef' || name === "useLazyRef") && id.type === 'Identifier') {
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yup - useLazyRef is used in useFragment.

const result: QueryResult<TData, TVariables> = Object.assign(useQueryResult, {
called: !!execOptionsRef.current,
});
useQueryResult.called = !!execOptionsRef.current;
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

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

return promise;
},
[]
[eagerMethods, initialFetchPolicy, internalState]
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

watchQueryOptions,
calledDuringRender,
client,
]
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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!

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

const ref = React.useRef({
const { current } = React.useRef({
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

@@ -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,
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

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

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Agreed

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

Object.assign(ref.current, { client, subscription, options });
}, [client, subscription, options, canResetObservableRef.current]);
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

TIL we might have a tendency to overuse refs 😅

Comment on lines 191 to 192
// 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?
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

See comment 🙃

@@ -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.
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

);

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

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 🙂

Copy link
Member

@jerelmiller jerelmiller left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

@github-actions github-actions bot added the auto-cleanup 🤖 label Feb 6, 2024
Copy link
Member Author

@phryneas phryneas left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Comment on the wrong PR and I cannot delete this 🤦

Copy link
Member

@alessbell alessbell left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

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

netlify bot commented May 16, 2024

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...

QR Code

Use your smartphone camera to open QR code link.

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

Copy link

netlify bot commented May 16, 2024

Deploy Preview for apollo-client-docs ready!

Name Link
🔨 Latest commit 9eaa883
🔍 Latest deploy log https://app.netlify.com/sites/apollo-client-docs/deploys/66695fc767876200080e3f70
😎 Deploy Preview https://deploy-preview-11511--apollo-client-docs.netlify.app
📱 Preview on mobile
Toggle QR Code...

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 phryneas marked this pull request as ready for review May 22, 2024 14:53
@phryneas phryneas requested a review from alessbell May 22, 2024 14:53
@phryneas
Copy link
Member Author

Adding the "eslint-plugin-react-compiler" lint package was very uneventful, so I just added that on top of this PR.

Copy link
Member

@jerelmiller jerelmiller left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🎉 will be nice to have this enabled

@phryneas phryneas merged commit 6536369 into main Jun 12, 2024
44 checks passed
@github-actions github-actions bot mentioned this pull request Jun 12, 2024
@github-actions github-actions bot locked as resolved and limited conversation to collaborators Jul 13, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants