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 additional re-render when calling fetchMore from useSuspenseQuery when using startTransition. #11638

Merged
merged 14 commits into from
Mar 5, 2024
Merged
5 changes: 5 additions & 0 deletions .changeset/rich-hotels-sniff.md
Original file line number Diff line number Diff line change
@@ -0,0 +1,5 @@
---
"@apollo/client": patch
---

Fix issue where calling `fetchMore` inside a `startTransition` from `useSuspenseQuery` causes an additional rerender.
2 changes: 1 addition & 1 deletion .size-limits.json
Original file line number Diff line number Diff line change
@@ -1,4 +1,4 @@
{
"dist/apollo-client.min.cjs": 39075,
"dist/apollo-client.min.cjs": 39077,
"import { ApolloClient, InMemoryCache, HttpLink } from \"dist/index.js\" (production)": 32584
}
71 changes: 0 additions & 71 deletions src/react/hooks/__tests__/useBackgroundQuery.test.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -4927,25 +4927,6 @@ describe("fetchMore", () => {
expect(renderedComponents).toStrictEqual([App, SuspenseFallback]);
}

// TODO: Determine why we have this extra render here.
// Possibly related: https://github.com/apollographql/apollo-client/issues/11315
{
const { snapshot } = await Profiler.takeRender();

expect(snapshot.result).toEqual({
data: {
letters: [
{ __typename: "Letter", position: 1, letter: "A" },
{ __typename: "Letter", position: 2, letter: "B" },
{ __typename: "Letter", position: 3, letter: "C" },
{ __typename: "Letter", position: 4, letter: "D" },
],
},
error: undefined,
networkStatus: NetworkStatus.ready,
});
}

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

Expand Down Expand Up @@ -5034,25 +5015,6 @@ describe("fetchMore", () => {
expect(renderedComponents).toStrictEqual([App, SuspenseFallback]);
}

// TODO: Determine why we have this extra render here.
// Possibly related: https://github.com/apollographql/apollo-client/issues/11315
{
const { snapshot } = await Profiler.takeRender();

expect(snapshot.result).toEqual({
data: {
letters: [
{ __typename: "Letter", position: 1, letter: "A" },
{ __typename: "Letter", position: 2, letter: "B" },
{ __typename: "Letter", position: 3, letter: "C" },
{ __typename: "Letter", position: 4, letter: "D" },
],
},
error: undefined,
networkStatus: NetworkStatus.ready,
});
}

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

Expand Down Expand Up @@ -5245,39 +5207,6 @@ describe("fetchMore", () => {
});
}

// TODO: Determine why we have this extra render here. This should mimic
// the update in the next render where we see <App /> included in the
// rerendered components.
// Possibly related: https://github.com/apollographql/apollo-client/issues/11315
Copy link
Member Author

Choose a reason for hiding this comment

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

Narrator: It was related

Copy link
Member

Choose a reason for hiding this comment

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

Made me laugh. Also, great we're rid of them now!

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

expect(renderedComponents).toStrictEqual([ReadQueryHook]);
expect(snapshot).toEqual({
isPending: false,
result: {
data: {
todos: [
{
__typename: "Todo",
id: "1",
name: "Clean room",
completed: false,
},
{
__typename: "Todo",
id: "2",
name: "Take out trash",
completed: true,
},
],
},
error: undefined,
networkStatus: NetworkStatus.ready,
},
});
}

{
// Eventually we should see the updated todos content once its done
// suspending.
Expand Down
134 changes: 132 additions & 2 deletions src/react/hooks/__tests__/useSuspenseQuery.test.tsx
Original file line number Diff line number Diff line change
@@ -1,4 +1,4 @@
import React, { Fragment, StrictMode, Suspense } from "react";
import React, { Fragment, StrictMode, Suspense, useTransition } from "react";
import {
act,
screen,
Expand Down Expand Up @@ -51,7 +51,15 @@ import {
RefetchWritePolicy,
WatchQueryFetchPolicy,
} from "../../../core/watchQueryOptions";
import { profile, spyOnConsole } from "../../../testing/internal";
import {
PaginatedCaseData,
PaginatedCaseVariables,
createProfiler,
profile,
setupPaginatedCase,
spyOnConsole,
useTrackRenders,
} from "../../../testing/internal";

type RenderSuspenseHookOptions<Props, TSerializedCache = {}> = Omit<
RenderHookOptions<Props>,
Expand Down Expand Up @@ -9978,6 +9986,128 @@ describe("useSuspenseQuery", () => {
});
});

