potential fix for #10802 #11633

phryneas · 2024-03-01T14:24:07Z

This is a potential fix for #10802. I'm not exactly happy with this change yet, but it does solve the problem, so I already wanted to create a PR release.

changeset-bot · 2024-03-01T14:24:11Z

🦋 Changeset detected

Latest commit: ec3ade9

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	Patch

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

phryneas · 2024-03-01T14:24:16Z

/release:pr

github-actions · 2024-03-01T14:25:29Z

size-limit report 📦

Path	Size
dist/apollo-client.min.cjs	38.41 KB (+0.03% 🔺)
import { ApolloClient, InMemoryCache, HttpLink } from "dist/main.cjs"	46.23 KB (+0.03% 🔺)
import { ApolloClient, InMemoryCache, HttpLink } from "dist/main.cjs" (production)	43.79 KB (+0.03% 🔺)
import { ApolloClient, InMemoryCache, HttpLink } from "dist/index.js"	33.96 KB (+0.03% 🔺)
import { ApolloClient, InMemoryCache, HttpLink } from "dist/index.js" (production)	31.88 KB (+0.04% 🔺)
import { ApolloProvider } from "dist/react/index.js"	1.23 KB (0%)
import { ApolloProvider } from "dist/react/index.js" (production)	1.22 KB (0%)
import { useQuery } from "dist/react/index.js"	5.26 KB (0%)
import { useQuery } from "dist/react/index.js" (production)	4.35 KB (0%)
import { useLazyQuery } from "dist/react/index.js"	5.5 KB (0%)
import { useLazyQuery } from "dist/react/index.js" (production)	4.58 KB (0%)
import { useMutation } from "dist/react/index.js"	3.51 KB (0%)
import { useMutation } from "dist/react/index.js" (production)	2.73 KB (0%)
import { useSubscription } from "dist/react/index.js"	3.19 KB (0%)
import { useSubscription } from "dist/react/index.js" (production)	2.38 KB (0%)
import { useSuspenseQuery } from "dist/react/index.js"	5.41 KB (0%)
import { useSuspenseQuery } from "dist/react/index.js" (production)	4.07 KB (0%)
import { useBackgroundQuery } from "dist/react/index.js"	4.89 KB (0%)
import { useBackgroundQuery } from "dist/react/index.js" (production)	3.55 KB (0%)
import { useLoadableQuery } from "dist/react/index.js"	5.02 KB (0%)
import { useLoadableQuery } from "dist/react/index.js" (production)	3.68 KB (0%)
import { useReadQuery } from "dist/react/index.js"	3.17 KB (0%)
import { useReadQuery } from "dist/react/index.js" (production)	3.11 KB (0%)
import { useFragment } from "dist/react/index.js"	2.27 KB (0%)
import { useFragment } from "dist/react/index.js" (production)	2.22 KB (0%)

github-actions · 2024-03-01T14:25:35Z

A new release has been made for this PR. You can install it with npm i @apollo/client@0.0.0-pr-11633-20240301142454.

netlify · 2024-03-01T14:29:16Z

✅ Deploy Preview for apollo-client-docs ready!

Name	Link
🔨 Latest commit	`ec3ade9`
🔍 Latest deploy log	https://app.netlify.com/sites/apollo-client-docs/deploys/66029b84234afc00081da139
😎 Deploy Preview	https://deploy-preview-11633--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.

phryneas · 2024-03-01T14:47:33Z

Not sure what's up with GitHub right now, but the tests are green on this.

jerelmiller

I honestly don't have a better idea on how to fix this. Take a look at my refactoring though and I think you'll see we end up with a bit more readable solution.

Would it also be possible to write a test for this? I think my refactoring is equivalent to the change you made here, but I'm not able to check my work against this.

jerelmiller · 2024-03-12T04:15:26Z

src/core/ObservableQuery.ts

@@ -252,7 +252,8 @@ export class ObservableQuery<
    if (
      // These fetch policies should never deliver data from the cache, unless
      // redelivering a previously delivered result.
-      skipCacheDataFor(fetchPolicy) ||
+      (skipCacheDataFor(fetchPolicy) &&
+        !(fetchPolicy === "network-only" && !this.waitForOwnResult)) ||


I always find the !(...) harder to reason about since you have to understand the stuff inside the parens, then negate it. Instead, we can apply De Morgan's Law which states that "not (A and B)" is equaivalent of "(not A) or (not B)" (essentially distributing the ! to each clause and changing the && to an ||.

I find that a bit easier to reason about:

Suggested change

!(fetchPolicy === "network-only" && !this.waitForOwnResult)) ||

