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

Call nextFetchPolicy with "variables-changed" even if there is a fetchPolicy option specified. #11626

Merged
merged 8 commits into from
Jul 8, 2024

Conversation

phryneas
Copy link
Member

fixes #11365

@phryneas phryneas requested a review from a team as a code owner February 27, 2024 11:46
Copy link

changeset-bot bot commented Feb 27, 2024

🦋 Changeset detected

Latest commit: 014d370

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

Copy link
Contributor

github-actions bot commented Feb 27, 2024

size-limit report 📦

Path Size
dist/apollo-client.min.cjs 38.99 KB (+0.05% 🔺)
import { ApolloClient, InMemoryCache, HttpLink } from "dist/main.cjs" 47.7 KB (+0.02% 🔺)
import { ApolloClient, InMemoryCache, HttpLink } from "dist/main.cjs" (production) 45.24 KB (+0.04% 🔺)
import { ApolloClient, InMemoryCache, HttpLink } from "dist/index.js" 34.27 KB (+0.02% 🔺)
import { ApolloClient, InMemoryCache, HttpLink } from "dist/index.js" (production) 32.13 KB (+0.03% 🔺)
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.23 KB (0%)
import { useQuery } from "dist/react/index.js" (production) 4.31 KB (0%)
import { useLazyQuery } from "dist/react/index.js" 5.71 KB (0%)
import { useLazyQuery } from "dist/react/index.js" (production) 4.79 KB (0%)
import { useMutation } from "dist/react/index.js" 3.62 KB (0%)
import { useMutation } from "dist/react/index.js" (production) 2.84 KB (0%)
import { useSubscription } from "dist/react/index.js" 3.63 KB (0%)
import { useSubscription } from "dist/react/index.js" (production) 2.78 KB (0%)
import { useSuspenseQuery } from "dist/react/index.js" 5.49 KB (0%)
import { useSuspenseQuery } from "dist/react/index.js" (production) 4.15 KB (0%)
import { useBackgroundQuery } from "dist/react/index.js" 4.99 KB (0%)
import { useBackgroundQuery } from "dist/react/index.js" (production) 3.64 KB (0%)
import { useLoadableQuery } from "dist/react/index.js" 5.07 KB (0%)
import { useLoadableQuery } from "dist/react/index.js" (production) 3.72 KB (0%)
import { useReadQuery } from "dist/react/index.js" 3.39 KB (0%)
import { useReadQuery } from "dist/react/index.js" (production) 3.33 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%)

Copy link

netlify bot commented Feb 27, 2024

Deploy Preview for apollo-client-docs ready!

Name Link
🔨 Latest commit 014d370
🔍 Latest deploy log https://app.netlify.com/sites/apollo-client-docs/deploys/668baf37fb1a340008c62aa1
😎 Deploy Preview https://deploy-preview-11626--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.

(options.fetchPolicy === oldFetchPolicy ||
// A `nextFetchPolicy` function has even higher priority, though,
// so in that case `applyNextFetchPolicy` must be called.
typeof options.nextFetchPolicy === "function")
) {
this.applyNextFetchPolicy("variables-changed", options);
if (newNetworkStatus === void 0) {
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It seems that lines 917-920
(

if (newNetworkStatus === void 0) {
    newNetworkStatus = NetworkStatus.setVariables;
        }

could be removed completely without failing any test. In fact, the new network status is set as part of QueryInfo.init.

I did not remove it, though, to prevent any unforseen consequences - but wanted to have noted it somewhere.

@phryneas
Copy link
Member Author

/release:pr

Copy link
Contributor

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

Copy link
Member

@jerelmiller jerelmiller left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I had a small comment about the test, but overall I think this change makes sense. Let's get this in 3.11!

link,
});

const fetchQueryByPolicySpy = jest.spyOn(
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

While I am beginning to understand the purpose of checking on whether this is called, I'm curious if we even need this. Would it be enough to check that observable.options.fetchPolicy is set to the correct value like you have below and trust that ObservableQuery does the right thing with it? I'd be ok delegating the testing of that behavior in ObservableQuery tests. On top of that, this makes it a tad more difficult to refactor any of that internal private logic since changes to this API (which are non-breaking) would require changes to this test. If we can avoid that at all possible, that would be awesome.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Would it be enough to check that observable.options.fetchPolicy is set to the correct value like you have below and trust that ObservableQuery does the right thing with it?

Rereading the test: observable.options.fetchPolicy changes multiple times - so we can't tell for sure if we assert on an intermediate or the final value.

The

      // first network request triggers with initial fetchPolicy
      expectQueryTriggered(1, "network-only");

could probably not be asserted in another way, since it changes around too fast.

=> I can't think of another way of testing that. I also hate that we test an internal detail, but I fear we can't avoid it here.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

On a kinda-positive note, we have an abstraction for mocking this in tests - I'll use that. At least then, if we break internals, they will do everywhere in a consistent matter.

import { mockFetchQuery } from "../../../core/__tests__/ObservableQuery";

@jerelmiller jerelmiller changed the base branch from main to release-3.11 July 5, 2024 22:24
@jerelmiller jerelmiller added this to the 3.11.0 milestone Jul 5, 2024
@phryneas phryneas added the auto-cleanup 🤖 label Jul 8, 2024
@phryneas phryneas merged commit 228429a into release-3.11 Jul 8, 2024
42 checks passed
@phryneas phryneas deleted the pr/fix-11365 branch July 8, 2024 09:26
@dylanwulf
Copy link
Contributor

Hi @phryneas! I just wanted to note that this is a breaking change for my team so I was wondering if you would be able to document this as a breaking/disruptive change in the changelog? Our implementation of nextFetchPolicy actually predates the existence of the context.reason option so we haven't been checking that option in our logic, and because of #11365 our existing nextFetchPolicy implementation hasn't broken until now. I agree that the change in this PR is the way to go, I just think the changelog should point out that this might break things for people who haven't looked at the nextFetchPolicy docs in a long time.

Also, I feel that the nextFetchPolicy docs are a little unclear on this topic:

In addition to being called after each request, your nextFetchPolicy function will also be called when variables change, which by default resets the fetchPolicy to its initial value, which is often important to trigger a fresh network request for queries that started out with cache-and-network or network-only fetch policies.

To intercept and handle the variables-changed case yourself, you can use the NextFetchPolicyContext object passed as the second argument to your nextFetchPolicy function:

This part of the text mentions that the default behavior for variables changing is to reset the fetchPolicy to its initial value. However, this text does not mention that implementing your own nextFetchPolicy function will disable that default reset behavior. The code example that goes after this text does include a comment If you omit this logic, your nextFetchPolicy function can override this default behavior to prevent options.fetchPolicy from changing in this case. But even with this comment I still think it's not very clear that implementing a nextFetchPolicy function will disable the default reset behavior, and that the user needs to re-implement it themself. It's a pretty small detail though, so if you disagree then no worries! 😄

@github-actions github-actions bot mentioned this pull request Jul 9, 2024
@phryneas
Copy link
Member Author

Ugh, yeah :/

Tbh., we're a bit between a rock and a hard place with fetchPolicy and especially nextFetchPolicy - in the past, a lot of changes have been made here that should have been marked as breaking, and all we can do right now is to try and hone in on the documented behaviour.
And every of those fixes (while clearly fixes!) is breaking in itself.

Right now we only have this out in an RC, so we can definitely still work on the wording, and also adjust some docs.

I'll put it on our team agenda for next Monday so we can discuss this. Thank you for bringing this up!

@github-actions github-actions bot mentioned this pull request Jul 22, 2024
@github-actions github-actions bot locked as resolved and limited conversation to collaborators Aug 10, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

nextFetchPolicy function is not called sometimes
3 participants