// https://github.com/apollographql/apollo-client/issues/11315
it("fetchMore does not cause extra render", async () => {
const { query, link } = setupPaginatedCase();

const user = userEvent.setup();
const client = new ApolloClient({
cache: new InMemoryCache({
typePolicies: {
Query: {
fields: {
letters: offsetLimitPagination(),
},
},
},
}),
link,
});

const Profiler = createProfiler({
initialSnapshot: {
result: null as UseSuspenseQueryResult<
PaginatedCaseData,
PaginatedCaseVariables
> | null,
},
});

function SuspenseFallback() {
useTrackRenders();

return <div>Loading...</div>;
}

function App() {
useTrackRenders();
const [isPending, startTransition] = useTransition();
const result = useSuspenseQuery(query, {
variables: { offset: 0, limit: 2 },
});
const { data, fetchMore } = result;

Profiler.mergeSnapshot({ result });

return (
<button
disabled={isPending}
onClick={() =>
startTransition(() => {
fetchMore({
variables: {
offset: data.letters.length,
limit: data.letters.length + 1,
},
});
})
}
>
Fetch next
</button>
);
}

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

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

expect(renderedComponents).toStrictEqual([SuspenseFallback]);
}

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

expect(renderedComponents).toStrictEqual([App]);
expect(snapshot.result?.data).toEqual({
letters: [
{ __typename: "Letter", letter: "A", position: 1 },
{ __typename: "Letter", letter: "B", position: 2 },
],
});
}

await act(() => user.click(screen.getByText("Fetch next")));

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

expect(renderedComponents).toStrictEqual([App]);
expect(snapshot.result?.data).toEqual({
letters: [
{ __typename: "Letter", letter: "A", position: 1 },
{ __typename: "Letter", letter: "B", position: 2 },
],
});
}

Copy link
Member

Choose a reason for hiding this comment

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

Reading the test, it's not immediate apparent why this rerender happens. I'd assume to disable the button - but it might be good in that case to do an expect(button).toBeDisabled to make that obvious.

Copy link
Member Author

Choose a reason for hiding this comment

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

Good call! d9e2257

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

expect(renderedComponents).toStrictEqual([App]);
expect(snapshot.result?.data).toEqual({
letters: [
{ __typename: "Letter", letter: "A", position: 1 },
{ __typename: "Letter", letter: "B", position: 2 },
{ __typename: "Letter", letter: "C", position: 3 },
{ __typename: "Letter", letter: "D", position: 4 },
{ __typename: "Letter", letter: "E", position: 5 },
],
});
}

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

describe.skip("type tests", () => {
it("returns unknown when TData cannot be inferred", () => {
const query = gql`
Expand Down
20 changes: 16 additions & 4 deletions src/react/internal/cache/QueryReference.ts
Original file line number Diff line number Diff line change
Expand Up @@ -379,10 +379,22 @@ export class InternalQueryReference<TData = unknown> {
// promise is resolved correctly.
returnedPromise
.then((result) => {
if (this.promise.status === "pending") {
this.result = result;
this.resolve?.(result);
}
// In the case of `fetchMore`, this promise is resolved before a cache
// result is emitted due to the fact that `fetchMore` sets a `no-cache`
// fetch policy and runs `cache.batch` in its `.then` handler. Because
// the timing is different, we accidentally run this update twice
// causing an additional re-render with the `fetchMore` result by
// itself. By wrapping in `setTimeout`, this should provide a short
// delay to allow the `QueryInfo.notify` handler to run before this the
jerelmiller marked this conversation as resolved.
Show resolved Hide resolved
// promise is checked.
// See https://github.com/apollographql/apollo-client/issues/11315 for
// more information
setTimeout(() => {
if (this.promise.status === "pending") {
this.result = result;
this.resolve?.(result);
}
});
})
.catch(() => {});

Expand Down
4 changes: 2 additions & 2 deletions src/testing/internal/scenarios/index.ts
Original file line number Diff line number Diff line change
Expand Up @@ -80,8 +80,8 @@ export interface PaginatedCaseVariables {
export function setupPaginatedCase() {
const query: TypedDocumentNode<PaginatedCaseData, PaginatedCaseVariables> =
gql`
query letters($limit: Int, $offset: Int) {
letters(limit: $limit) {
query LettersQuery($limit: Int, $offset: Int) {
letters(limit: $limit, offset: $offset) {
Copy link
Member Author

@jerelmiller jerelmiller Mar 4, 2024

Choose a reason for hiding this comment

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

This offset as a variable is important for cache reasons and it was forgotten 😱. Thankfully no tests were harmed in this change, but it was necessary to reproduce the bug in this PR.

letter
position
}
Expand Down
Loading