refactor useQuery to not use an internal class by phryneas · Pull Request #11869 · apollographql/apollo-client

phryneas · 2024-05-27T16:30:47Z

After #11890, I'm fairly confident in this now.

This needs to be reviewed commit-by commit, although a bunch of merges added additional commits at the start and end - I'm really sorry for those! (I believe some of them will disappear here if #11890 gets merged first!)

There might be more PRs to further streamline the result handling, but I think this PR is big enough as it is right now.

changeset-bot · 2024-05-27T16:30:51Z

🦋 Changeset detected

Latest commit: 33c0fef

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	Minor

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

github-actions · 2024-05-27T16:32:08Z

size-limit report 📦

Path	Size
dist/apollo-client.min.cjs	38.89 KB (+0.52% 🔺)
import { ApolloClient, InMemoryCache, HttpLink } from "dist/main.cjs"	47.63 KB (+0.31% 🔺)
import { ApolloClient, InMemoryCache, HttpLink } from "dist/main.cjs" (production)	45.17 KB (+0.21% 🔺)
import { ApolloClient, InMemoryCache, HttpLink } from "dist/index.js"	34.22 KB (-0.01% 🔽)
import { ApolloClient, InMemoryCache, HttpLink } from "dist/index.js" (production)	32.08 KB (-0.01% 🔽)
import { ApolloProvider } from "dist/react/index.js"	1.26 KB (0%)
import { ApolloProvider } from "dist/react/index.js" (production)	1.24 KB (0%)
import { useQuery } from "dist/react/index.js"	5.2 KB (-2.19% 🔽)
import { useQuery } from "dist/react/index.js" (production)	4.28 KB (-2.67% 🔽)
import { useLazyQuery } from "dist/react/index.js"	5.68 KB (+1.42% 🔺)
import { useLazyQuery } from "dist/react/index.js" (production)	4.75 KB (+1.83% 🔺)
import { useMutation } from "dist/react/index.js"	3.59 KB (+0.03% 🔺)
import { useMutation } from "dist/react/index.js" (production)	2.81 KB (+0.04% 🔺)
import { useSubscription } from "dist/react/index.js"	3.63 KB (+0.03% 🔺)
import { useSubscription } from "dist/react/index.js" (production)	2.78 KB (+0.04% 🔺)
import { useSuspenseQuery } from "dist/react/index.js"	5.47 KB (0%)
import { useSuspenseQuery } from "dist/react/index.js" (production)	4.12 KB (0%)
import { useBackgroundQuery } from "dist/react/index.js"	4.98 KB (-0.02% 🔽)
import { useBackgroundQuery } from "dist/react/index.js" (production)	3.63 KB (0%)
import { useLoadableQuery } from "dist/react/index.js"	5.04 KB (0%)
import { useLoadableQuery } from "dist/react/index.js" (production)	3.69 KB (0%)
import { useReadQuery } from "dist/react/index.js"	3.35 KB (+0.03% 🔺)
import { useReadQuery } from "dist/react/index.js" (production)	3.3 KB (0%)
import { useFragment } from "dist/react/index.js"	2.32 KB (0%)
import { useFragment } from "dist/react/index.js" (production)	2.27 KB (0%)

netlify · 2024-06-21T14:12:38Z

✅ Deploy Preview for apollo-client-docs ready!

Name	Link
🔨 Latest commit	`33c0fef`
🔍 Latest deploy log	https://app.netlify.com/sites/apollo-client-docs/deploys/6686b3fab7f2da00083f95be
😎 Deploy Preview	https://deploy-preview-11869--apollo-client-docs.netlify.app
📱 Preview on mobile	Toggle 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 · 2024-07-03T00:28:30Z

I've done a commit-by-commit look through this PR, but I'd like to wait to fully comment on this until you've had a chance to merge the base branches into this one. I think this contains changes from a couple other PRs, so its difficult to tell whats from that and whats from the other branches.

Once that is done, I should be able to get a quick review submitted. Thanks!

…ions` out

…e.result`

…ptions]`

phryneas · 2024-07-03T09:52:28Z

Did a bunch of cherry-picking and history rewriting - it should be good now!

jerelmiller

I still find a good portion of this fairly awkward but I think that's less a fault of this refactor and more that this refactor really illuminated some of the already tight coupling in a lot of the areas of the hook that were there as a result of the class usage. As much as I'd love to continue refactoring into oblivion, we gotta ship something and I think this is a huge improvement over what we've had.

I've added quite a few suggestions, but many of them are non-blocking so I'll let you use your best judgement on whether it makes sense to change them or not.

jerelmiller · 2024-07-03T16:23:32Z

-
-*/
-        expect(obsQueries.size).toBe(IS_REACT_19 ? 1 : 2);
+        expect(obsQueries.size).toBe(2);


🎉 🎉 🎉

jerelmiller · 2024-07-03T16:38:07Z

@@ -1,19 +1,38 @@
+/**
+ * Function parameters in this file try to follow a common order for the sake of


I'm curious, why this approach vs using an object to pass in values to each of the functions? There are cases such as this one that I would find easier to reason about with the object syntax vs positional args.

getObsQueryOptions( undefined, client, options, makeWatchQueryOptions() ) // => getObsQueryOptions({ client, options, watchQueryOptions: makeWatchQueryOptions() })

I fear these rules will be too easy to break in the future and if we need to add more arguments it will be difficult to what the "right" order would be.

Positional function arguments can be minimized, while option object property names cannot.

In the end, you end up with

getObsQueryOptions( void 0, c, o, makeWatchQueryOptions() )

vs

getObsQueryOptions({ client: c, options: o, watchQueryOptions: makeWatchQueryOptions() })

and the same difference on the declaration side, too:

function getObsQueryOptions(a,b,c,d){}

vs

function getObsQueryOptions({ observable: a, client: b, options: c, watchQueryOptions: d }){}

or probably transpiled to

function getObsQueryOptions(o){ var a=o.observable, b=o.client, c=o.options, d=o.watchQueryOptions }

This adds up, a lot.

With a file like this, which contains a lot of helper hooks, we could probably in the long run even use something like the Clojure compiler that would inline a bunch of the hooks - but only with positional arguments, not with config objects.

I fear these rules will be too easy to break in the future and if we need to add more arguments it will be difficult to what the "right" order would be.

Tbh., I'm fine with that. I wanted to get this in as orderly as possible for now (because during refactoring, it was a mess), but if things evolve, that's okay too :)

jerelmiller · 2024-07-03T21:32:34Z

+          renderPromises.getSSRObservable(makeWatchQueryOptions())) ||
+        client.watchQuery(
+          getObsQueryOptions(


Suggested change

renderPromises.getSSRObservable(makeWatchQueryOptions())) ||

client.watchQuery(

getObsQueryOptions(

renderPromises.getSSRObservable(makeWatchQueryOptions())) ||

client.watchQuery(

getObsQueryOptions(

I know this is a result of keeping much of the code the same, but something I'm noticing here is that both makeWatchQueryOptions and getObsQueryOptions both return WatchQueryOptions. I find it very interesting that we sorta use them both together.

I'm not sure if there is any room for any further refactoring here, but it would be lovely if we could have a single place where we handle the creation of watch query options. I find it a bit difficult to understand when we'd want to use one vs the other.

Feel free to push back on this request, but I figured I'd note it as something that I found a bit confusing.

I think the name getObsQueryOptions is just poorly chosen.
It's more getWatchQueryOptionsPotentiallyMergingSomeOptionsFromThePreviousObservable.

In the getSSRObservable case, there is just a guarantee of "observable will never change", so the call is not necessary there.

I'd say let's leave it for now, but it is something I hope we can get rid of in the long term altogether.

jerelmiller · 2024-07-03T22:27:32Z

+  options: QueryHookOptions<NoInfer<TData>, NoInfer<TVariables>>,
+  watchQueryOptions: Readonly<WatchQueryOptions<TVariables, TData>>
+) {
+  {


Are these braces needed? Any chance we could remove the { } surrounding the implementation here?

Uhh... yah no idea how I got these there..

jerelmiller · 2024-07-03T22:33:57Z

-    );
+export function toQueryResult<TData, TVariables extends OperationVariables>(
+  result: ApolloQueryResult<TData>,
+  resultData: InternalResult<TData, TVariables>,


Suggested change

resultData: InternalResult<TData, TVariables>,

previousData: TData | undefined,

Could we pass in previousData here instead? The resultData.current = toQueryResult(resultData) feels a tad odd because I'd expect to mutate resultData or something else. Changing this to previousData makes it more obvious what the dependency is here.

jerelmiller · 2024-07-03T22:42:01Z

+  const queryResult: InternalQueryResult<TData, TVariables> = {
+    data, // Ensure always defined, even if result.data is missing.
+    ...resultWithoutPartial,
+    client: client,


I wonder if there is an opportunity to do a touch more with the value returned from this function. I see that client is set here, which appears to be used by _useQuery (via result), but I also see that useQueryInternals returns a client as well, which should be the same object. Seems we have some overlap between result and the return value of useQueryInternals. Any chance we could reduce the overlap between the two a bit further? Perhaps client and observable can be read from result instead of the top-level object in useQueryInternals.

I just gave that a try - it adds another 15 bytes (this touches a lot of code), and I'm not 100% sure without a very thorough review if this would always hold the right client for the right request.

As this whole "result handling" logic is something I kinda want to revamp anyways, I'd also say let's shelve this one.

jerelmiller · 2024-07-03T22:52:33Z

+      (handleStoreChange) => {
+        // reference `disableNetworkFetches` here to ensure that the rules of hooks
+        // keep it as a dependency of this effect, even though it's not used
+        disableNetworkFetches;


Side note:

At some point, it would be great to write a test for this. I've commented this out here and from the dependency array and everything continues to pass in both the main and ssr useQuery tests. Are we sure this reference is actually needed? Might be good to evaluate if this is actually covered by a test somewhere (sorry I'm not doing that deep dive myself right now 😆)

I'm pretty sure it's not covered by a test 😭
This is a very weird workaround for SSR that we should also get rid of at some point.

Co-authored-by: Jerel Miller <jerelmiller@gmail.com>

phryneas changed the base branch from pr/lint-hooks to main June 21, 2024 10:41

phryneas added this to the 3.11.0 milestone Jun 25, 2024

phryneas changed the title ~~[WIP] refactor useQuery to not use an internal class~~ refactor useQuery to not use an internal class Jun 25, 2024

phryneas requested review from alessbell and jerelmiller June 25, 2024 09:04

phryneas marked this pull request as ready for review June 25, 2024 09:05

jerelmiller changed the base branch from main to release-3.11 July 2, 2024 17:29

phryneas added 19 commits July 3, 2024 11:04

extract hooks from InternalState

c2bd48b

inline hooks and functions into each other, move `createWatchQueryOpt…

dc58beb

…ions` out

move executeQuery to useLazyQuery

10466e5

move more functions out

ea77f60

move unsafeHandlePartialRefetch into setResult

fde4c4d

remove toQueryResultCache, save transformed result in `internalStat…

e1b7ed7

…e.result`

fixup test

9c865ca

move getDefaultFetchPolicy out

b5e1643

move more functions out

b4c8bf9

moved all class methods out

12e19f9

replace class with single mutable object

24e4491

move callbacks into their own ref

e31d953

move obsQueryFields out of internalState

891e211

inline useInternalState

ec1c7a9

redactor away internalState.queryHookOptions

79566a8

make function arguments more explicit

873f24d

replace internalState.watchQueryOptions with `observable[lastWatchO…

8bb445c

…ptions]`

move observable fully into a readonly prop on internalState

635a32b

remove all direct access to internalState after initializing

d6de17f

phryneas added 3 commits July 3, 2024 11:04

extract new useInternalState hook

06d98ac

extract useResubscribeIfNecessary hook

7717b4f

add comment

3889fff

phryneas force-pushed the pr/rewrite-useQuery branch 2 times, most recently from ec7621f to a8f9e70 Compare July 3, 2024 09:52

phryneas added 9 commits July 3, 2024 11:54

extract bindObservableMethods

da4b840

extract useHandleSkip and useRegisterSSRObservable hooks

f18b753

extract useObservableSubscriptionResult hook

8480ce6

change some method arguments. remove obsolete comment

30b1769

curry createMakeWatchQueryOptions

c896885

bring function and hook argyuments into a common order

1485651

Move onQueryExecuted into useInternalState

70f5aaf

some style adjustments to be more compiler-friendly

fed117b

remove R19 exception from test, chores

a69327c

phryneas force-pushed the pr/rewrite-useQuery branch from a8f9e70 to a69327c Compare July 3, 2024 09:54

jerelmiller approved these changes Jul 3, 2024

View reviewed changes

phryneas force-pushed the pr/rewrite-useQuery branch 3 times, most recently from c903b43 to dfda94e Compare July 4, 2024 14:18

Apply suggestions from code review

987aaad

Co-authored-by: Jerel Miller <jerelmiller@gmail.com>

phryneas force-pushed the pr/rewrite-useQuery branch from dfda94e to 987aaad Compare July 4, 2024 14:24

Merge branch 'release-3.11' into pr/rewrite-useQuery

33c0fef

phryneas force-pushed the pr/rewrite-useQuery branch from 8be4f63 to 33c0fef Compare July 4, 2024 14:38

phryneas merged commit 5ae4876 into release-3.11 Jul 4, 2024

phryneas deleted the pr/rewrite-useQuery branch July 4, 2024 14:44

github-actions Bot mentioned this pull request Jul 9, 2024

Version Packages (rc) #11940

Merged

github-actions Bot mentioned this pull request Jul 22, 2024

Version Packages #11967

Merged

github-actions Bot locked as resolved and limited conversation to collaborators Aug 4, 2024

		@@ -1,19 +1,38 @@
		/**
		* Function parameters in this file try to follow a common order for the sake of

	resultData: InternalResult<TData, TVariables>,
	previousData: TData \| undefined,

Conversation

phryneas commented May 27, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

changeset-bot Bot commented May 27, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

🦋 Changeset detected

Uh oh!

github-actions Bot commented May 27, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

size-limit report 📦

Uh oh!

netlify Bot commented Jun 21, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

✅ Deploy Preview for apollo-client-docs ready!

Uh oh!

jerelmiller commented Jul 3, 2024

Uh oh!

phryneas commented Jul 3, 2024

Uh oh!

jerelmiller left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

phryneas Jul 4, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

phryneas Jul 4, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jerelmiller Jul 3, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

phryneas commented May 27, 2024 •

edited

Loading

changeset-bot Bot commented May 27, 2024 •

edited

Loading

github-actions Bot commented May 27, 2024 •

edited

Loading

netlify Bot commented Jun 21, 2024 •

edited

Loading

jerelmiller left a comment •

edited

Loading

phryneas Jul 4, 2024 •

edited

Loading

phryneas Jul 4, 2024 •

edited

Loading

jerelmiller Jul 3, 2024 •

edited

Loading