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

useLazyQuery: fix rules of React violations #11851

Merged
merged 4 commits into from
May 21, 2024
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
5 changes: 5 additions & 0 deletions .changeset/shaggy-mirrors-judge.md
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.
5 changes: 5 additions & 0 deletions .changeset/stupid-planes-nail.md
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.
2 changes: 1 addition & 1 deletion .size-limits.json
Original file line number Diff line number Diff line change
@@ -1,4 +1,4 @@
{
"dist/apollo-client.min.cjs": 39574,
"dist/apollo-client.min.cjs": 39607,
"import { ApolloClient, InMemoryCache, HttpLink } from \"dist/index.js\" (production)": 32821
}
26 changes: 15 additions & 11 deletions src/react/hooks/useLazyQuery.ts
Original file line number Diff line number Diff line change
Expand Up @@ -9,7 +9,6 @@ import type {
LazyQueryHookOptions,
LazyQueryResultTuple,
NoInfer,
QueryResult,
} from "../types/types.js";
import { useInternalState } from "./useQuery.js";
import { useApolloClient } from "./useApolloClient.js";
Expand Down Expand Up @@ -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;
Copy link
Member Author

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 unlike result (where they are spread in), it's stable.

// 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]);
Copy link
Member Author

Choose a reason for hiding this comment

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

Before this, these methods would never follow switching clients.


const called = !!execOptionsRef.current;
const result = React.useMemo(
() => ({
...useQueryResult,
...eagerMethods,
called,
}),
[useQueryResult, eagerMethods, called]
);

const execute = React.useCallback<LazyQueryResultTuple<TData, TVariables>[0]>(
(executeOptions) => {
Expand Down Expand Up @@ -147,7 +151,7 @@ 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.

Before this, execute would never follow switching clients.

);

return [execute, result];
Expand Down
37 changes: 22 additions & 15 deletions src/react/hooks/useQuery.ts
Original file line number Diff line number Diff line change
Expand Up @@ -109,23 +109,30 @@ export function useInternalState<TData, TVariables extends OperationVariables>(
client: ApolloClient<any>,
Copy link
Member Author

Choose a reason for hiding this comment

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

Pulling that hook into this PR because useLazyQuery contained a bunch of untrue assumptions about internalState's referential stability.

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;
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 a rule of React violation and would cause bugs in scenario where a client changed instance in a suspended subtree.

Copy link
Member

Choose a reason for hiding this comment

The 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?

Copy link
Member Author

Choose a reason for hiding this comment

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

Choose a reason for hiding this comment

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

Going with useState and an in-render setState call instead, see https://react.dev/reference/react/useState#storing-information-from-previous-renders


return state;
}
Expand Down Expand Up @@ -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"
>;
Expand Down