Skip to content

Commit

Permalink
Fix issue where multiple fetches might report data if result contai…
Browse files Browse the repository at this point in the history
…ned errors (#11984)
  • Loading branch information
jerelmiller committed Aug 5, 2024
1 parent c95848e commit 5db1659
Show file tree
Hide file tree
Showing 4 changed files with 98 additions and 5 deletions.
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": 40243,
"import { ApolloClient, InMemoryCache, HttpLink } from \"dist/index.js\" (production)": 33041
"dist/apollo-client.min.cjs": 40252,
"import { ApolloClient, InMemoryCache, HttpLink } from \"dist/index.js\" (production)": 33052
}
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 @@ -10081,6 +10081,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

0 comments on commit 5db1659

Please sign in to comment.