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

Promise thrown by useSuspenseQuery gets stuck "pending" forever when fetchMore request fails #12103

Closed
qiuzhangxi77 opened this issue Oct 25, 2024 · 6 comments · Fixed by #12110
Labels
🐞 bug 🧞 fixed-by-contributor issues where the contributor went the extra mile and fixed the problem with a PR

Comments

@qiuzhangxi77
Copy link

qiuzhangxi77 commented Oct 25, 2024

Issue Description

ErrorBoundary cannot catch the error throw by the useSuspenseQuery, since the promise throw by useSuspenseQuery is always pending when fetchMore request promise was rejected caused by some errors.

This is what I found when viewing the implementation.

  1. fetchMore will setPromise with this.promise in QueryReference.ts. it is different with returnedPromise returned by fetchQuery.
  2. this.promise is created as pending promise in initiateFetch and keep pending during the whole process of fecthMore, except returnedPromise is fulfilled.
  3. returnedPromise handles error capture with the callback catch(() => {})
private initiateFetch(returnedPromise: Promise<ApolloQueryResult<TData>>) {
    this.promise = this.createPendingPromise();
    this.promise.catch(() => {});

    returnedPromise
      .then(() => {
        setTimeout(() => {
          if (this.promise.status === "pending") {
            // See the following for more information:
            // https://github.com/apollographql/apollo-client/issues/11642
            this.result = this.observable.getCurrentResult();
            this.resolve?.(this.result);
          }
        });
      })
      .catch(() => {});

    return returnedPromise;
  }
  1. this.promise is still in pending even though the returnedPromise is rejected.
  2. this.promise is what the suspense waits for in my example.

Therefore, after executing "fetchMore", the promise throw by useSuspenseQuery is always pending.

Link to Reproduction

Reproduction in code sandbox here

Reproduction Steps
easy to reproduce, just make sure the fetchMore request has an error, like network error or variables error...

@apollo/client version

3.8.8

just tested a solution provider by @destin-estrela , which appears to work from.

modified   src/react/internal/cache/QueryReference.ts
@@ -486,7 +486,11 @@ export class InternalQueryReference<TData = unknown> {
          }
        });
      })
