facebookexperimental / Recoil

Recoil is an experimental state management library for React apps. It provides several capabilities that are difficult to achieve with React alone, while being compatible with the newest features of React.
https://recoiljs.org/
MIT License
19.5k stars 1.18k forks source link

Feature Request: option to refresh to not recursively refresh all upstream selectors #1677

Open wuweiweiwu opened 2 years ago

wuweiweiwu commented 2 years ago

from the docs https://recoiljs.org/docs/api-reference/core/useRecoilRefresher

It is currently a no-op to "refresh" an atom, it will retain its current state. Selectors will have their caches cleared. Because wrapper selectors are often used as abstractions, refreshing a selector will also recursively refresh the caches of all selectors that it depends on.

However, if those upstream selectors have not changed but is used from the refreshed selector it will once again refresh and refetch (if doing some data fetching). IMO a good solution is an option that we can pass in to refresh to not refresh upstream selectors especially since its also a common use case. the current behavior would cause a cascade of refreshes when a lot of them are unnecessary

drarmstr commented 2 years ago

Yes, we decided to start conservatively and invalidate all upstream selectors (including all upstream selectors for all possible values of dependencies). This default avoids missing refreshes if queries are performed from upstream selectors or secretly missing refreshes if wrapper selectors are added in abstraction layers, which is a common practice.

We discussed different options for minimizing the set of invalidations, but it hasn't been a priority, so PRs are welcome. That's because often when you want this much control over using Recoil as a local cache for remote mutable or changing data it can be better to use an atom with Atom Effects.

wuweiweiwu commented 2 years ago

yeah i definitely understand where the team is coming from!

I kinda stumbled onto this from https://recoiljs.org/docs/guides/asynchronous-data-queries/#retry-query-from-error-message

brainstorming a bit here:

is it possible for snapshot.getNodes_UNSTABLE to return info on whether the selector is an asynchronous selector? Then perhaps we can iterate over the dependency notes and check if its async and then selectively invalidate. This would also require a new option in refresh, something like skipDependencies

a stopgap solution for me can be request id atomFamily to invalidate the correct selector. but as a part of that i would need to be able to read the parameters of a selectorFamily, is there an api that currently exposes that? (perhaps inside the snapshot api?). an example of a node key from snapshot.getNodes_UNSTABLE was timeUsedChartDataState__selectorFamily/{"spaceId":"1","timeZone":"America/Los_Angeles"}/24 but i feel like i should avoid manually parsing the json out of there 👀

what do you think @drarmstr?

drarmstr commented 2 years ago

Selectors may dynamically be synchronous or asynchronous depending on their input values, but I'm not sure that's the best criteria for determining if refreshing should be recursive or not. An option to recurse or not could be included in the refresh() API, but again it may silently break if the selector implementation adds a wrapper.

For that request ID pattern what has been used is to use another selectorFamily to produce the parameters. You can then access that from the actual query selector as well as other places where you may want to access them, such as for targeted refreshing. Or, if appropriate, you could just refresh the entire family.

wuweiweiwu commented 2 years ago

the selectorFamily request id pattern described makes sense. But I don't think it would cover the use-case where we want to refresh dynamic selectors that threw an exception in an error boundary. example from the docs

in that example i go through the snapshot and find the nodes that threw the error and then refresh() them. i would like to be able to selectively refresh(). I

think this would solve the "retry queries" problem without refreshing all upstream selectors. though it is true this may be confusing if the selector impl adds a wrapper (and thus not actually refreshing the target selector) but i think having documentation around that option as well it being default false should alleviate any issues

@drarmstr is the team open to having an option parameter to refresh() taking in an optional parameter shallow: boolean? if so, i'm happy to make that contribution.

thinking something like

refresh(node, { shallow: true })

or (because im slightly lazier)

refresh(node, true)
signorbusi commented 1 year ago

This feature would make my life so much easier right now. Any updates on this?

drarmstr commented 1 year ago

No work on this is currently planned, but we do accept PRs. In general I am personally wary of "refresh" and tend to recommend using atoms instead of selectors when users need more explicit control over updating values.