-
Notifications
You must be signed in to change notification settings - Fork 2.7k
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
useLazyQuery: fix rules of React violations #11851
Changes from 2 commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,5 @@ | ||
--- | ||
"@apollo/client": patch | ||
--- | ||
|
||
Avoid usage of useRef in useInternalState to prevent ref access in render. |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,5 @@ | ||
--- | ||
"@apollo/client": patch | ||
--- | ||
|
||
Fix a bug where `useLazyQuery` would not pick up a client change. |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -1,4 +1,4 @@ | ||
{ | ||
"dist/apollo-client.min.cjs": 39573, | ||
"dist/apollo-client.min.cjs": 39607, | ||
"import { ApolloClient, InMemoryCache, HttpLink } from \"dist/index.js\" (production)": 32821 | ||
} |
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -9,7 +9,6 @@ import type { | |
LazyQueryHookOptions, | ||
LazyQueryResultTuple, | ||
NoInfer, | ||
QueryResult, | ||
} from "../types/types.js"; | ||
import { useInternalState } from "./useQuery.js"; | ||
import { useApolloClient } from "./useApolloClient.js"; | ||
|
@@ -95,30 +94,35 @@ export function useLazyQuery< | |
useQueryResult.observable.options.initialFetchPolicy || | ||
internalState.getDefaultFetchPolicy(); | ||
|
||
const result: QueryResult<TData, TVariables> = Object.assign(useQueryResult, { | ||
called: !!execOptionsRef.current, | ||
}); | ||
|
||
const { forceUpdateState, obsQueryFields } = internalState; | ||
// We use useMemo here to make sure the eager methods have a stable identity. | ||
const eagerMethods = React.useMemo(() => { | ||
const eagerMethods: Record<string, any> = {}; | ||
for (const key of EAGER_METHODS) { | ||
const method = result[key]; | ||
const method = obsQueryFields[key]; | ||
eagerMethods[key] = function () { | ||
if (!execOptionsRef.current) { | ||
execOptionsRef.current = Object.create(null); | ||
// Only the first time populating execOptionsRef.current matters here. | ||
internalState.forceUpdateState(); | ||
forceUpdateState(); | ||
} | ||
// @ts-expect-error this is just too generic to type | ||
return method.apply(this, arguments); | ||
}; | ||
} | ||
|
||
return eagerMethods; | ||
}, []); | ||
|
||
Object.assign(result, eagerMethods); | ||
}, [forceUpdateState, obsQueryFields]); | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Before this, these methods would never follow switching |
||
|
||
const called = !!execOptionsRef.current; | ||
const result = React.useMemo( | ||
() => ({ | ||
...useQueryResult, | ||
...eagerMethods, | ||
called, | ||
}), | ||
[useQueryResult, eagerMethods, called] | ||
); | ||
|
||
const execute = React.useCallback<LazyQueryResultTuple<TData, TVariables>[0]>( | ||
(executeOptions) => { | ||
|
@@ -147,7 +151,7 @@ export function useLazyQuery< | |
|
||
return promise; | ||
}, | ||
[] | ||
[eagerMethods, initialFetchPolicy, internalState] | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Before this, |
||
); | ||
|
||
return [execute, result]; | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -109,23 +109,30 @@ export function useInternalState<TData, TVariables extends OperationVariables>( | |
client: ApolloClient<any>, | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Pulling that hook into this PR because |
||
query: DocumentNode | TypedDocumentNode<TData, TVariables> | ||
): InternalState<TData, TVariables> { | ||
const stateRef = React.useRef<InternalState<TData, TVariables>>(); | ||
if ( | ||
!stateRef.current || | ||
client !== stateRef.current.client || | ||
query !== stateRef.current.query | ||
) { | ||
stateRef.current = new InternalState(client, query, stateRef.current); | ||
} | ||
const state = stateRef.current; | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Honestly, I wouldn't know where to start testing that - this is extremely theoretical 😅 |
||
|
||
// By default, InternalState.prototype.forceUpdate is an empty function, but | ||
// we replace it here (before anyone has had a chance to see this state yet) | ||
// with a function that unconditionally forces an update, using the latest | ||
// setTick function. Updating this state by calling state.forceUpdate is the | ||
// only way we trigger React component updates (no other useState calls within | ||
// the InternalState class). | ||
state.forceUpdateState = React.useReducer((tick) => tick + 1, 0)[1]; | ||
// setTick function. Updating this state by calling state.forceUpdate or the | ||
// uSES notification callback are the only way we trigger React component updates. | ||
const forceUpdateState = React.useReducer((tick) => tick + 1, 0)[1]; | ||
|
||
function createInternalState(previous?: InternalState<TData, TVariables>) { | ||
return Object.assign(new InternalState(client, query, previous), { | ||
forceUpdateState, | ||
}); | ||
} | ||
|
||
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))); | ||
} | ||
Comment on lines
+125
to
+135
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Going with |
||
|
||
return state; | ||
} | ||
|
@@ -511,7 +518,7 @@ class InternalState<TData, TVariables extends OperationVariables> { | |
private onError(error: ApolloError) {} | ||
|
||
private observable!: ObservableQuery<TData, TVariables>; | ||
private obsQueryFields!: Omit< | ||
public obsQueryFields!: Omit< | ||
ObservableQueryFields<TData, TVariables>, | ||
"variables" | ||
>; | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
internalState.obsQueryFields
contains all the methods we need, and unlikeresult
(where they are spread in), it's stable.