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 regression that causes partial data to be reported unexpectedly in some circumstances #11579

Merged
merged 33 commits into from
Feb 15, 2024
Merged
Changes from 1 commit
Commits
Show all changes
33 commits
Select commit Hold shift + click to select a range
3b90234
Write failing test for issue 11400
jerelmiller Feb 8, 2024
3019c02
Remove properties from test that would pass with fix that arent there
jerelmiller Feb 8, 2024
3432591
Set data to undefined if its partial and we don't allow partial data
jerelmiller Feb 9, 2024
3c3e8fc
Add comment about partial data
jerelmiller Feb 9, 2024
5b1853d
Update test to check for react version
jerelmiller Feb 9, 2024
5d70cd8
Add test to ensure getCurrentResult doesn't have data loss
jerelmiller Feb 12, 2024
1ca66c6
Remove first proposed fix from ObservableQuery
jerelmiller Feb 12, 2024
1d7629d
Remove part of test that performs refetch
jerelmiller Feb 12, 2024
42fea55
Skip reporting result when cache diff is partial and returnPartailDat…
jerelmiller Feb 12, 2024
22355a6
Add changeset
jerelmiller Feb 12, 2024
ee92d43
Merge branch 'main' into jerel/issue-11400-unexpected-partial-data
jerelmiller Feb 12, 2024
7ffd17e
Minor tweak to comment
jerelmiller Feb 12, 2024
e7fb857
Update size limits
jerelmiller Feb 12, 2024
364ec36
Move check above updateLastDiff
jerelmiller Feb 12, 2024
0e78965
Schedule notification if cache eviction happened
jerelmiller Feb 13, 2024
5cee1db
Remove redundant check
jerelmiller Feb 13, 2024
3c24aa8
Add additional context to comment
jerelmiller Feb 13, 2024
2a624b2
Simplify conditional in test for React 17
jerelmiller Feb 13, 2024
d78f1ff
Add check against getCurrentResult to ensure partial data is not set
jerelmiller Feb 13, 2024
b497ebd
Remove unnecessary setTimeouts wrapped around simulateResult
jerelmiller Feb 13, 2024
ff3c8b1
Merge remote-tracking branch 'origin/main' into jerel/issue-11400-une…
jerelmiller Feb 13, 2024
fcf33f2
Update size limits
jerelmiller Feb 13, 2024
d4f5bf8
Add additional checks in test to show failed situation
jerelmiller Feb 13, 2024
c2afd2a
Account for extra React 17 render
jerelmiller Feb 13, 2024
1e8184b
Conditionally set last diff when we notify
jerelmiller Feb 13, 2024
359a43f
Update size limit
jerelmiller Feb 13, 2024
c9b9317
Only schedule notification if not dirty
jerelmiller Feb 14, 2024
73de9f5
Simplify inner conditional to avoid duplicate logic
jerelmiller Feb 14, 2024
e5033b5
Restore old if condition
jerelmiller Feb 14, 2024
449b6a2
Add delays to remove React 17 conditional
jerelmiller Feb 14, 2024
aa1dbd8
Update size limits
jerelmiller Feb 14, 2024
2896004
Add additional changeset
jerelmiller Feb 14, 2024
1e408bd
Merge branch 'main' into jerel/issue-11400-unexpected-partial-data
jerelmiller Feb 15, 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
Next Next commit
Write failing test for issue 11400
  • Loading branch information