-      .catch(() => {});
+      .catch((error) => {
+        if (this.promise.status === "pending") {
+          this.handleError(error);
+        }
@qiuzhangxi77 qiuzhangxi77 changed the title The promise throw by useSuspenseQuery is always pending when fetchMore request promise was rejected caused by some errors Promise thrown by useSuspenseQuery gets stuck "pending" forever when fetchMore request fails Oct 25, 2024
@phryneas phryneas added 🐞 bug 🧞 fixed-by-contributor issues where the contributor went the extra mile and fixed the problem with a PR labels Oct 28, 2024
@phryneas
Copy link
Member

Hi @qiuzhangxi77, thank you for the bug report, reproduction and solution pointer - that's an amazing level of effort!

We're all just back from conferences and part of our team is still out, so we'll need a few days to catch up, but it's really appreciated and we'll take a look as soon as we can!

@qiuzhangxi77
Copy link
Author

Hi @phryneas, thank you for push it in fix progress so quickly!! I also wanted to mention that I had a similar issue with the refetch method not throwing errors correctly from the useSuspenseQuery.

@jerelmiller
Copy link
Member

Hey @qiuzhangxi77 👋

Good catch! I opened #12110 which should fix this. Thanks for the detailed report! Made it easy to fix 🙂

As for refetch, are you able to create a reproduction of that? We have that tested fairly well with all error policies. See these tests for refetch with returned errors and different error policies:

it("throws errors when errors are returned after calling `refetch`", async () => {
using _consoleSpy = spyOnConsole("error");
const query = gql`
query UserQuery($id: String!) {
user(id: $id) {
id
name
}
}
`;
const mocks = [
{
request: { query, variables: { id: "1" } },
result: {
data: { user: { id: "1", name: "Captain Marvel" } },
},
},
{
request: { query, variables: { id: "1" } },
result: {
errors: [new GraphQLError("Something went wrong")],
},
},
];
const { result, renders } = renderSuspenseHook(
() => useSuspenseQuery(query, { variables: { id: "1" } }),
{ mocks }
);
await waitFor(() => {
expect(result.current).toMatchObject({
...mocks[0].result,
networkStatus: NetworkStatus.ready,
error: undefined,
});
});
act(() => {
result.current.refetch();
});
await waitFor(() => {
expect(renders.errorCount).toBe(1);
});
expect(renders.errors).toEqual([
new ApolloError({
graphQLErrors: [new GraphQLError("Something went wrong")],
}),
]);
expect(renders.frames).toMatchObject([
{
...mocks[0].result,
networkStatus: NetworkStatus.ready,
error: undefined,
},
]);
});
it('ignores errors returned after calling `refetch` when errorPolicy is set to "ignore"', async () => {
const query = gql`
query UserQuery($id: String!) {
user(id: $id) {
id
name
}
}
`;
const mocks = [
{
request: { query, variables: { id: "1" } },
result: {
data: { user: { id: "1", name: "Captain Marvel" } },
},
},
{
request: { query, variables: { id: "1" } },
result: {
errors: [new GraphQLError("Something went wrong")],
},
},
];
const { result, renders } = renderSuspenseHook(
() =>
useSuspenseQuery(query, {
errorPolicy: "ignore",
variables: { id: "1" },
}),
{ mocks }
);
await waitFor(() => {
expect(result.current).toMatchObject({
...mocks[0].result,
networkStatus: NetworkStatus.ready,
error: undefined,
});
});
await act(async () => {
await result.current.refetch();
});
expect(renders.errorCount).toBe(0);
expect(renders.errors).toEqual([]);
expect(renders.frames).toMatchObject([
{
...mocks[0].result,
networkStatus: NetworkStatus.ready,
error: undefined,
},
{
...mocks[0].result,
networkStatus: NetworkStatus.ready,
error: undefined,
},
]);
});
it('returns errors after calling `refetch` when errorPolicy is set to "all"', async () => {
const query = gql`
query UserQuery($id: String!) {
user(id: $id) {
id
name
}
}
`;
const mocks = [
{
request: { query, variables: { id: "1" } },
result: {
data: { user: { id: "1", name: "Captain Marvel" } },
},
},
{
request: { query, variables: { id: "1" } },
result: {
errors: [new GraphQLError("Something went wrong")],
},
},
];
const { result, renders } = renderSuspenseHook(
() =>
useSuspenseQuery(query, {
errorPolicy: "all",
variables: { id: "1" },
}),
{ mocks }
);
const expectedError = new ApolloError({
graphQLErrors: [new GraphQLError("Something went wrong")],
});
await waitFor(() => {
expect(result.current).toMatchObject({
...mocks[0].result,
networkStatus: NetworkStatus.ready,
error: undefined,
});
});
act(() => {
result.current.refetch();
});
await waitFor(() => {
expect(result.current).toMatchObject({
...mocks[0].result,
networkStatus: NetworkStatus.error,
error: expectedError,
});
});
expect(renders.errorCount).toBe(0);
expect(renders.errors).toEqual([]);
expect(renders.frames).toMatchObject([
{
...mocks[0].result,
networkStatus: NetworkStatus.ready,
error: undefined,
},
{
...mocks[0].result,
networkStatus: NetworkStatus.error,
error: expectedError,
},
]);
});
it('handles partial data results after calling `refetch` when errorPolicy is set to "all"', async () => {
const query = gql`
query UserQuery($id: String!) {
user(id: $id) {
id
name
}
}
`;
const mocks = [
{
request: { query, variables: { id: "1" } },
result: {
data: { user: { id: "1", name: "Captain Marvel" } },
},
},
{
request: { query, variables: { id: "1" } },
result: {
data: { user: { id: "1", name: null } },
errors: [new GraphQLError("Something went wrong")],
},
},
];
const { result, renders } = renderSuspenseHook(
() =>
useSuspenseQuery(query, {
errorPolicy: "all",
variables: { id: "1" },
}),
{ mocks }
);
const expectedError = new ApolloError({
graphQLErrors: [new GraphQLError("Something went wrong")],
});
await waitFor(() => {
expect(result.current).toMatchObject({
...mocks[0].result,
error: undefined,
});
});
act(() => {
result.current.refetch();
});
await waitFor(() => {
expect(result.current).toMatchObject({
data: mocks[1].result.data,
networkStatus: NetworkStatus.error,
error: expectedError,
});
});
expect(renders.errorCount).toBe(0);
expect(renders.errors).toEqual([]);
expect(renders.frames).toMatchObject([
{
...mocks[0].result,
networkStatus: NetworkStatus.ready,
error: undefined,
},
{
data: mocks[1].result.data,
networkStatus: NetworkStatus.error,
error: expectedError,
},
]);
});

@qiuzhangxi77
Copy link
Author

qiuzhangxi77 commented Nov 7, 2024

Hey @qiuzhangxi77 👋

Good catch! I opened #12110 which should fix this. Thanks for the detailed report! Made it easy to fix 🙂

As for refetch, are you able to create a reproduction of that? We have that tested fairly well with all error policies. See these tests for refetch with returned errors and different error policies:

it("throws errors when errors are returned after calling `refetch`", async () => {
using _consoleSpy = spyOnConsole("error");
const query = gql`
query UserQuery($id: String!) {
user(id: $id) {
id
name
}
}
`;
const mocks = [
{
request: { query, variables: { id: "1" } },
result: {
data: { user: { id: "1", name: "Captain Marvel" } },
},
},
{
request: { query, variables: { id: "1" } },
result: {
errors: [new GraphQLError("Something went wrong")],
},
},
];
const { result, renders } = renderSuspenseHook(
() => useSuspenseQuery(query, { variables: { id: "1" } }),
{ mocks }
);
await waitFor(() => {
expect(result.current).toMatchObject({
...mocks[0].result,
networkStatus: NetworkStatus.ready,
error: undefined,
});
});
act(() => {
result.current.refetch();
});
await waitFor(() => {
expect(renders.errorCount).toBe(1);
});
expect(renders.errors).toEqual([
new ApolloError({
graphQLErrors: [new GraphQLError("Something went wrong")],
}),
]);
expect(renders.frames).toMatchObject([
{
...mocks[0].result,
networkStatus: NetworkStatus.ready,
error: undefined,
},
]);
});
it('ignores errors returned after calling `refetch` when errorPolicy is set to "ignore"', async () => {
const query = gql`
query UserQuery($id: String!) {
user(id: $id) {
id
name
}
}
`;
const mocks = [
{
request: { query, variables: { id: "1" } },
result: {
data: { user: { id: "1", name: "Captain Marvel" } },
},
},
{
request: { query, variables: { id: "1" } },
result: {
errors: [new GraphQLError("Something went wrong")],
},
},
];
const { result, renders } = renderSuspenseHook(
() =>
useSuspenseQuery(query, {
errorPolicy: "ignore",
variables: { id: "1" },
}),
{ mocks }
);
await waitFor(() => {
expect(result.current).toMatchObject({
...mocks[0].result,
networkStatus: NetworkStatus.ready,
error: undefined,
});
});
await act(async () => {
await result.current.refetch();
});
expect(renders.errorCount).toBe(0);
expect(renders.errors).toEqual([]);
expect(renders.frames).toMatchObject([
{
...mocks[0].result,
networkStatus: NetworkStatus.ready,
error: undefined,
},
{
...mocks[0].result,
networkStatus: NetworkStatus.ready,
error: undefined,
},
]);
});
it('returns errors after calling `refetch` when errorPolicy is set to "all"', async () => {
const query = gql`
query UserQuery($id: String!) {
user(id: $id) {
id
name
}
}
`;
const mocks = [
{
request: { query, variables: { id: "1" } },
result: {
data: { user: { id: "1", name: "Captain Marvel" } },
},
},
{
request: { query, variables: { id: "1" } },
result: {
errors: [new GraphQLError("Something went wrong")],
},
},
];
const { result, renders } = renderSuspenseHook(
() =>
useSuspenseQuery(query, {
errorPolicy: "all",
variables: { id: "1" },
}),
{ mocks }
);
const expectedError = new ApolloError({
graphQLErrors: [new GraphQLError("Something went wrong")],
});
await waitFor(() => {
expect(result.current).toMatchObject({
...mocks[0].result,
networkStatus: NetworkStatus.ready,
error: undefined,
});
});
act(() => {
result.current.refetch();
});
await waitFor(() => {
expect(result.current).toMatchObject({
...mocks[0].result,
networkStatus: NetworkStatus.error,
error: expectedError,
});
});
expect(renders.errorCount).toBe(0);
expect(renders.errors).toEqual([]);
expect(renders.frames).toMatchObject([
{
...mocks[0].result,
networkStatus: NetworkStatus.ready,
error: undefined,
},
{
...mocks[0].result,
networkStatus: NetworkStatus.error,
error: expectedError,
},
]);
});
it('handles partial data results after calling `refetch` when errorPolicy is set to "all"', async () => {
const query = gql`
query UserQuery($id: String!) {
user(id: $id) {
id
name
}
}
`;
const mocks = [
{
request: { query, variables: { id: "1" } },
result: {
data: { user: { id: "1", name: "Captain Marvel" } },
},
},
{
request: { query, variables: { id: "1" } },
result: {
data: { user: { id: "1", name: null } },
errors: [new GraphQLError("Something went wrong")],
},
},
];
const { result, renders } = renderSuspenseHook(
() =>
useSuspenseQuery(query, {
errorPolicy: "all",
variables: { id: "1" },
}),
{ mocks }
);
const expectedError = new ApolloError({
graphQLErrors: [new GraphQLError("Something went wrong")],
});
await waitFor(() => {
expect(result.current).toMatchObject({
...mocks[0].result,
error: undefined,
});
});
act(() => {
result.current.refetch();
});
await waitFor(() => {
expect(result.current).toMatchObject({
data: mocks[1].result.data,
networkStatus: NetworkStatus.error,
error: expectedError,
});
});
expect(renders.errorCount).toBe(0);
expect(renders.errors).toEqual([]);
expect(renders.frames).toMatchObject([
{
...mocks[0].result,
networkStatus: NetworkStatus.ready,
error: undefined,
},
{
data: mocks[1].result.data,
networkStatus: NetworkStatus.error,
error: expectedError,
},
]);
});

Hi @jerelmiller , thank you for the quickly fix!! I made a simple example, and it works fine. Maybe I used it incorrectly in my project, which caused the error not to be thrown normally.

Copy link
Contributor

github-actions bot commented Nov 7, 2024

Do you have any feedback for the maintainers? Please tell us by taking a one-minute survey. Your responses will help us understand Apollo Client usage and allow us to serve you better.

@jerelmiller
Copy link
Member

@qiuzhangxi77 glad to hear its working there! I've gone ahead and merged #12110 and will get that released shortly. Feel free to open a new issue if you end up figuring out that Apollo Client has a bug with refetch. Hope you're able to figure that one out! I'd check errorPolicy to see if you're setting to to something other than none because refetch won't throw in those cases.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
🐞 bug 🧞 fixed-by-contributor issues where the contributor went the extra mile and fixed the problem with a PR
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants