-
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
Colocate and flatten hook types #12465
Conversation
🦋 Changeset detectedLatest commit: ba20497 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 |
|
commit: |
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.
Will revisit more of the core types in a separate PR as this is focused specifically on the react types.
@@ -90,14 +90,6 @@ export interface QueryOptions< | |||
export interface WatchQueryOptions< | |||
TVariables extends OperationVariables = OperationVariables, | |||
TData = unknown, | |||
> extends SharedWatchQueryOptions<TVariables, 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.
This was the one core change I made since SharedWatchQueryOptions
were only used by the react types and could thus be flatten after those were updated.
src/react/hooks/index.ts
Outdated
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.
We had several cases in the existing types where an internal type was exported publicly since it was shared between several files. I've removed export *
so we explicitly list which types are publicly available rather than the case where we could accidentally introduce one later.
|
||
import { useDeepMemo, wrapHook } from "./internal/index.js"; | ||
import { useApolloClient } from "./useApolloClient.js"; | ||
import { useSyncExternalStore } from "./useSyncExternalStore.js"; | ||
|
||
export interface UseFragmentOptions<TData, TVariables> | ||
extends Omit< |
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.
We were omitting way more fields from these types than this actually brought in. This is where the flattening is nice because its much easier to see exactly which options are supported.
src/react/hooks/useMutation.ts
Outdated
/** {@inheritDoc @apollo/client!MutationOptionsDocumentation#mutation:member} */ | ||
// TODO: Remove this option. We shouldn't allow the mutation to be overridden | ||
// in the mutation function | ||
mutation?: DocumentNode | TypedDocumentNode<TData, TVariables>; |
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.
Noticed this while going over this. I don't think we should allow the mutate
function to accept a different mutation document. In fact, we don't persist this mutation document in any form, so its used for only that execution of the mutation, but this means that we will be returning results for a mutation that doesn't matched the document passed to the hook itself. This could add way too much confusion. I'll remove this in a separate PR since this is focused on the React types, but I wanted to point this out.
ca28fcd
to
e40bd80
Compare
size-limit report 📦
|
|
||
/* Query types */ | ||
|
||
export interface BaseQueryOptions< |
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.
I kept the old hook options/result types but got rid of these base types that were only used for type inheritance. Should we keep these as well or are we ok getting rid of them and just keeping the types previously used by each hook?
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.
Tbh., those should never have been used in userland, so I'm fine with removing them.
✅ Deploy Preview for apollo-client-docs ready!
To edit notification comments on pull requests, go to your Netlify site configuration. |
9948c88
to
9fb96f8
Compare
@phryneas this one should be ready for rereview again using the namespace implementation that you proposed for all hooks. |
6d230fb
to
4810ff1
Compare
@phryneas something I noticed looking at the build output is that types-only files get a generated |
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.
Approved!
Granted, this is almost impossible to review prop-by-prop without repeating the full PR from zero, but it seems everything has moved over, and the fact that none of our tests needed any adjustments beyond lower-casing the first character and inserting a dot, I'm pretty confident these changes are correct.
useSuspenseFragment, | ||
useSuspenseQuery, | ||
} from "@apollo/client/react/hooks"; | ||
|
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.
LoadableQueryHookFetchPolicy
, FetchMoreFunction
and RefetchFunction
have disappeared - do we want to add these to this file, too?
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.
FetchMoreFunction
and RefetchFunction
were always meant to be internal, so I'd rather leave those out. And good catch about LoadbleQueryHookFetchPolicy
. For some reason I exported the first attempt new names instead of the current names. Fixed in 20aa031
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.
Oh I should mention that FetchMoreFunction
and RefetchFunction
were exported from useSuspenseQuery
previously, but we never exported those types in our index file. The only way you could get them was importing from useSuspenseQuery
directly, so I'm fine leaving these out.
|
||
/* Query types */ | ||
|
||
export interface BaseQueryOptions< |
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.
Tbh., those should never have been used in userland, so I'm fine with removing them.
Co-authored-by: Lenz Weber-Tronic <[email protected]>
Moves hook types from the shared
react/types/types.ts
file and colocates them with the hooks themselves. The types have also been renamed to ensure a consistent naming scheme. Hook types are now prefixed with the hook name.