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

RTQK: Better manual cache updates or clearer info on limitations (eg first query errored, resetApiState) #1529

Closed
PEsteves8 opened this issue Sep 20, 2021 · 8 comments

Comments

@PEsteves8
Copy link

Hi,

I noticed that the docs are being changed to better convey how to perform manual cache updated here 1525, but I feel that the current implementation can be a bit misleading and lead devs to unwittingly create bugs.

I think that most people think of manual cache updates as changing a cache entry without triggering a new query request (ie invalidating tags), and for the most part this is possible.

However, if the first query request errored, or the entry hasn't been created, or the dev calls resetApiState, then truly manual cache updates will all fail.

Here's a use case. Let's say that the dev is using rtkq to manage the current user session (maybe user sessions isn't the most common use case, but I'm sure other cases could have similar issues):
1 - Presumably, on app load there'll be a getCurrentSession request.
2 - The user logs in and the request returns the session data. The dev doesn't want to invalidate the getCurrentSession tag and only wants to replace the cache entry with the data from the login request.
3 - Now the problem is that if the first getCurrentUser request for some reason failed (the cache has no data, only the error property) or if on logout the dev calls resetApiState, manual cache updates won't work and login will be broken.

This in some sense also breaks optimistic updates, technically the data will show up, but it only after tag invalidation and its respective request, which could be a confusing behavior.

I'm aware that the docs do indicate 'if a cache entry exists', but it might not be obvious that this applies to a query that errored on first (only) request and after resetApiState calls. It's very easy to assume that manual cache updates would always work no matter what.

  • So basically I'm wondering if updating a cache entry that has errored or doesn't exist is something that might be added as a feature (if I recall correctly this is supported by react-query).
  • Alternatively, perhaps the docs should indicate more clearly that manual cache updates won't work quite as expected if the cache entry errored on first request or doesn't exist (either due to never having been called or due to a resetApiState call) and that you should always use invalidatingTags whatever strategy you use to update cache entries.

Anyway, thanks for you work.

@PEsteves8 PEsteves8 changed the title RTQK: Better manual cache updates RTQK: Better manual cache updates or clearer info on limitations (eg first error query errored, resetApiState) Sep 20, 2021
@PEsteves8 PEsteves8 changed the title RTQK: Better manual cache updates or clearer info on limitations (eg first error query errored, resetApiState) RTQK: Better manual cache updates or clearer info on limitations (eg first query errored, resetApiState) Sep 20, 2021
@msutkowski
Copy link
Member

Hi,

I noticed that the docs are being changed to better convey how to perform manual cache updated here 1525, but I feel that the current implementation can be a bit misleading and lead devs to unwittingly create bugs.

I think that most people think of manual cache updates as changing a cache entry without triggering a new query request (ie invalidating tags), and for the most part this is possible.

However, if the first query request errored, or the entry hasn't been created, or the dev calls resetApiState, then truly manual cache updates will all fail.

Here's a use case. Let's say that the dev is using rtkq to manage the current user session (maybe user sessions isn't the most common use case, but I'm sure other cases could have similar issues):
1 - Presumably, on app load there'll be a getCurrentSession request.
2 - The user logs in and the request returns the session data. The dev doesn't want to invalidate the getCurrentSession tag and only wants to replace the cache entry with the data from the login request.
3 - Now the problem is that if the first getCurrentUser request for some reason failed (the cache has no data, only the error property) or if on logout the dev calls resetApiState, manual cache updates won't work and login will be broken.

This in some sense also breaks optimistic updates, technically the data will show up, but it only after tag invalidation and its respective request, which could be a confusing behavior.

I'm aware that the docs do indicate 'if a cache entry exists', but it might not be obvious that this applies to a query that errored on first (only) request and after resetApiState calls. It's very easy to assume that manual cache updates would always work no matter what.

  • So basically I'm wondering if updating a cache entry that has errored or doesn't exist is something that might be added as a feature (if I recall correctly this is supported by react-query).
  • Alternatively, perhaps the docs should indicate more clearly that manual cache updates won't work quite as expected if the cache entry errored on first request or doesn't exist (either due to never having been called or due to a resetApiState call) and that you should always use invalidatingTags whatever strategy you use to update cache entries.

Anyway, thanks for you work.

Thanks for your feedback! I understand your concerns, but from a documentation clarity perspective, I have no idea of how the following scenario could be any more clearly defined as it's spelled out in the docs as well as in the type hints + jsdoc blocks.

I'm aware that the docs do indicate 'if a cache entry exists', but it might not be obvious that this applies to a query that errored on first (only) request and after resetApiState calls. It's very easy to assume that manual cache updates would always work no matter what.

In those scenarios, it should be very obvious that there is no cache entry because you: 1. never had cached data due to a failed request, and 2. you reset the api state (in this case, you should be aware that you called resetApiState yourself?)

I do believe the docs mention the pitfalls and encourage preferring tag invalidation over manual cache entries. IMO, these are just tools that can be used in certain scenarios.

If we were to support a 'force create entry' type of feature, what would you imagine that API looked like?

@PEsteves8
Copy link
Author

PEsteves8 commented Sep 20, 2021

Hi, thanks for the response.

I suppose I might've gotten a bit too used to react-query's approach.

If we were to support a 'force create entry' type of feature, what would you imagine that API looked like?

Well from the end dev point of view, ideally it would look similar to react-query. So for context, here's a query in react-query:

const { data } = useQuery('todos', () => axios.get('api/v1/auth'));

The equivalent to invalidateTags is invalidateQueries, like this:

 const mutation = useMutation(addTodo, {
   onSuccess: () => {
     queryClient.invalidateQueries('todos')
   },
 })

The difference being that this sort of thing goes in an onSuccess callback, instead of a tags array.

And just setting the query manually, whether or not it exists just looks like this:

 const mutation = useMutation(addTodo, {
   onSuccess: (data) => {
     queryClient.setQueryData('todos', data);
   },
 })

(naturally, these hooks can then be set up as custom hooks to avoid repeating this code on each component that uses the query)

Here's their docs: https://react-query.tanstack.com/reference/QueryClient#queryclientsetquerydata

"setQueryData is a synchronous function that can be used to immediately update a query's cached data. If the query does not exist, it will be created."

"After successful changing query's cached data via setQueryData, it will also trigger onSuccess callback from that query."

This approach, solves any possible issues related to failed requests or reset caches. The query is considered successful and (if I recall correctly), the isLoading doesn't trigger on new requests.

So they try to be a bit agnostic about whether the user prefers to invalidateQueries or set it manually with setQueryData, which adds a lot of flexibility, and in my opinion, makes the behavior a lot more consistent, than just updating a resource if a request has been made and was successful in the past.

Of couse, different tools have different philosophies and react-query may just want to have a broader scope by not focusing necessarily on a request based approach (although, I would've expected rtk-query to also follow that route given that it's linked to redux).

As for how the api would work internally, unfortunately I'm not yet well versed enough in rtk-query to be able to give a good answer.

I can't say for sure if a lot of people would want to have this in the lib to warrant consideration, but I get the impression that many people would want it.

EDIT: Just noticed RTKQ uses the query name for the updateQueryData call, although it uses tags for query invalidation. react-query uses the equivalent of tags for all of these operations. I wonder if the invalidatesTags array could also just use the query name. Or alternatively if updateQueryData could just use tags. Anyway, this is a totally different conversation. I was just kind of thinking out loud. I suppose it's likely there are limitations that prevent this sort of thing.

@phryneas
Copy link
Member

Let's start from the beginning: updateQueryData updates the data of a query. When there is no data, there is nothing to update. It doesn't really matter why where is no data - because it never was there or because your deliberately removed it: it is not there, it cannot be updated.

I had initially a second argument that would take another callback to calculate a potential initial value, but I removed it again.

Because honestly, this seems very smelly to me.

RTK Query is an API cache, in the sense that it always should just mimic the data that the server has access to. It is not meant to store any data beyond that. And the main way of getting that data into the cache should always be by just getting it from the server.
The server is also always right: the next response from the server will always throw away any manually added data or manual modifications to existing data.

If you start adding data in there manually, it kinda feels that you are not using it as a cache, but as some kind of weird key-value-store. And when you want any kind of global store, it is right at your fingertips: Redux.
The "authentication" example above is essentially a prime example of something where you would just make a little slice for it and just save that data to the side.

There will be a way to "put" data into the store, using the rehydration options, but this is also explicitly not meant to get any hand-created data into the store, but just to restore data that already was in there in the past.

@PEsteves8
Copy link
Author

I get your point. It's just that it's common for people to use the result of a POST request, without triggering a new GET request for that resource. After all, what the POST returns also comes from the server, it doesn't have to be "hand created data".

I personally enjoy having the option of using it as a kind of key-value-store. In certain, even if uncommon, situations I can set a query using the result of a POST request, and only have it fetched if that page is being loaded. I wouldn't want to manage a request version with the highly convenient custom hooks, while at the same time managing state using the rest of the redux api just because I might not want to use a request. I would want the hooks to work for any case (which is the case with react-query).

Anyway, I guess this comes down to personal preference and library philosophy.

I only understood clearly that cache entries are only created with a successful request (and only then would updateQueryData work), until I experimented with the code. Not sure if this can be a common misconception, but if it is, it might be helpful to have a note on the docs indicating that, for pessimistic updates, one shouldn't refrain from invalidating tags as relying on manual updates alone may not work as expected in certain edge cases.

Anyway, thanks for your help :)

@PEsteves8
Copy link
Author

PEsteves8 commented Oct 5, 2021

There's also another somewhat related edge case that might be relevant. If an api returns null or empty body successfully, rkt-query will still set the isLoading boolean to true the next time it performs a query. Any empty response will result in data being undefined.

Again, it's an edge case. One case that comes to mind is firebase's currentUser request that returns null if no user is logged in. It's certainly not exactly what rtk-query was built for, but I wonder if similar situations might come up.

And also, you wouldn't be able to manually update the cache.

@phryneas
Copy link
Member

phryneas commented Oct 5, 2021

From the back of my head, that should be fixed in 1.7 and only show that behaviour on undefined, but not on null - and undefined cannot come from a JSON.parse call.

@MaybeSacred
Copy link

An additional use case: with server-side push/WebSockets, we could potentially receive new entities that haven't been explicitly added into the cache as the result of a query. Attempting to add them is a no-op currently

@markerikson
Copy link
Collaborator

@MaybeSacred There's a PR in the works for this: #2266 . We'd appreciate any feedback you have on the API design and behavior!

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

No branches or pull requests

5 participants