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

useSubscription: fix rules of React violations #11863

Merged
merged 14 commits into from
Jul 3, 2024
Prev Previous commit
Next Next commit
no more need for stable options, performance optimization
phryneas committed May 21, 2024
commit 4aa14cfe643356802f01b897130f3cd589c57e3a
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": 39585,
"dist/apollo-client.min.cjs": 39573,
"import { ApolloClient, InMemoryCache, HttpLink } from \"dist/index.js\" (production)": 32821
}
22 changes: 8 additions & 14 deletions src/react/hooks/useSubscription.ts
Original file line number Diff line number Diff line change
@@ -22,8 +22,6 @@ import { useApolloClient } from "./useApolloClient.js";
import { useDeepMemo } from "./internal/useDeepMemo.js";
import { useSyncExternalStore } from "./useSyncExternalStore.js";

const emptyObject = Object.freeze({});

/**
* > Refer to the [Subscriptions](https://www.apollographql.com/docs/react/data/subscriptions/) section for a more in-depth overview of `useSubscription`.
*
@@ -114,10 +112,7 @@ export function useSubscription<
TVariables extends OperationVariables = OperationVariables,
>(
subscription: DocumentNode | TypedDocumentNode<TData, TVariables>,
options: SubscriptionHookOptions<
NoInfer<TData>,
NoInfer<TVariables>
> = emptyObject
options: SubscriptionHookOptions<NoInfer<TData>, NoInfer<TVariables>> = {}
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
options: SubscriptionHookOptions<NoInfer<TData>, NoInfer<TVariables>> = {}
options: SubscriptionHookOptions<NoInfer<TData>, NoInfer<TVariables>> = Object.create(null)

I believe we've been using this version of an empty object throughout the project.

Copy link
Member Author

Choose a reason for hiding this comment

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

Fair enough.

) {
const hasIssuedDeprecationWarningRef = React.useRef(false);
const client = useApolloClient(options.client);
@@ -145,9 +140,6 @@ export function useSubscription<

let { skip, fetchPolicy, variables, shouldResubscribe, context } = options;
variables = useDeepMemo(() => variables, [variables]);
if (typeof shouldResubscribe === "function") {
shouldResubscribe = !!shouldResubscribe(options!);
}

let [observable, setObservable] = React.useState(() =>
options.skip ? null : (
@@ -161,11 +153,13 @@ export function useSubscription<
}
} else if (
!observable ||
(shouldResubscribe !== false &&
(client !== observable.__.client ||
subscription !== observable.__.query ||
!equal(variables, observable.__.variables) ||
fetchPolicy !== observable.__.fetchPolicy))
((client !== observable.__.client ||
subscription !== observable.__.query ||
fetchPolicy !== observable.__.fetchPolicy ||
!equal(variables, observable.__.variables)) &&
(typeof shouldResubscribe === "function" ?
!!shouldResubscribe(options!)
Copy link
Member Author

Choose a reason for hiding this comment

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

shouldResubscribe should now only execute if something is actually different - but since we're doing this in render, it will be executed every render if they are different, not only if these values changed.

: shouldResubscribe) !== false)
) {
setObservable(
(observable = createSubscription(