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

Colocate and flatten hook types #12465

Merged
merged 125 commits into from
Mar 24, 2025
Merged

Conversation

jerelmiller
Copy link
Member

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.

@jerelmiller jerelmiller requested a review from phryneas March 20, 2025 04:28
Copy link

changeset-bot bot commented Mar 20, 2025

🦋 Changeset detected

Latest commit: ba20497

The changes in this PR will be included in the next version bump.

This PR includes changesets to release 1 package
Name Type
@apollo/client Major

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

@svc-apollo-docs
Copy link

svc-apollo-docs commented Mar 20, 2025

⚠️ Docs preview not attached to branch

The preview was not built because the PR's base branch release-4.0 is not in the list of sources.

An Apollo team member can comment one of the following commands to dictate which branch to attach the preview to:

  • !docs set-base-branch version-2.6
  • !docs set-base-branch main

Build ID: 1e2d2f6dcbe7ad38943ca404

@jerelmiller jerelmiller changed the title Colocate hook types Colocate and flatten hook types Mar 20, 2025
Copy link

pkg-pr-new bot commented Mar 20, 2025

npm i https://pkg.pr.new/@apollo/client@12465

commit: 20aa031

Copy link
Member Author

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> {
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 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.

Copy link
Member Author

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<
Copy link
Member Author

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.

/** {@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>;
Copy link
Member Author

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.

Base automatically changed from jerel/remove-called-from-usequery to release-4.0 March 20, 2025 14:41
@jerelmiller jerelmiller force-pushed the jerel/inline-react-types branch from ca28fcd to e40bd80 Compare March 20, 2025 18:53
Copy link
Contributor

github-actions bot commented Mar 20, 2025

size-limit report 📦

Path Size
import { ApolloClient, InMemoryCache, HttpLink } from "@apollo/client" (CJS) 41.14 KB (0%)
import { ApolloClient, InMemoryCache, HttpLink } from "@apollo/client" (production) (CJS) 36.66 KB (0%)
import { ApolloClient, InMemoryCache, HttpLink } from "@apollo/client" 31.79 KB (0%)
import { ApolloClient, InMemoryCache, HttpLink } from "@apollo/client" (production) 26.8 KB (0%)
import { ApolloProvider } from "@apollo/client/react" 5.17 KB (0%)
import { ApolloProvider } from "@apollo/client/react" (production) 960 B (0%)
import { useQuery } from "@apollo/client/react" 7.23 KB (0%)
import { useQuery } from "@apollo/client/react" (production) 3 KB (-0.26% 🔽)
import { useLazyQuery } from "@apollo/client/react" 6.38 KB (-0.14% 🔽)
import { useLazyQuery } from "@apollo/client/react" (production) 2.16 KB (-0.14% 🔽)
import { useMutation } from "@apollo/client/react" 6.46 KB (-0.02% 🔽)
import { useMutation } from "@apollo/client/react" (production) 2.23 KB (+0.09% 🔺)
import { useSubscription } from "@apollo/client/react" 6.83 KB (-0.02% 🔽)
import { useSubscription } from "@apollo/client/react" (production) 2.62 KB (-0.12% 🔽)
import { useSuspenseQuery } from "@apollo/client/react" 8.18 KB (+0.02% 🔺)
import { useSuspenseQuery } from "@apollo/client/react" (production) 3.97 KB (0%)
import { useBackgroundQuery } from "@apollo/client/react" 8.02 KB (-0.05% 🔽)
import { useBackgroundQuery } from "@apollo/client/react" (production) 3.81 KB (0%)
import { useLoadableQuery } from "@apollo/client/react" 8.06 KB (-0.21% 🔽)
import { useLoadableQuery } from "@apollo/client/react" (production) 3.87 KB (-0.18% 🔽)
import { useReadQuery } from "@apollo/client/react" 5.82 KB (0%)
import { useReadQuery } from "@apollo/client/react" (production) 1.58 KB (+0.07% 🔺)
import { useFragment } from "@apollo/client/react" 5.86 KB (-0.04% 🔽)
import { useFragment } from "@apollo/client/react" (production) 1.65 KB (+1.26% 🔺)


/* Query types */

export interface BaseQueryOptions<
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 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?

Copy link
Member

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.

Copy link

netlify bot commented Mar 20, 2025

Deploy Preview for apollo-client-docs ready!

Name Link
🔨 Latest commit ba20497
🔍 Latest deploy log https://app.netlify.com/sites/apollo-client-docs/deploys/67e172c9422f0400081d4b8a
😎 Deploy Preview https://deploy-preview-12465--apollo-client-docs.netlify.app
📱 Preview on mobile
Toggle QR Code...

QR Code

Use your smartphone camera to open QR code link.

To edit notification comments on pull requests, go to your Netlify site configuration.

@jerelmiller jerelmiller force-pushed the jerel/inline-react-types branch from 9948c88 to 9fb96f8 Compare March 20, 2025 22:38
@jerelmiller
Copy link
Member Author

@phryneas this one should be ready for rereview again using the namespace implementation that you proposed for all hooks.

@jerelmiller jerelmiller force-pushed the jerel/inline-react-types branch from 6d230fb to 4810ff1 Compare March 21, 2025 00:29
@jerelmiller
Copy link
Member Author

@phryneas something I noticed looking at the build output is that types-only files get a generated .js file with an empty export (export {}). Is this something we can detect in the build and filter out? That might help a touch with bundle size. Looks like this is something happening on the release-4.0 branch as well.

Copy link
Member

@phryneas phryneas left a 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";

Copy link
Member

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?

Copy link
Member Author

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

Copy link
Member Author

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<
Copy link
Member

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.

@github-actions github-actions bot added the auto-cleanup 🤖 label Mar 24, 2025
@jerelmiller jerelmiller merged commit a132163 into release-4.0 Mar 24, 2025
39 checks passed
@jerelmiller jerelmiller deleted the jerel/inline-react-types branch March 24, 2025 15:07
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants