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

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

Merged
merged 47 commits into from
Feb 7, 2024
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
47 commits
Select commit Hold shift + click to select a range
6e44067
Provide failing test for #9448
jerelmiller Dec 1, 2023
8d3d0fe
Merge branch 'main' into jerel/issue-9448-lazy-query-double-call
jerelmiller Feb 1, 2024
eb924aa
Add qualifier to test that is specific to no-cache fetch policy
jerelmiller Feb 1, 2024
eda5450
Move no-cache to hook for explictness
jerelmiller Feb 1, 2024
6a9f81a
Fix assertion against incorrect data
jerelmiller Feb 1, 2024
2b271d0
Maintain existing options in execOptionsRef between calls to execute
jerelmiller Feb 1, 2024
c0687ba
Add changeset
jerelmiller Feb 1, 2024
1160142
Test against all fetch policies that guarantee network requests
jerelmiller Feb 1, 2024
4110dab
Merge branch 'main' into jerel/issue-9448-lazy-query-double-call
jerelmiller Feb 1, 2024
eb53ea2
Update size limits
jerelmiller Feb 1, 2024
a334ec4
Remove accidental call to expect.anything() and replace with real value
jerelmiller Feb 1, 2024
760b92d
Resolve with different data when id is null in test
jerelmiller Feb 2, 2024
660b1d2
Update expectation on data returned from 2nd execute call
jerelmiller Feb 2, 2024
b2b61a1
Use result.current to trigger execution
jerelmiller Feb 2, 2024
8a4acbf
Revert change to store previous result in execOptionsRef
jerelmiller Feb 2, 2024
173d976
Don't merge previous variables in execute function when calling witho…
jerelmiller Feb 2, 2024
48af005
Remove unneeded optionsRef
jerelmiller Feb 2, 2024
1bf404c
Pass query as dep to execute useCallback
jerelmiller Feb 2, 2024
590115d
Add initialFetchPolicy to dep on useCallback
jerelmiller Feb 2, 2024
6234179
Update size-limits
jerelmiller Feb 2, 2024
40d86a5
Merge branch 'main' into jerel/issue-9448-lazy-query-double-call
jerelmiller Feb 2, 2024
0d2be0d
Add note about variables merging
jerelmiller Feb 2, 2024
93ff34a
Add new useStableCallback internal hook
jerelmiller Feb 2, 2024
31911f9
Fix for stale closures and stable execute callback
jerelmiller Feb 2, 2024
bf81a2a
Better test stability of execute function and stale closures
jerelmiller Feb 2, 2024
04215dd
Update size limits
jerelmiller Feb 2, 2024
895af15
Add comment on useStableCallback
jerelmiller Feb 2, 2024
16acef0
Add note about stable callbacks in useLazyQuery
jerelmiller Feb 2, 2024
51d79dc
Add one more check for function identity
jerelmiller Feb 2, 2024
8d4ae0e
Remove unneeded fetchCount in test
jerelmiller Feb 2, 2024
b6652ae
Add clarifying update to test name
jerelmiller Feb 2, 2024
2d42a09
Add note about behavior change to changelog
jerelmiller Feb 2, 2024
031f5c5
Minor tweak to comment
jerelmiller Feb 2, 2024
79c3d8a
Update important info in changeset
jerelmiller Feb 6, 2024
7dced46
Merge remote-tracking branch 'origin/main' into jerel/issue-9448-lazy…
jerelmiller Feb 6, 2024
3a51d12
Rename useStableCallback to useEvent
jerelmiller Feb 6, 2024
8a45baa
Make T the function itself in useEvent
jerelmiller Feb 6, 2024
1689136
Revert change to ternary clause
jerelmiller Feb 6, 2024
11ac51f
Use useEvent for execute function callback
jerelmiller Feb 6, 2024
ce512cf
Update size limits
jerelmiller Feb 6, 2024
0495f52
Remove unneeded note on changeset
jerelmiller Feb 6, 2024
7474100
Remove outdated comment
jerelmiller Feb 6, 2024
263c5ca
Restore previous behavior in useLazyQuery but set optionsRef.current …
jerelmiller Feb 7, 2024
24aa08c
Remove useEvent
jerelmiller Feb 7, 2024
5d00c58
Restore comment
jerelmiller Feb 7, 2024
73298b7
Restore old style to prevent diff noise
jerelmiller Feb 7, 2024
95ab853
Merge branch 'main' into jerel/issue-9448-lazy-query-double-call
jerelmiller Feb 7, 2024
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/honest-turtles-move.md
Original file line number Diff line number Diff line change
@@ -0,0 +1,5 @@
---
"@apollo/client": patch
---

