-
Notifications
You must be signed in to change notification settings - Fork 2.7k
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
optimize useQuery
result handling
#11954
Conversation
Coverage shows that this code path was never hit - in the case of a `standby` fetchPolicy, `resultData.current` would already have been reset in `useResubscribeIfNecessary`
While there are three code paths to `toQueryResult`, the other two are guaranteed to never have an `errors` property
instead, we calculate and memoize an "override result" in `useObservableSubscriptionResult`
🦋 Changeset detectedLatest commit: 2456a9a The changes in this PR will be included in the next version bump. This PR includes changesets to release 1 package
Not sure what this means? Click here to learn what changesets are. Click here if you're a maintainer who wants to add another changeset to this PR |
size-limit report 📦
|
✅ Deploy Preview for apollo-client-docs ready!
To edit notification comments on pull requests, go to your Netlify site configuration. |
0eaf524
to
4161379
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Had one minor comment about including a changeset, but the change looks great!
* @deprecated This property will be removed in a future version of Apollo Client. | ||
* Please use `error.graphQLErrors` instead. | ||
*/ | ||
errors?: ReadonlyArray<GraphQLFormattedError>; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can we get a changeset for this change? I think we're fine without one for the refactor changes, but since this is external facing, it would be great to call this out 🙂
src/react/hooks/useQuery.ts
Outdated
interface InternalQueryResult<TData, TVariables extends OperationVariables> | ||
extends Omit< | ||
QueryResult<TData, TVariables>, | ||
Exclude<keyof ObservableQueryFields<TData, TVariables>, "variables"> | ||
> { | ||
[originalResult]: ApolloQueryResult<TData>; | ||
} | ||
> {} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
[nit] Since this is now empty, can we convert this to a type
and assign to it rather than extend
?
type InternalQueryResult<TData, TVariables extends OperationVariables> = Omit<
QueryResult<TData, TVariables>,
Exclude<keyof ObservableQueryFields<TData, TVariables>, "variables">
>
This gets rid of
useHandleSkip
, which mutated results during render, among a few other optimizations. Please review Commit by Commit.