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 issue where a network request is made when using skip/skipToken with useSuspenseQuery in strict mode #11769

Merged
merged 7 commits into from
Apr 10, 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/thick-buttons-juggle.md
Original file line number Diff line number Diff line change
@@ -0,0 +1,5 @@
---
"@apollo/client": patch
---

Fix an issue where using `skipToken` or the `skip` option with `useSuspenseQuery` in React's strict mode would perform a network request.
4 changes: 2 additions & 2 deletions .size-limits.json
Original file line number Diff line number Diff line change
@@ -1,4 +1,4 @@
{
"dist/apollo-client.min.cjs": 39368,
"import { ApolloClient, InMemoryCache, HttpLink } from \"dist/index.js\" (production)": 32634
"dist/apollo-client.min.cjs": 39373,
"import { ApolloClient, InMemoryCache, HttpLink } from \"dist/index.js\" (production)": 32636
}
186 changes: 186 additions & 0 deletions src/react/hooks/__tests__/useBackgroundQuery.test.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -2100,6 +2100,192 @@ it("does not make network requests when `skipToken` is used", async () => {
}
});

it("does not make network requests when `skipToken` is used in strict mode", async () => {
const { query, mocks } = setupSimpleCase();
const Profiler = createDefaultProfiler<SimpleCaseData>();
const { SuspenseFallback, ReadQueryHook } =
createDefaultTrackedComponents(Profiler);
const user = userEvent.setup();

let fetchCount = 0;

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

const mock = mocks.find(({ request }) =>
equal(request.query, operation.query)
);

if (!mock) {
throw new Error("Could not find mock for operation");
}

observer.next((mock as any).result);
observer.complete();
});
});

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

function App() {
useTrackRenders();
const [skip, setSkip] = React.useState(true);
const [queryRef] = useBackgroundQuery(query, skip ? skipToken : undefined);

return (
<>
<button onClick={() => setSkip((skip) => !skip)}>Toggle skip</button>
<Suspense fallback={<SuspenseFallback />}>
{queryRef && <ReadQueryHook queryRef={queryRef} />}
</Suspense>
</>
);
}

renderWithClient(<App />, {
client,
wrapper: ({ children }) => (
<React.StrictMode>
<Profiler>{children}</Profiler>
</React.StrictMode>
),
});

// initial skipped result
await Profiler.takeRender();
expect(fetchCount).toBe(0);

// Toggle skip to `false`
await act(() => user.click(screen.getByText("Toggle skip")));
await Profiler.takeRender();

{
const { snapshot } = await Profiler.takeRender();

expect(snapshot.result).toEqual({
data: { greeting: "Hello" },
error: undefined,
networkStatus: NetworkStatus.ready,
});
}

expect(fetchCount).toBe(1);

// Toggle skip to `true`
await act(() => user.click(screen.getByText("Toggle skip")));

{
const { snapshot } = await Profiler.takeRender();

expect(snapshot.result).toEqual({
data: { greeting: "Hello" },
error: undefined,
networkStatus: NetworkStatus.ready,
});
}

expect(fetchCount).toBe(1);

await expect(Profiler).not.toRerender();
});

it("does not make network requests when using `skip` option in strict mode", async () => {
const { query, mocks } = setupSimpleCase();
const Profiler = createDefaultProfiler<SimpleCaseData>();
const { SuspenseFallback, ReadQueryHook } =
createDefaultTrackedComponents(Profiler);
const user = userEvent.setup();

let fetchCount = 0;

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

const mock = mocks.find(({ request }) =>
equal(request.query, operation.query)
);

if (!mock) {
throw new Error("Could not find mock for operation");
}

observer.next((mock as any).result);
observer.complete();
});
});

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

function App() {
useTrackRenders();
const [skip, setSkip] = React.useState(true);
const [queryRef] = useBackgroundQuery(query, { skip });

return (
<>
<button onClick={() => setSkip((skip) => !skip)}>Toggle skip</button>
<Suspense fallback={<SuspenseFallback />}>
{queryRef && <ReadQueryHook queryRef={queryRef} />}
</Suspense>
</>
);
}

renderWithClient(<App />, {
client,
wrapper: ({ children }) => (
<React.StrictMode>
<Profiler>{children}</Profiler>
</React.StrictMode>
),
});