jerelmiller committed Feb 8, 2024
commit 3b90234e8862ca8b131bd02467c79980af712775
201 changes: 200 additions & 1 deletion src/react/hooks/__tests__/useQuery.test.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -26,8 +26,13 @@ import {
import { QueryResult } from "../../types/types";
import { useQuery } from "../useQuery";
import { useMutation } from "../useMutation";
import { profileHook, spyOnConsole } from "../../../testing/internal";
import {
createProfiler,
profileHook,
spyOnConsole,
} from "../../../testing/internal";
import { useApolloClient } from "../useApolloClient";
import { useLazyQuery } from "../useLazyQuery";

describe("useQuery Hook", () => {
describe("General use", () => {
Expand Down Expand Up @@ -4018,6 +4023,200 @@ describe("useQuery Hook", () => {
});
});

// https://github.com/apollographql/apollo-client/issues/11400
it("does not return partial data unexpectedly when one query errors, then another succeeds with overlapping data", async () => {
interface Query1 {
person: {
__typename: "Person";
id: number;
firstName: string;
alwaysFails: boolean;
} | null;
}

interface Query2 {
person: { __typename: "Person"; id: number; lastName: string } | null;
}

interface Variables {
id: number;
}

const user = userEvent.setup();

const query1: TypedDocumentNode<Query1, Variables> = gql`
query PersonQuery1($id: ID!) {
person(id: $id) {
id
firstName
alwaysFails
}
}
`;

const query2: TypedDocumentNode<Query2, Variables> = gql`
query PersonQuery2($id: ID!) {
person(id: $id) {
id
lastName
}
}
`;

const Profiler = createProfiler({
initialSnapshot: {
useQueryResult: null as QueryResult<Query1, Variables> | null,
useLazyQueryResult: null as QueryResult<Query2, Variables> | null,
},
});

const client = new ApolloClient({
link: new MockLink([
{
request: { query: query1, variables: { id: 1 } },
result: {
data: { person: null },
errors: [new GraphQLError("Intentional error")],
},
maxUsageCount: Number.POSITIVE_INFINITY,
},
{
request: { query: query2, variables: { id: 1 } },
result: {
data: { person: { __typename: "Person", id: 1, lastName: "Doe" } },
},
},
]),
cache: new InMemoryCache(),
});

function App() {
const useQueryResult = useQuery(query1, {
variables: { id: 1 },
// This is necessary to reproduce the behavior
notifyOnNetworkStatusChange: true,
});

const [execute, useLazyQueryResult] = useLazyQuery(query2, {
variables: { id: 1 },
});

Profiler.replaceSnapshot({ useQueryResult, useLazyQueryResult });

return <button onClick={() => execute()}>Run 2nd query</button>;
}

render(<App />, {
wrapper: ({ children }) => (
<ApolloProvider client={client}>
<Profiler>{children}</Profiler>
</ApolloProvider>
),
});

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

expect(snapshot.useQueryResult).toMatchObject({
called: true,
data: undefined,
loading: true,
networkStatus: NetworkStatus.loading,
});

expect(snapshot.useLazyQueryResult).toMatchObject({
called: false,
data: undefined,
loading: false,
networkStatus: NetworkStatus.ready,
});
}

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

expect(snapshot.useQueryResult).toMatchObject({
data: undefined,
error: new ApolloError({
graphQLErrors: [new GraphQLError("Intentional error")],
}),
loading: false,
networkStatus: NetworkStatus.error,
});

expect(snapshot.useLazyQueryResult).toMatchObject({
called: false,
data: undefined,
loading: false,
networkStatus: NetworkStatus.ready,
});
}

await act(() => user.click(screen.getByText("Run 2nd query")));

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

expect(snapshot.useQueryResult).toMatchObject({
data: undefined,
error: new ApolloError({
graphQLErrors: [new GraphQLError("Intentional error")],
}),
loading: false,
networkStatus: NetworkStatus.error,
});

expect(snapshot.useLazyQueryResult).toMatchObject({
called: true,
data: { person: { __typename: "Person", id: 1, lastName: "Doe" } },
loading: false,
networkStatus: NetworkStatus.ready,
});
}

// Since we write data to the cache that does not contain all the data to
// fulfill the first query, it will try and fetch again
{
const { snapshot } = await Profiler.takeRender();

expect(snapshot.useQueryResult).toMatchObject({
data: undefined,
error: undefined,
loading: true,
networkStatus: NetworkStatus.loading,
});

expect(snapshot.useLazyQueryResult).toMatchObject({
called: true,
data: { person: { __typename: "Person", id: 1, lastName: "Doe" } },
loading: false,
networkStatus: NetworkStatus.ready,
});
}

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

expect(snapshot.useQueryResult).toMatchObject({
data: undefined,
error: new ApolloError({
graphQLErrors: [new GraphQLError("Intentional error")],
}),
loading: false,
networkStatus: NetworkStatus.error,
});

expect(snapshot.useLazyQueryResult).toMatchObject({
called: true,
data: { person: { __typename: "Person", id: 1, lastName: "Doe" } },
loading: false,
networkStatus: NetworkStatus.ready,
});
}

await expect(Profiler).not.toRerender();
Copy link
Member Author

@jerelmiller jerelmiller Feb 13, 2024

Choose a reason for hiding this comment

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

One minor difference with this fix vs prior behavior is that previously the errored useQuery would be refetched when the partial data result was written to the cache. Now if the query has errored and we get a partial cache update, no network request happens.

I think that's ok as the network request felt a bit surprising, but its also difficult to tell if anyone relies on this "auto refetch" behavior right now.

You can see this particular change in behavior for this test in 1d7629d

Copy link
Member Author

Choose a reason for hiding this comment

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

Should this be mentioned in the changelog?

Copy link
Member

Choose a reason for hiding this comment

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

Yes, I think it should be mentioned in the changelog.
Maybe add a second changeset to the PR and also classify it as a bug-fix/patch. It's probably better to have as a separate point.

Copy link
Member Author

Choose a reason for hiding this comment

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

});

describe("Refetching", () => {
it("refetching with different variables", async () => {
const query = gql`
Expand Down
Loading