Skip to content

Conversation

Ephem
Copy link
Collaborator

@Ephem Ephem commented Aug 24, 2020

This PR refactors internals, specifically how scheduleStaleTimeout and scheduleGarbageCollection works behind the scenes, but leaves the documented public API unaffected. An earlier version was begun in #728, but has been fleshed out here and made sense as it's own PR.

The main goal is to remove scheduling of timeouts from the constructor of Query and instead let the consumer creating a new Query mark when timeouts are ready to be scheduled. This is needed to implement hydration, but will also make it easier to remove scheduling of timeouts from the render-phase in useBaseQuery as a next step.

  • Remove scheduling timeouts from constructor
  • Require explicit timeout activation via new query.activateTimeouts()
    • This only applies internally and specifically anyone using buildQuery. This is an undocumented API, so should be okay to change?
  • Refactor all internal code to use activateTimeouts
  • Rename schedule(GC/StaleTimeout) to rescheduleX
  • Make reschedule-methods do the right thing based on query state
    • Previously we relied on these methods being called only at the correct times, now they can be called safely at any time
    • In a sense, this refactor mirrors how state and effects work in React, they "sync the timeout-sideeffects to what the current state is"
    • This gathers all the logic for stale/GC in their respective functions instead of being spread out
  • Stale timeout is now based on updatedAt
    • Besides being necessary for the "reschedule"-approach, this is also necessary for hydration.
    • This could schedule queries to be stale a few ms earlier than previously (equivalent to execution time between updatedAt is set to when the timeout is scheduled, which is a sync process). Docs makes no promises about exact details here so this should be fine since the meaning of staleTime is preserved.

All tests are passing, I'm open to suggestions for adding new ones if there is something we need to verify further.

I'm not sure about the naming of activateTimeouts(), it could also be a more generic initialize() that could contain any future side effects for setting up queries. It could also be just rescheduleTimeouts() which would reflect that it actually schedules those timeouts and that it's is fine to use repeatedly, but that would loose the pretty important semantics that no timeouts will be scheduled until this function is called for the first time.

This is just an undocumented internal name though and easy to change, where it's used now I think activateTimeouts() makes sense, but I'm very open to suggestions. 😄

* Remove scheduling timeouts from constructor
* Require explicit timeout activation via new activateTimeouts
* Refactor all internal code to use activateTimeout
* Rename schedule(GC/StaleTimeout) to rescheduleX
* Make reschedule(GC/StaleTimeout) and heal private
* Make reschedule-methods do the right thing based on query state
* Stale timeout is now based on updatedAt
@vercel
Copy link

vercel bot commented Aug 24, 2020

This pull request is being automatically deployed with Vercel (learn more).
To see the status of your deployment, click below or on the icon next to each commit.

🔍 Inspect: https://vercel.com/tannerlinsley/react-query/d0qgrynhr
✅ Preview: https://react-query-git-fork-ephem-refactor-query-timeouts.tannerlinsley.vercel.app

@tannerlinsley
Copy link
Collaborator

🎉 This PR is included in version 2.12.1 🎉

The release is available on:

Your semantic-release bot 📦🚀

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants