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

Avoid excessive cache.watch / unsubscribe on every render in useFragment #11464

Merged
merged 17 commits into from
Jan 9, 2024
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
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
5 changes: 5 additions & 0 deletions .changeset/great-badgers-jog.md
Original file line number Diff line number Diff line change
@@ -0,0 +1,5 @@
---
'@apollo/client': patch
---

Prevent `useFragment` from excessively unsubscribing and resubscribing the fragment with the cache on every render.
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": 37901,
"dist/apollo-client.min.cjs": 37932,
"import { ApolloClient, InMemoryCache, HttpLink } from \"dist/index.js\" (production)": 31976
}
6 changes: 0 additions & 6 deletions src/react/hooks/__tests__/useFragment.test.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -1513,12 +1513,6 @@ describe("has the same timing as `useQuery`", () => {

cache.writeQuery({ query, data: { item: updatedItem } });

if (React.version.startsWith("17.")) {
const { snapshot } = await ProfiledComponent.takeRender();
expect(snapshot.queryData).toStrictEqual({ item: initialItem });
expect(snapshot.fragmentData).toStrictEqual(updatedItem);
}

{
const { snapshot } = await ProfiledComponent.takeRender();
expect(snapshot.queryData).toStrictEqual({ item: updatedItem });
Expand Down
1 change: 1 addition & 0 deletions src/react/hooks/internal/index.ts
Original file line number Diff line number Diff line change
@@ -1,4 +1,5 @@
// These hooks are used internally and are not exported publicly by the library
export { useDeepMemo } from "./useDeepMemo.js";
export { useIsomorphicLayoutEffect } from "./useIsomorphicLayoutEffect.js";
export { useLazyRef } from "./useLazyRef.js";
export { __use } from "./__use.js";
13 changes: 13 additions & 0 deletions src/react/hooks/internal/useLazyRef.ts
Original file line number Diff line number Diff line change
@@ -0,0 +1,13 @@
import * as React from "react";

const INIT = {};

export function useLazyRef<T>(getInitialValue: () => T) {
const ref = React.useRef<T>(INIT as unknown as T);
Copy link
Member Author

@jerelmiller jerelmiller Jan 5, 2024

Choose a reason for hiding this comment

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

Comparing against this INIT sentinel here instead of something like undefined makes sure that we can still set undefined as a value on subsequent renders without re-running the init function.


if (ref.current === INIT) {
ref.current = getInitialValue();
}

return ref;
}
82 changes: 46 additions & 36 deletions src/react/hooks/useFragment.ts
Original file line number Diff line number Diff line change
Expand Up @@ -14,6 +14,7 @@ import { useApolloClient } from "./useApolloClient.js";
import { useSyncExternalStore } from "./useSyncExternalStore.js";
import type { OperationVariables } from "../../core/index.js";
import type { NoInfer } from "../types/types.js";
import { useDeepMemo, useLazyRef } from "./internal/index.js";

export interface UseFragmentOptions<TData, TVars>
extends Omit<
Expand Down Expand Up @@ -46,48 +47,57 @@ export function useFragment<TData = any, TVars = OperationVariables>(
): UseFragmentResult<TData> {
const { cache } = useApolloClient();

const { fragment, fragmentName, from, optimistic = true, ...rest } = options;
const diffOptions = useDeepMemo<Cache.DiffOptions<TData, TVars>>(() => {
const {
fragment,
fragmentName,
from,
optimistic = true,
...rest
} = options;

const diffOptions: Cache.DiffOptions<TData, TVars> = {
...rest,
returnPartialData: true,
id: typeof from === "string" ? from : cache.identify(from),
query: cache["getFragmentDoc"](fragment, fragmentName),
optimistic,
};
return {
...rest,
returnPartialData: true,
id: typeof from === "string" ? from : cache.identify(from),
query: cache["getFragmentDoc"](fragment, fragmentName),
optimistic,
};
}, [options]);

const resultRef = React.useRef<UseFragmentResult<TData>>();
let latestDiff = cache.diff<TData>(diffOptions);
const resultRef = useLazyRef<UseFragmentResult<TData>>(() =>
Copy link
Member Author

Choose a reason for hiding this comment

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

Introducing this useLazyRef avoids the need to call cache.diff on every render which should give a small perf benefit.

Copy link
Member

Choose a reason for hiding this comment

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

Did you check if we can utilize this in other places in our codebase?

Copy link
Member Author

Choose a reason for hiding this comment

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

I just did a quick glance through some of our other hooks. There is only one other place right now that could potentially utilize it, which is the useInternalState hook in useQuery that initializes the InternalState instance (would still need some of that if statement though, so not sure what the bundle size diff would be)

diffToResult(cache.diff<TData>(diffOptions))
);

// Used for both getSnapshot and getServerSnapshot
const getSnapshot = () => {
const latestDiffToResult = diffToResult(latestDiff);
return (
resultRef.current &&
equal(resultRef.current.data, latestDiffToResult.data)
) ?
resultRef.current
: (resultRef.current = latestDiffToResult);
};
const getSnapshot = React.useCallback(() => resultRef.current, []);
Copy link
Member Author

Choose a reason for hiding this comment

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

This prevents React from doing extra work when the getSnapshot function isn't stable across renders.


return useSyncExternalStore(
(forceUpdate) => {
let lastTimeout = 0;
const unsubcribe = cache.watch({
...diffOptions,
immediate: true,
callback(diff) {
if (!equal(diff, latestDiff)) {
resultRef.current = diffToResult((latestDiff = diff));
lastTimeout = setTimeout(forceUpdate) as any;
}
},
});
return () => {
unsubcribe();
clearTimeout(lastTimeout);
};
},
React.useCallback(
Copy link
Member Author

Choose a reason for hiding this comment

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

Oddly, this change seems to now throw off the timing of the update between useQuery and useFragment when used in the same component.

Copy link
Member Author

Choose a reason for hiding this comment

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

Turns out we had tried to re-render a 2nd time in useFragment before the first had finished, so a lingering setTimeout had been queued before our cache.writeQuery in the test. Because of this, useFragment would re-render before useQuery and threw of the timing. I was able to fix this in 6c1372c by cancelling the timeout from the previous attempt in case it hadn't yet flushed before queuing another attempt at the render.

Copy link
Member Author

Choose a reason for hiding this comment

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

Turns out this change also makes the timing in React 17 and 18 the same. I was able to remove the additional React version check in the test.

(forceUpdate) => {
let lastTimeout = 0;
const unsubscribe = cache.watch({
...diffOptions,
immediate: true,
callback(diff) {
if (!equal(diff.result, resultRef.current.data)) {
resultRef.current = diffToResult(diff);
// If we get another update before we've re-rendered, bail out of
// the update and try again. This ensures that the relative timing
// between useQuery and useFragment stays roughly the same as
// fixed in https://github.com/apollographql/apollo-client/pull/11083
clearTimeout(lastTimeout);
lastTimeout = setTimeout(forceUpdate) as any;
}
},
});
return () => {
unsubscribe();
clearTimeout(lastTimeout);
};
},
[cache, diffOptions]
),
getSnapshot,
getSnapshot
);
Expand Down
Loading