Fix issue in `useLazyQuery` that results in a double network call when calling the execute function with no arguments after having called it previously with another set of arguments.
6 changes: 6 additions & 0 deletions docs/source/data/queries.mdx
Original file line number Diff line number Diff line change
Expand Up @@ -269,6 +269,12 @@ The first item in `useLazyQuery`'s return tuple is the query function, and the s

As shown above, you can pass options to the query function just like you pass them to `useLazyQuery` itself. If you pass a particular option to _both_, the value you pass to the query function takes precedence. This is a handy way to pass _default_ options to `useLazyQuery` and then customize those options in the query function.

<Note>

Variables are merged by taking the `variables` passed as options to the hook and merging them with the `variables` passed to the query function. If you do not pass `variables` to the query function, only the `variables` passed to the hook are used in the query execution.

</Note>

For a full list of supported options, see the [API reference](../api/react/hooks/#uselazyquery).

## Setting a fetch policy
Expand Down
270 changes: 270 additions & 0 deletions src/react/hooks/__tests__/useLazyQuery.test.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -5,6 +5,7 @@ import { act, render, renderHook, waitFor } from "@testing-library/react";

import {
ApolloClient,
ApolloError,
ApolloLink,
ErrorPolicy,
InMemoryCache,
Expand All @@ -19,6 +20,7 @@ import {
wait,
tick,
MockSubscriptionLink,
MockLink,
} from "../../../testing";
import { useLazyQuery } from "../useLazyQuery";
import { QueryResult } from "../../types/types";
Expand Down Expand Up @@ -1483,6 +1485,274 @@ describe("useLazyQuery Hook", () => {
expect(fetchCount).toBe(1);
});

// https://github.com/apollographql/apollo-client/issues/9448
it.each(["network-only", "no-cache", "cache-and-network"] as const)(
"does not issue multiple network calls when calling execute again without variables with a %s fetch policy",
async (fetchPolicy) => {
interface Data {
user: { id: string; name: string };
}

interface Variables {
id?: string;
}

const query: TypedDocumentNode<Data, Variables> = gql`
query UserQuery($id: ID) {
user(id: $id) {
id
name
}
}
`;

let fetchCount = 0;

const link = new ApolloLink((operation) => {
fetchCount++;
return new Observable((observer) => {
const { id } = operation.variables;

setTimeout(() => {
observer.next({
data: {
user:
id ?
{ id, name: "John Doe" }
: { id: null, name: "John Default" },
},
});
observer.complete();
}, 20);
});
});

const client = new ApolloClient({
link,
cache: new InMemoryCache(),
});

const { result } = renderHook(
() => useLazyQuery(query, { fetchPolicy }),
{
wrapper: ({ children }) => (
<ApolloProvider client={client}>{children}</ApolloProvider>
),
}
);

await act(() => result.current[0]({ variables: { id: "2" } }));

expect(fetchCount).toBe(1);

await waitFor(() => {
expect(result.current[1].data).toEqual({
user: { id: "2", name: "John Doe" },
});
});

expect(fetchCount).toBe(1);

await act(() => result.current[0]());

await waitFor(() => {
expect(result.current[1].data).toEqual({
user: { id: null, name: "John Default" },
});
});

expect(fetchCount).toBe(2);
}
);

it("maintains stable execute function when passing in dynamic function options", async () => {
interface Data {
user: { id: string; name: string };
}

interface Variables {
id: string;
}

const query: TypedDocumentNode<Data, Variables> = gql`
query UserQuery($id: ID!) {
user(id: $id) {
id
name
}
}
`;

const link = new MockLink([
{
request: { query, variables: { id: "1" } },
result: { data: { user: { id: "1", name: "John Doe" } } },
delay: 20,
},
{
request: { query, variables: { id: "2" } },
result: { errors: [new GraphQLError("Oops")] },
delay: 20,
},
{
request: { query, variables: { id: "3" } },
result: { data: { user: { id: "3", name: "Johnny Three" } } },
delay: 20,
maxUsageCount: Number.POSITIVE_INFINITY,
},
]);

const client = new ApolloClient({ link, cache: new InMemoryCache() });

let countRef = { current: 0 };

const trackClosureValue = jest.fn();

const { result, rerender } = renderHook(
() => {
let count = countRef.current;

return useLazyQuery(query, {
fetchPolicy: "cache-first",
variables: { id: "1" },
onCompleted: () => {
trackClosureValue("onCompleted", count);
},
onError: () => {
trackClosureValue("onError", count);
},
skipPollAttempt: () => {
trackClosureValue("skipPollAttempt", count);
return false;
},
nextFetchPolicy: (currentFetchPolicy) => {
trackClosureValue("nextFetchPolicy", count);
return currentFetchPolicy;
},
});
},
{
wrapper: ({ children }) => (
<ApolloProvider client={client}>{children}</ApolloProvider>
),
}
);

const [originalExecute] = result.current;

countRef.current++;
rerender();

expect(result.current[0]).toBe(originalExecute);

// Check for stale closures with onCompleted
await act(() => result.current[0]());
await waitFor(() => {
expect(result.current[1].data).toEqual({
user: { id: "1", name: "John Doe" },
});
});

// after fetch
expect(trackClosureValue).toHaveBeenNthCalledWith(1, "nextFetchPolicy", 1);
expect(trackClosureValue).toHaveBeenNthCalledWith(2, "onCompleted", 1);
trackClosureValue.mockClear();

countRef.current++;
rerender();

expect(result.current[0]).toBe(originalExecute);

// Check for stale closures with onError
await act(() => result.current[0]({ variables: { id: "2" } }));
await waitFor(() => {
expect(result.current[1].error).toEqual(
new ApolloError({ graphQLErrors: [new GraphQLError("Oops")] })
);
});

// variables changed
expect(trackClosureValue).toHaveBeenNthCalledWith(1, "nextFetchPolicy", 2);
// after fetch
expect(trackClosureValue).toHaveBeenNthCalledWith(2, "nextFetchPolicy", 2);
expect(trackClosureValue).toHaveBeenNthCalledWith(3, "onError", 2);
trackClosureValue.mockClear();

countRef.current++;
rerender();

expect(result.current[0]).toBe(originalExecute);

await act(() => result.current[0]({ variables: { id: "3" } }));
await waitFor(() => {
expect(result.current[1].data).toEqual({
user: { id: "3", name: "Johnny Three" },
});
});

// variables changed
expect(trackClosureValue).toHaveBeenNthCalledWith(1, "nextFetchPolicy", 3);
// after fetch
expect(trackClosureValue).toHaveBeenNthCalledWith(2, "nextFetchPolicy", 3);
expect(trackClosureValue).toHaveBeenNthCalledWith(3, "onCompleted", 3);
trackClosureValue.mockClear();

// Test for stale closures for skipPollAttempt
result.current[1].startPolling(20);
await wait(50);
result.current[1].stopPolling();

expect(trackClosureValue).toHaveBeenCalledWith("skipPollAttempt", 3);
});

it("maintains stable execute function identity when changing non-callback options", async () => {
interface Data {
user: { id: string; name: string };
}

interface Variables {
id: string;
}

const query: TypedDocumentNode<Data, Variables> = gql`
query UserQuery($id: ID!) {
user(id: $id) {
id
name
}
}
`;

const link = new ApolloLink((operation) => {
return new Observable((observer) => {
setTimeout(() => {
observer.next({
data: { user: { id: operation.variables.id, name: "John Doe" } },
});
observer.complete();
}, 20);
});
});

const client = new ApolloClient({ link, cache: new InMemoryCache() });

const { result, rerender } = renderHook(
({ id }) => useLazyQuery(query, { variables: { id } }),
{
initialProps: { id: "1" },
wrapper: ({ children }) => (
<ApolloProvider client={client}>{children}</ApolloProvider>
),
}
);

const [execute] = result.current;

rerender({ id: "2" });

expect(result.current[0]).toBe(execute);
});

describe("network errors", () => {
async function check(errorPolicy: ErrorPolicy) {
const networkError = new Error("from the network");
Expand Down
2 changes: 1 addition & 1 deletion src/react/hooks/useLazyQuery.ts
Original file line number Diff line number Diff line change
Expand Up @@ -78,7 +78,7 @@ export function useLazyQuery<

// Use refs to track options and the used query to ensure the `execute`
// function remains referentially stable between renders.
optionsRef.current = merged;
optionsRef.current = options;
queryRef.current = document;

const internalState = useInternalState<TData, TVariables>(
Expand Down
Loading