-
-
Notifications
You must be signed in to change notification settings - Fork 30
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
feat(plugins): Add refetch interval plugin #153
Conversation
✅ Deploy Preview for funny-banoffee-0afb46 ready!
To edit notification comments on pull requests, go to your Netlify site configuration. |
Great! Does this overlap with #97 ? I yes, I might need to merge the other one first and then refactor the two PRs. We can change the API of plugins with breaking changes to improve them. Edit: this is great but it feels off to have document visibility integrated into it. The query key can be serialized but the method is not exported so I fixed that in 542e15e with
Sure 🙌 If you have any question, let me know. It would also help me out to point out details in the docs |
}, | ||
"version": "0.0.1", | ||
"description": "Refetch queries on a regular interval with Pinia Colada", | ||
"author": { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Feel free to add yourself if you want: https://docs.npmjs.com/cli/v11/configuring-npm/package-json#people-fields-author-contributors
export interface RefetchIntervalOptions { | ||
/** | ||
* If set to a number, queries will continuously refetch at this frequency in milliseconds. | ||
* If set to false, no automatic refetching will occur. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What about setting it to true
or 'auto'
and have them auto refresh when stale? #97
timeoutId: setTimeout(() => { | ||
// Only refetch if refetchIntervalInBackground is true or document is visible | ||
if (options.refetchIntervalInBackground || isDocumentVisible) { | ||
Promise.resolve(queryCache.fetch(queryEntry)) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Does it make sense to force a fetch? If the data is fresh, it should be fine to not fetch again 🤔
return ({ queryCache }) => { | ||
const intervalMap = new Map<string, IntervalEntry>() | ||
|
||
let isDocumentVisible = typeof document !== 'undefined' |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm wondering if this makes sense at this level. It could be set in the enabled
option so it feels a bit redundant here, the overlap between options could prove difficult in the future to debug.
// setupNextInterval() | ||
|
||
// If we can't refetch now, invalidate the query | ||
queryCache.invalidate(queryEntry) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
FYI: invalidate also fetches if the query is active (displayed somewhere)
import type { PiniaColadaOptions, UseQueryOptions } from '@pinia/colada' | ||
import { PiniaColadaRefetchInterval } from '.' | ||
|
||
describe('Refetch Interval plugin', () => { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nice! A lot of tests 😄
I release the plugin |
@posva Thanks for the review! As for the auto-refetch plugin, I didn’t realize right away that it basically does the same thing. I’ll give it a try, and it’ll probably be enough 👍 |
@posva Auto-refetch plugin does everything I need, but I'm currently really missing the ability to control autoRefetch options via a ref or getter—for example, to disable auto-refetch when the user leaves the tab. EDIT: Sorry, it seems like it's working because the plugin uses toValue with this option, but there's a type mismatch. The plugin expects a boolean, but I guess it should be MaybeRefOrGetter<boolean>. |
Nice catch! I will release a patch in a second. |
I will close this for the moment, let me know if you find anything missing 🙂 |
Hi! Colada is awesome! I’m planning to migrate from TanStack, but the key feature I’m currently missing to start using the library is
refetchInterval
.I’ve added a base plugin and some test cases, following the structure of other plugins. It works and behaves similarly to TanStack, but Colada’s plugin API feels a bit complicated to me, so I’d appreciate your help in finishing it properly.
In particular, I’m unsure about the Map keys I’m using. Should we use the query key (array?) itself as the Map key? And should it be a WeakMap instead?