// initial skipped result
await Profiler.takeRender();
expect(fetchCount).toBe(0);

// Toggle skip to `false`
await act(() => user.click(screen.getByText("Toggle skip")));
await Profiler.takeRender();

{
const { snapshot } = await Profiler.takeRender();

expect(snapshot.result).toEqual({
data: { greeting: "Hello" },
error: undefined,
networkStatus: NetworkStatus.ready,
});
}

expect(fetchCount).toBe(1);

// Toggle skip to `true`
await act(() => user.click(screen.getByText("Toggle skip")));

{
const { snapshot } = await Profiler.takeRender();

expect(snapshot.result).toEqual({
data: { greeting: "Hello" },
error: undefined,
networkStatus: NetworkStatus.ready,
});
}

expect(fetchCount).toBe(1);

await expect(Profiler).not.toRerender();
});

it("result is referentially stable", async () => {
const { query, mocks } = setupSimpleCase();

Expand Down
94 changes: 94 additions & 0 deletions src/react/hooks/__tests__/useSuspenseQuery.test.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -5560,6 +5560,100 @@ describe("useSuspenseQuery", () => {
expect(fetchCount).toBe(1);
});

// https://github.com/apollographql/apollo-client/issues/11768
it("does not make network requests when using `skipToken` with strict mode", async () => {
const { query, mocks } = useVariablesQueryCase();

let fetchCount = 0;

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

const mock = mocks.find(({ request }) =>
equal(request.variables, operation.variables)
);

if (!mock) {
throw new Error("Could not find mock for operation");
}

observer.next(mock.result);
observer.complete();
});
});

const { result, rerender } = renderSuspenseHook(
({ skip, id }) =>
useSuspenseQuery(query, skip ? skipToken : { variables: { id } }),
{ mocks, link, strictMode: true, initialProps: { skip: true, id: "1" } }
);

expect(fetchCount).toBe(0);

rerender({ skip: false, id: "1" });

expect(fetchCount).toBe(1);

await waitFor(() => {
expect(result.current).toMatchObject({
...mocks[0].result,
networkStatus: NetworkStatus.ready,
error: undefined,
});
});

rerender({ skip: true, id: "2" });

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

it("does not make network requests when using `skip` with strict mode", async () => {
const { query, mocks } = useVariablesQueryCase();

let fetchCount = 0;

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

const mock = mocks.find(({ request }) =>
equal(request.variables, operation.variables)
);

if (!mock) {
throw new Error("Could not find mock for operation");
}

observer.next(mock.result);
observer.complete();
});
});

const { result, rerender } = renderSuspenseHook(
({ skip, id }) => useSuspenseQuery(query, { skip, variables: { id } }),
{ mocks, link, strictMode: true, initialProps: { skip: true, id: "1" } }
);

expect(fetchCount).toBe(0);

rerender({ skip: false, id: "1" });

expect(fetchCount).toBe(1);

await waitFor(() => {
expect(result.current).toMatchObject({
...mocks[0].result,
networkStatus: NetworkStatus.ready,
error: undefined,
});
});

rerender({ skip: true, id: "2" });

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

it("`skip` result is referentially stable", async () => {
const { query, mocks } = useSimpleQueryCase();

Expand Down
10 changes: 6 additions & 4 deletions src/react/internal/cache/QueryReference.ts
Original file line number Diff line number Diff line change
Expand Up @@ -207,18 +207,20 @@ export class InternalQueryReference<TData = unknown> {
const { observable } = this;

const originalFetchPolicy = this.watchQueryOptions.fetchPolicy;
const avoidNetworkRequests =
originalFetchPolicy === "no-cache" || originalFetchPolicy === "standby";

try {
if (originalFetchPolicy !== "no-cache") {
if (avoidNetworkRequests) {
Copy link
Member Author

Choose a reason for hiding this comment

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

I swapped the order of these if statements as the positive connotation is always easier for my brain to comprehend when there is an else included

observable.silentSetOptions({ fetchPolicy: "standby" });
} else {
observable.resetLastResults();
observable.silentSetOptions({ fetchPolicy: "cache-first" });
} else {
observable.silentSetOptions({ fetchPolicy: "standby" });
}

this.subscribeToQuery();

if (originalFetchPolicy === "no-cache") {
if (avoidNetworkRequests) {
return;
}

Expand Down
Loading