(fetchPolicy !== "network-only" || this.waitForOwnResult)) ||

Ok now to dissect this further. For me to understand this better, it helps for me to do a bit of refactoring to remove the function calls and inline the checks here. Let's first start with inline method to pull in the body of skipCacheData in place of its check.

((fetchPolicy === "no-cache" || fetchPolicy === "network-only" || fetchPolicy === "standby") && (fetchPolicy !== "network-only" || this.waitForOwnResult))

This now makes it possible to see the combinations of fetchPolicy checks here.

Digging further, we know that the 2nd clause won't be executed unless the first is truthy. Using this, we can deduce that fetchPolicy !== 'network-only' is the same as fetchPolicy === 'no-cache' || fetchPolicy === 'standby' since these are the only two fetch policies left over from the first check.

((fetchPolicy === "no-cache" || fetchPolicy === "network-only" || fetchPolicy === "standby") && (fetchPolicy === "no-cache" || fetchPolicy === 'standby' || this.waitForOwnResult))

We now begin to really see some repetition here, notably that when fetchPolicy === 'no-cache' || fetchPolicy === 'standby' is true, the whole thing will always be true.

Let's temporarily refactor out this check to its own variable to make the conditional a bit more obvious.

const absoluteNoCache = fetchPolicy === "no-cache" || fetchPolicy === "standby"; // ... ((absoluteNoCache || fetchPolicy === "network-only") && (absoluteNoCache || this.waitForOwnResult))

This makes it obvious that the condition will pass whenever absoluteNoCache is true. We should be able to pull absoluteNoCache out into its own clause and "or" it with the rest of the logic.

(absoluteNoCache || ((fetchPolicy === "network-only") && (this.waitForOwnResult)))

which is the same as the following when removing unnecessary parens

(absoluteNoCache || (fetchPolicy === "network-only" && this.waitForOwnResult))

Now that we've got the common bits together, let's inline the variable back into the conditional

(fetchPolicy === "no-cache" || fetchPolicy === "standby" || (fetchPolicy === "network-only" && this.waitForOwnResult))

Here is the total of the if altogether:

if ( // These fetch policies should never deliver data from the cache, unless // redelivering a previously delivered result. fetchPolicy === "no-cache" || fetchPolicy === "standby" || (fetchPolicy === "network-only" && this.waitForOwnResult) || // If this.options.query has @client(always: true) fields, we cannot // trust diff.result, since it was read from the cache without running // local resolvers (and it's too late to run resolvers now, since we must // return a result synchronously). this.queryManager.getDocumentInfo(this.query).hasForcedResolvers ) {

Unfortunately I have no tests to check my work, but hopefully stepping through this makes sense.

I don't have a great recommendation other than the fix you have here. In fact, with the refactor, I'd say its about the same as it was before. The only difference is that you're checking && this.waitForOwnResult now when using this with a network-only fetch policy.

Before

// This is the body of skipCacheDataFor(fetchPolicy) inlined fetchPolicy === "no-cache" || fetchPolicy === "standby" || fetchPolicy === "network-only"

After

fetchPolicy === "no-cache" || fetchPolicy === "standby" || (fetchPolicy === "network-only" && this.waitForOwnResult)

Putting it in this light, I feel better about the change 🙂. If you are able to write a test, that would be awesome, then I'd be happy with this PR.

this was from #6353

phryneas · 2024-03-26T09:56:17Z

src/core/__tests__/ObservableQuery.ts

-            // Unlike network-only or cache-and-network, the no-cache
-            // FetchPolicy does not switch to cache-first after the first
-            // network request.


It seems that back in the times of 3.0.0 betas, this was the assumption, which doesn't hold true anymore today (and probably caused this issue).
See #6353

I'll dig on that a bit more.

This was later moved around in 1e5d4f8 and still is there, but apparently doesn't have the intended effect?

apollo-client/src/core/ObservableQuery.ts

Lines 732 to 742 in ec3ade9

// initial FetchPolicy, they often do not want future cache updates to

// trigger unconditional network requests, which is what repeatedly

// applying the "cache-and-network" or "network-only" policies would

// seem to imply. Instead, when the cache reports an update after the

// initial network request, it may be desirable for subsequent network

// requests to be triggered only if the cache result is incomplete. To

// that end, the options.nextFetchPolicy option provides an easy way to

// update options.fetchPolicy after the initial network request, without

// having to call observableQuery.setOptions.

options.fetchPolicy = options.nextFetchPolicy(fetchPolicy, {

reason,

Hm.. the comment had at that moment in time already been disconnected from code.

Yup, this has been removed in 1cd1df9 - ~~but the comment survived until today~~ the comment was adjusted, I had just not re-read the whole black after reading the initial version - I just saw the wall of text with the unchanged first line and assumed it had not changed 😅.

phryneas · 2024-03-26T12:23:03Z

This leaves us with a decision to make:

From what I have unearthed, the current behavior of network-only seems to be quite intentional.

It also matches our documentation for network-only:

Apollo Client executes the full query against your GraphQL server, without first checking the cache. The query's result is stored in the cache.

Prioritizes consistency with server data, but can't provide a near-instantaneous response when cached data is available.

Note how this says that it will update the cache, but it doesn't say that the cache updates it.

=> The originally intended way here was that the user has to specify a nextFetchPolicy of cache-first together with a fetchPolicy of network-only, and that should also be a solution in the original issue.

We'll have to discuss if we want to keep it that way of introduce what is essentially yet another kinda-breaking change to this functionality.

phryneas added 2 commits March 1, 2024 15:22

potential fix for #10802

d0eef2d

changeset

e7fd946

bump CI

ebe40b6

jerelmiller requested changes Mar 12, 2024

View reviewed changes

phryneas added 3 commits March 25, 2024 13:46

Merge remote-tracking branch 'origin/main' into pr/fix-10802

6c4e98b

size-limits

073f318

remove outdated comment

ec3ade9

this was from #6353

phryneas commented Mar 26, 2024

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

potential fix for #10802 #11633

potential fix for #10802 #11633

phryneas commented Mar 1, 2024

changeset-bot bot commented Mar 1, 2024 •

edited

phryneas commented Mar 1, 2024

github-actions bot commented Mar 1, 2024 •

edited

github-actions bot commented Mar 1, 2024

netlify bot commented Mar 1, 2024 •

edited

phryneas commented Mar 1, 2024

jerelmiller left a comment

jerelmiller Mar 12, 2024

jerelmiller Mar 12, 2024 •

edited

phryneas Mar 26, 2024

phryneas Mar 26, 2024 •

edited

phryneas Mar 26, 2024

phryneas Mar 26, 2024 •

edited

phryneas commented Mar 26, 2024

	!(fetchPolicy === "network-only" && !this.waitForOwnResult)) \|\|
	(fetchPolicy !== "network-only" \|\| this.waitForOwnResult)) \|\|

	// initial FetchPolicy, they often do not want future cache updates to
	// trigger unconditional network requests, which is what repeatedly
	// applying the "cache-and-network" or "network-only" policies would
	// seem to imply. Instead, when the cache reports an update after the
	// initial network request, it may be desirable for subsequent network
	// requests to be triggered only if the cache result is incomplete. To
	// that end, the options.nextFetchPolicy option provides an easy way to
	// update options.fetchPolicy after the initial network request, without
	// having to call observableQuery.setOptions.
	options.fetchPolicy = options.nextFetchPolicy(fetchPolicy, {
	reason,

potential fix for #10802 #11633

Are you sure you want to change the base?

potential fix for #10802 #11633

Conversation

phryneas commented Mar 1, 2024

changeset-bot bot commented Mar 1, 2024 • edited

🦋 Changeset detected

phryneas commented Mar 1, 2024

github-actions bot commented Mar 1, 2024 • edited

size-limit report 📦

github-actions bot commented Mar 1, 2024

netlify bot commented Mar 1, 2024 • edited

✅ Deploy Preview for apollo-client-docs ready!

phryneas commented Mar 1, 2024

jerelmiller left a comment

Choose a reason for hiding this comment

jerelmiller Mar 12, 2024

Choose a reason for hiding this comment

jerelmiller Mar 12, 2024 • edited

Choose a reason for hiding this comment

phryneas Mar 26, 2024

Choose a reason for hiding this comment

phryneas Mar 26, 2024 • edited

Choose a reason for hiding this comment

phryneas Mar 26, 2024

Choose a reason for hiding this comment

phryneas Mar 26, 2024 • edited

Choose a reason for hiding this comment

phryneas commented Mar 26, 2024

changeset-bot bot commented Mar 1, 2024 •

edited

github-actions bot commented Mar 1, 2024 •

edited

netlify bot commented Mar 1, 2024 •

edited

jerelmiller Mar 12, 2024 •

edited

phryneas Mar 26, 2024 •

edited

phryneas Mar 26, 2024 •

edited