feat: Add skipPollAttempt option to control polling refetch behavior

[aditya-kumawat]

Summary

This adds skipPollAttempt optional option to skip refetching if it returns true and works as usual if not set or if it returns false.

const client = new ApolloClient({
  defaultOptions: {
    // This should only be available for watchQuery since its the only one that allows polling
    watchQuery: {
      // Only allow polling when the window is focused. Enables it everywhere. Default is `false`
      skipPollAttempt: () => document.hidden
    }
  }
});

function MyComponent() {
  // Override the global focusPolling option for a single query
  useQuery(QUERY, { pollInterval: 1000, skipPollAttempt: () => document.hidden })
}

This will solve the frequent use-case of disabling polling when the window is inactive(Feature request #247).

Details about the implementation:

Took inspiration from idea proposed here.

Evidences

pollInterval: 1000

Kapture.2023-11-30.at.04.37.52.mp4

pollInterval: 1000, skipPollAttempt: () => document.hidden

Kapture.2023-11-30.at.04.39.50.mp4

skipPollAttempt: () => document.hidden with startPolling and stopPolling

Kapture.2023-11-30.at.04.42.44.mp4

Global skipPollAttempt: () => document.hidden

Kapture.2023-11-30.at.04.46.11.mp4

Checklist:

• [✔️] If this PR contains changes to the library itself (not necessary for e.g. docs updates), please include a changeset (see CONTRIBUTING.md)
• [✖️] If this PR is a new feature, please reference an issue where a consensus about the design was reached (not necessary for small changes)
• [✔️] Make sure all of the significant new logic is covered by tests

[apollo-cla]

@aditya-kumawat: Thank you for submitting a pull request! Before we can merge it, you'll need to sign the Apollo Contributor License Agreement here: https://contribute.apollographql.com/

[netlify]

‼️ Deploy request for apollo-client-docs rejected.

Name	Link
🔨 Latest commit	8e38108

[changeset-bot]

🦋 Changeset detected

Latest commit: e452328

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

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

[jerelmiller]

Hey @aditya-kumawat 👋

Appreciate the contribution here!

I'd love to get a couple things from you here regarding this change. We are generally hesitant to expand the footprint of options that can be provided to our APIs so I'd love to dig a bit deeper and discuss what existing limitations there are and how you see this as an improvement on the "status quo".

useQuery currently exports startPolling and stopPolling functions that you can use to manually start/stop polling on your query.

Given your example using document.hidden here, the following can be achieved without expanding the footprint of the API:

const { startPolling, stopPolling } = useQuery(QUERY, { pollInterval: 5000 })

useEffect(() => {
  function handleVisibilityChange() {
    if (document.hidden) {
      stopPolling()
    } else {
      startPolling(5000)
    }
  }

  document.addEventListener('visibilitychange', handleVisibilityChange)

  return () => {
    document.removeEventListener('visibilitychange', handleVisibilityChange)
  }
}, [startPolling, stopPolling])

Changes to pollInterval between renders are also applied, so if you prefer the setState approach, you could do this as well.

const [pollInterval, setPollInterval] = useState(5000)
useQuery(QUERY, { pollInterval })

useEffect(() => {
  function handleVisibilityChange() {
    setPollInterval(document.hidden ? 0 : 5000)
  }

  document.addEventListener('visibilitychange', handleVisibilityChange)

  return () => {
    document.removeEventListener('visibilitychange', handleVisibilityChange)
  }
}, [])

I know its a tad verbose, but what I'm trying to illustrate here is that this should be possible given the current API.

---

What I'd love to get from you is to more deeply understand what you see as the limitation with the existing tools, what other use cases you can see for this API, and how this option helps avoid those challenges with the existing API.

Once again, appreciate the contribution here!

[aditya-kumawat]

I see I need to make fresh changes after the rebase. Will do it but unable to run npm install after rebase with latest master.

2023-11-29T20_35_58_783Z-debug-0.log

[aditya-kumawat]

@jerelmiller Thanks for quick response.

Solution proposed by you works fine for a single query. In my organisation we are using useQuery extensively(more than 1000 use-cases with most of the places using pollInterval and few of the places by using startPolling and stopPolling.

It's not feasible to update each and every instance given the large number. We already have a wrapper created on top of useQuery which is used at most of the places and can be used to remove polling when tab is inactive if we have an option available directly as part of useQuery.

[jerelmiller]

@aditya-kumawat that makes sense.

Solution proposed by you works fine for a single query. In my organisation we are using useQuery ...more than 1000 use-cases

Perhaps I'm just not seeing it, but I don't quite see how your proposed solution here fixes this issue. ObservableQuery is still just single instance of a query. In fact useQuery is just a wrapper around client.watchQuery, which returns an instance of ObservableQuery. This option would still need to be given on a per-query basis to work. Would you mind walking me through how you see this proposal solving this issue?

Or are you specifically referring to the fact that you're trying to avoid writing that useEffect a bunch of times?

To touch a bit more on the goal of this API, are you specifically after focus refetching with this feature as talked about in apollographql/apollo-feature-requests#247, or are you looking for this to be a more generalized API that lets you selectively enable/disable polling based on any condition?

[aditya-kumawat]

@aditya-kumawat that makes sense.

Solution proposed by you works fine for a single query. In my organisation we are using useQuery ...more than 1000 use-cases

Perhaps I'm just not seeing it, but I don't quite see how your proposed solution here fixes this issue. ObservableQuery is still just single instance of a query. In fact useQuery is just a wrapper around client.watchQuery, which returns an instance of ObservableQuery. This option would still need to be given on a per-query basis to work. Would you mind walking me through how you see this proposal solving this issue?

Or are you specifically referring to the fact that you're trying to avoid writing that useEffect a bunch of times?

To touch a bit more on the goal of this API, are you specifically after focus refetching with this feature as talked about in apollographql/apollo-feature-requests#247, or are you looking for this to be a more generalized API that lets you selectively enable/disable polling based on any condition?

We created following utility to control things across the codebase.

export const generateQueryHook =
  <TQuery, TVariables extends OperationVariables = OperationVariables>(
    query: DocumentNode,
  ) =>
  (options?: QueryHookOptions<TQuery, TVariables>) =>
    useQuery<TQuery, TVariables>(query, options);

If I have an option to pass shouldPollingBeEnabled or shouldFetch(something already present privately in latest master), I would need to make changes at only one place.
Assuming even if this was not present, its easier to find all instances of useQuery and add shouldPollingBeEnabled everywhere compared to adding useEffect bunch of times at 1000+ instances.

My use-case is limited to apollographql/apollo-feature-requests#247 only for now. I raised a PR for more generalised API expecting something like that might be needed in future. If you don't want to expose, it should be fine if we can add a boolean option just to handle inactivity scenario.

[jerelmiller]

We created following utility to control things across the codebase.

I love that you've got an abstraction thats been working well for you in your codebase! I always love to see creative ways people use these hooks.

Something to keep in mind with this specific feature is that unfortunately most of the Apollo users won't have something like this in their codebase and tend to use useQuery directly. For those users, adding in focus refetching with an additional option for every useQuery would still be a lot of updates across the codebase.

My use-case is limited to apollographql/apollo-feature-requests#247 only for now.

This is useful! I'd propose then that whatever API is added to the client would be a feature specific to this issue rather than a more generalized API. If we approach it this way, we'll have the freedom to make this a more "automatic" thing in the client.

I'd also like to separate this particular concept (I'll call it focus polling) from focus refetching. I see the two as slightly different concepts. Focus polling would allow you to only run the poll interval when the window is focused, where focus refetching would allow you to run a refetch when the user has focused the page to keep it fresh (but it would only execute a single network request, unlike polling). In fact, this is something I should probably comment about in apollographql/apollo-feature-requests#247 to ensure we are all on the same page 😆.

---

If we were to move this feature forward, there are a few things that we'd need to make this work in order to make it a non-breaking change.

• This would need to be something opt-in and work as part of the core API, that way users of other view libraries (such as vue-apollo or apollo-angular) can benefit as well. You've already taken a step here by adding this code to ObservableQuery, which is a good start.
• This option should be available to use globally as well as a per-query basis. Per-query basis should override whatever is global (so that you could, for example enable it globally, but disable it for a single query).
• The client should not crash in server environment where window is not available.

Here is a rough idea for what this might look like (terrible name for the option for now, but it gets the idea across)

const client = new ApolloClient({
  defaultOptions: {
    // This should only be available for watchQuery since its the only one that allows polling
    watchQuery: {
      // Only allow polling when the window is focused. Enables it everywhere. Default is `false`
      focusPolling: true
    }
  }
});

function MyComponent() {
  // Override the global focusPolling option for a single query
  useQuery(QUERY, { focusPolling: false })
}

Given the above, are you still interesting in working on this feature? And if so, would you prefer to work on it in this PR, or open a new, fresh PR? I'd be happy to collaborate with you to help you get it across the finish line.

[aditya-kumawat]

@jerelmiller This really helps.

I like the solution you proposed and can work on it(unless I get stucked and might need others to pick it up). I will prefer to use the same PR as the discussion might come in handy for others. I can create a new one if you feel otherwise.

The client should not crash in server environment where window is not available.

I am not sure about how to handle this scenario though. Can you help out with this one?

[jerelmiller]

@aditya-kumawat this PR should be fine! Feel free to update this one and work on it here.

A couple things I'd ask if you could do for us:

1. Put this PR in draft state so that we know its a work-in-progress
2. Rename the PR to better reflect the feature request rather than the option name
3. Please make sure to write thorough tests for this feature

Tests are the critical one to us getting this merged as it lets us know you've considered all the cases and makes sure we don't break anything in the future regarding this feature. These things would be super helpful though in the mean time!

I am not sure about how to handle this scenario though. Can you help out with this one?

You should be able to check whether window is available in a way that doesn't crash with the following check:

typeof window !== 'undefined'

Assuming you'll be using window.addEventListener to watch for changes to focus, you'll need this check before you add listeners, otherwise the client will crash on the server.

---

Oh, and one more thing I just thought of while I'm here. Tanstack Query has the ability to determine which events cause focus, that way you're not limited to whatever the library chose. I'm not asking for an API like this one, but would love for you to think through this a bit and whether we'd need the ability for the user to specify or change the events that would trigger focus so that polling enables/disables appropriately. Hopefully this makes sense!

[aditya-kumawat]

@jerelmiller I will make sure of the checklist you mentioned above.

I am refraining myself from using window.addEventListener for focus event as it gets triggered even if someone is using dev console of the browser.
Also, I don't think I will be needing an event listener for the same. Solution I am thinking of should simply skip the refetch if at that time document.hidden is set as true which happens when someone moves to a different window or tab. It works normally when someone is using dev console of the browser.
I am using document.hasFocus() instead of document.hidden due to the same dev console focus issue. More info in this stackoverflow answer.

What are your thoughs on this?

[jerelmiller]

Solution I am thinking of should simply skip the refetch if at that time document.hidden is set as true which happens when someone moves to a different window or tab.

This may very well be the right solution! I'll leave it up to you what seems the most appropriate here and give you a chance to play around with it a bit to see what works the best.

Something to consider here is whether you'd like the fetch to occur the moment the window is focused, or whether it just re-enables itself on the next tick of the interval (which might be several seconds after focus, depending on the poll interval). Again, this might be totally appropriate here! Just wanted to make sure you're thinking of this as well so that we can explain the decision/behavior here in case we get questions about how this feature works.

[jerelmiller]

@aditya-kumawat FYI just realized you're using a fork of the library that is several years old at this point. Some of those methods you're touching don't exist anymore. To save yourself some time, I'd recommend ensuring your fork is up-to-date with the latest changes in main first, otherwise you might end up having to rework this feature entirely if you sync later.

Just wanted you to be aware of this!

[aditya-kumawat]

@jerelmiller Thanks for pointing this out. Earlier I made the changes correctly but after rebase with origin/master and not origin/main, I struggled and made fresh changes again.
Reverted changes to the older version now.

PTAL at the code changes for initial review. Will add tests and mark this PR ready for review if it looks good.

[jerelmiller]

Initial glance looks good, but I'd prefer to do a thorough review after you've had a chance to write tests in case you find edge cases that require you to change the logic needed to make this work correctly.

[aditya-kumawat]

@jerelmiller Done adding the tests as well but some unrelated tests are failing. I think so as those tests are failing locally on origin/main as well.
PTAL when you get a chance. I will be AFK for sometime now.

[aditya-kumawat]

@jerelmiller @Meschreiber Thanks for the review.
Addressed the comments. PTAL

[Meschreiber]

Doc comment LGTM! Thanks @aditya-kumawat !

[jerelmiller]

Hey @aditya-kumawat 👋

We'll be discussing the name of this option in our next team meeting and will get back to you assuming we reach consensus on a final naming decision here. I'm hopeful to have another review to you tomorrow!

[aditya-kumawat]

@jerelmiller Sounds good to me. Thanks for the update.

[jerelmiller]

Hey @aditya-kumawat 👋

This should be my last review needed. I've talked with the team and we decided we like the name skipPollAttempt for this option. Once you're able to get this PR updated with that name, I'll get this PR approved and into the release branch. We should be able to get this into the next alpha release.

Thanks so much again for the contribution! Excited to get this one in there!

[aditya-kumawat]

@jerelmiller Suggestions looks good. Done with the changes.
It feels great to contribute here, will try to ensure that I get less review comments next time 😄

[jerelmiller]

🎉

[jerelmiller]

will try to ensure that I get less review comments next time

@aditya-kumawat don't worry about this at all! We also went back and forth on our end a bit. I'm very appreciative for receiving my feedback and iterating on this. Hope to see more contributions for you in the future!

I'll get this merged to our next release branch. Look out for this in the next minor release 3.9.0! We'll also plan to get this out in the next prerelease alpha version so you can try it out in your own apps before 3.9.0 ships.