Skip to content

Commit

Permalink
Fix additional re-render when calling fetchMore from `useSuspenseQu…
Browse files Browse the repository at this point in the history
…ery` when using `startTransition`. (#11638)
  • Loading branch information
jerelmiller committed Mar 5, 2024
1 parent f1d8bc4 commit bf93ada
Show file tree
Hide file tree
Showing 6 changed files with 157 additions and 80 deletions.
5 changes: 5 additions & 0 deletions .changeset/rich-hotels-sniff.md
@@ -0,0 +1,5 @@
---
"@apollo/client": patch
---

Fix issue where calling `fetchMore` from a suspense-enabled hook inside `startTransition` caused an unnecessary rerender.
2 changes: 1 addition & 1 deletion .size-limits.json
@@ -1,4 +1,4 @@
{
"dist/apollo-client.min.cjs": 39209,
"dist/apollo-client.min.cjs": 39211,
"import { ApolloClient, InMemoryCache, HttpLink } from \"dist/index.js\" (production)": 32584
}
71 changes: 0 additions & 71 deletions src/react/hooks/__tests__/useBackgroundQuery.test.tsx
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
{
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
135 changes: 133 additions & 2 deletions src/react/hooks/__tests__/useSuspenseQuery.test.tsx
@@ -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,129 @@ 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(screen.getByText("Fetch next")).toBeDisabled();
expect(snapshot.result?.data).toEqual({
letters: [
{ __typename: "Letter", letter: "A", position: 1 },
{ __typename: "Letter", letter: "B", position: 2 },
],
});
}

{
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
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
// 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
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) {
letter
position
}
Expand Down

0 comments on commit bf93ada

Please sign in to comment.