useLazyQuery: fix rules of React violations #11851
Conversation
🦋 Changeset detectedLatest commit: 76693c1 The changes in this PR will be included in the next version bump. This PR includes changesets to release 1 package
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 |
size-limit report 📦
|
phryneas
force-pushed
the
pr/rules-of-hooks/useLazyQuery
branch
from
73571f9
to
b328604
Compare
✅ Deploy Preview for apollo-client-docs ready!
To edit notification comments on pull requests, go to your Netlify site configuration. |
| called: !!execOptionsRef.current, | ||
| }); | ||
|
|
||
| const { forceUpdateState, obsQueryFields } = internalState; |
There was a problem hiding this comment.
internalState.obsQueryFields contains all the methods we need, and unlike result (where they are spread in), it's stable.
| }, []); | ||
|
|
||
| Object.assign(result, eagerMethods); | ||
| }, [forceUpdateState, obsQueryFields]); |
There was a problem hiding this comment.
Before this, these methods would never follow switching clients.
| @@ -147,7 +151,7 @@ export function useLazyQuery< | |||
|
|
|||
| return promise; | |||
| }, | |||
| [] | |||
| [eagerMethods, initialFetchPolicy, internalState] | |||
There was a problem hiding this comment.
Before this, execute would never follow switching clients.
| ) { | ||
| stateRef.current = new InternalState(client, query, stateRef.current); | ||
| } | ||
| const state = stateRef.current; |
There was a problem hiding this comment.
This was a rule of React violation and would cause bugs in scenario where a client changed instance in a suspended subtree.
There was a problem hiding this comment.
Any chance we could have a test that does this so we ensure a future rewrite of this hook doesn't break things?
There was a problem hiding this comment.
Honestly, I wouldn't know where to start testing that - this is extremely theoretical 😅
| let [state, updateState] = React.useState(createInternalState); | ||
|
|
||
| if (client !== state.client || query !== state.query) { | ||
| // If the client or query have changed, we need to create a new InternalState. | ||
| // This will trigger a re-render with the new state, but it will also continue | ||
| // to run the current render function to completion. | ||
| // Since we sometimes trigger some side-effects in the render function, we | ||
| // re-assign `state` to the new state to ensure that those side-effects are | ||
| // triggered with the new state. | ||
| updateState((state = createInternalState(state))); | ||
| } |
There was a problem hiding this comment.
Going with useState and an in-render setState call instead, see https://react.dev/reference/react/useState#storing-information-from-previous-renders
| @@ -109,23 +109,30 @@ export function useInternalState<TData, TVariables extends OperationVariables>( | |||
| client: ApolloClient<any>, | |||
There was a problem hiding this comment.
Pulling that hook into this PR because useLazyQuery contained a bunch of untrue assumptions about internalState's referential stability.
✅ Deploy Preview for apollo-client-docs ready!
To edit notification comments on pull requests, go to your Netlify site configuration. |
I'm splitting #11511 up a bit, this is the first of those PRs.