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 multiple fetches might report data if result contained errors #11984

Merged
merged 6 commits into from
Aug 5, 2024
Merged
Show file tree
Hide file tree
Changes from 4 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/fluffy-impalas-cross.md
Original file line number Diff line number Diff line change
@@ -0,0 +1,5 @@
---
"@apollo/client": patch
---

Fix an issue where multiple fetches with results that returned errors would sometimes set the `data` property with an `errorPolicy` of `none`.
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": 40168,
"import { ApolloClient, InMemoryCache, HttpLink } from \"dist/index.js\" (production)": 32974
"dist/apollo-client.min.cjs": 40181,
"import { ApolloClient, InMemoryCache, HttpLink } from \"dist/index.js\" (production)": 32985
}
13 changes: 11 additions & 2 deletions src/core/QueryManager.ts
Original file line number Diff line number Diff line change
Expand Up @@ -1176,11 +1176,12 @@ export class QueryManager<TStore> {
(result) => {
const graphQLErrors = getGraphQLErrorsFromResult(result);
const hasErrors = graphQLErrors.length > 0;
const { errorPolicy } = options;

// If we interrupted this request by calling getResultsFromLink again
// with the same QueryInfo object, we ignore the old results.
if (requestId >= queryInfo.lastRequestId) {
if (hasErrors && options.errorPolicy === "none") {
if (hasErrors && errorPolicy === "none") {
// Throwing here effectively calls observer.error.
throw queryInfo.markError(
new ApolloError({
Expand All @@ -1206,7 +1207,15 @@ export class QueryManager<TStore> {
networkStatus: NetworkStatus.ready,
};

if (hasErrors && options.errorPolicy !== "ignore") {
// In the case we start multiple network requests simulatenously, we
// want to ensure we properly set `data` if we're reporting on an old
// result which will not be caught by the conditional above that ends up
// throwing the markError result.
if (hasErrors && errorPolicy === "none") {
aqr.data = void 0 as TData;
}

if (hasErrors && errorPolicy !== "ignore") {
aqr.errors = graphQLErrors;
aqr.networkStatus = NetworkStatus.error;
}
Expand Down
81 changes: 80 additions & 1 deletion src/react/hooks/__tests__/useQuery.test.tsx
Original file line number Diff line number Diff line change
@@ -1,5 +1,5 @@
import React, { Fragment, ReactNode, useEffect, useRef, useState } from "react";
import { DocumentNode, GraphQLError } from "graphql";
import { DocumentNode, GraphQLError, GraphQLFormattedError } from "graphql";
import gql from "graphql-tag";
import { act } from "@testing-library/react";
import userEvent from "@testing-library/user-event";
Expand Down Expand Up @@ -9786,6 +9786,85 @@ describe("useQuery Hook", () => {
}
);
});

// https://github.com/apollographql/apollo-client/issues/11938
it("does not emit `data` on previous fetch when a 2nd fetch is kicked off and the result returns an error when errorPolicy is none", async () => {
const query = gql`
query {
user {
id
name
}
}
`;

const graphQLError: GraphQLFormattedError = { message: "Cannot get name" };

const mocks = [
{
request: { query },
result: {
data: { user: { __typename: "User", id: "1", name: null } },
errors: [graphQLError],
},
delay: 10,
maxUsageCount: Number.POSITIVE_INFINITY,
},
];

const ProfiledHook = profileHook(() =>
useQuery(query, { notifyOnNetworkStatusChange: true })
);

render(<ProfiledHook />, {
wrapper: ({ children }) => (
<MockedProvider mocks={mocks}>{children}</MockedProvider>
),
});

{
const { loading, data, error } = await ProfiledHook.takeSnapshot();

expect(loading).toBe(true);
expect(data).toBeUndefined();
expect(error).toBeUndefined();
}

{
const { loading, data, error } = await ProfiledHook.takeSnapshot();

expect(loading).toBe(false);
expect(data).toBeUndefined();
expect(error).toEqual(new ApolloError({ graphQLErrors: [graphQLError] }));
}

const { refetch } = ProfiledHook.getCurrentSnapshot();

refetch().catch(() => {});
refetch().catch(() => {});

{
const { loading, networkStatus, data, error } =
await ProfiledHook.takeSnapshot();

expect(loading).toBe(true);
expect(data).toBeUndefined();
expect(networkStatus).toBe(NetworkStatus.refetch);
expect(error).toBeUndefined();
}

{
const { loading, networkStatus, data, error } =
await ProfiledHook.takeSnapshot();

expect(loading).toBe(false);
expect(data).toBeUndefined();
expect(networkStatus).toBe(NetworkStatus.error);
expect(error).toEqual(new ApolloError({ graphQLErrors: [graphQLError] }));
}

await expect(ProfiledHook).not.toRerender({ timeout: 200 });
});
});

describe.skip("Type Tests", () => {
Expand Down
Loading