reduxjs / redux-toolkit

The official, opinionated, batteries-included toolset for efficient Redux development
https://redux-toolkit.js.org
MIT License
10.64k stars 1.15k forks source link

[RTK Query] Expose "abort" from useQuery hook #3218

Open ivan-jelev opened 1 year ago

ivan-jelev commented 1 year ago

There is cases when abort might be useful to use while using useQuery hook to stop chain of queryFn requests, polling or long running tasks.

Proposal

Expose abort method from useQuery hook same as we do with "refetch"

Use case

Inside queryFn I have a polling which submit the task (call A - once) and then poll task result by task id (call B - multiple with internal). Basically I need to abort data polling when it's not needed any more, for example on component unmount.

Currently we have to use useLazyQuery to achieve this:

const [trigger, { data, error }] = useLazyQuery();

useEffect(() => {
  const { abort, unsubscribe } = trigger();

  return () => {
    abort();
    unsubscribe();
  }
}, [])

What we want to achieve:

const { abort, data, error } = useQuery();

useEffect(() => abort, [])

Or ideally it would be great if abort would happen by default on unmount internally so we don't have to do this manually... or controlled by additional option prop like "abortOnUnmount: boolean, abortOnArgsChange: boolean"

phryneas commented 1 year ago

Usually, you don't want to abort queries in that situation.

First, you don't gain a lot by the abort: the server already got the request and started processing it - you cannot stop the server anymore in most setups. All you gain is that the data is no longer sent to the client.

But you lose things: that cache entry will go into an error state on abort.
If that cache entry already had valid data before (because you might be refreshing right now), that "before" data is probably lost to you because you get into an error state and will likely render an error message the next time you try to access it. Also, you "lose" data that has already been transmitted - if you need that data again, you will have to completely resend it, even if it might already have been transmitted 90%.

Why not just let the data come in fully and have them in a cache entry in case you need it again within the next 60 seconds or so?

LukerSpringtree commented 1 year ago

There are all sorts of problems with export abort function, but we want to have this feature that the type questioner is talking about. In our project, we have some bug with this.

berci-i commented 1 year ago

Hi @phryneas ,

We also kind of need this. We have an endpoint for loading a card data, but some cards have more information than others and there is a lot of calculation involved on the BE. Because of this another card might be selected and the data for the second one might load before the data for the first one. In this case the matchFulfilled Matcher will get called again and instead of the data for the selected card there is a part of the app where the data displayed will be the one from the first card (which will replace the data from the second request).

phryneas commented 1 year ago

@berci-i wouldn't it make more sense to keep track of the arguments you have sent with the last request, and only perform work in the extraReducer if those arguments match up?

berci-i commented 1 year ago

Thank you for the suggestion @phryneas For this specific specific case it wouldn't, because we are doing several request for a card. For example we want to get some buildings data and also some data for the first version for that scenario (but there can also be the case where the version is changed). This logic is kept in separate reducers and also the args are different. So the ideea is that we are doing the requests from one reusable component and cancelling them would be relatively easy, but keeping track will involve more data or variables carried around.

I guess we could use something like localStorage in order to at least avoid carrying a variable around and/or adding an extra key in the reducer just for this. However it would have been a little easier for this case if we could just abort the requests. But I am curios however: do you see any drawback in using something like localStorage for this?

Thanks again!

phryneas commented 1 year ago

I'd avoid any localStorage usage here (it doesn't have any benefit over a variable, and you should use neither of those, and keep your reducers pure). Can't you just track the "latest" request when you see pending actions coming in? All of those requests do have a request id.

I really don't want to expose abort - as I said it places the components in a weird error state and people will shoot themselves in the foot with it.

berci-i commented 1 year ago

@phryneas I understand the exposing part. I am not 100% sure what you mean with track the "latest" request when you see pending actions coming in tough 😅 Something like having a global variable in the slice file and setting it on matchPending + checking on matchFulfilled if is the same? like:

let lastRequestId: number
export const mySlice = createSlice({
  ...
  extraReducers: (builder) => {
    builder.addMatcher(myApi.endpoints.getData.matchPending, (state, action) => {
      lastRequestId = action.meta.requestId
    })
    builder.addMatcher(myApi.endpoints.getData.matchFulfilled, (state, action) => {
       if(lastRequestId == action.meta.requestId)  {
         ...
       }
    })

Many thanks! 🙏🏽

phryneas commented 1 year ago

Yup, something like that.

WillisShek commented 1 year ago

I got an actual use case that the abort is needed.

My team comes across a case that a refetch is called after a mutation is fired. The thing is that if there is already a pending query, the refetch after the mutation won't be fired. It causes the result data being not accurate due to it not including the mutation it just did.

phryneas commented 1 year ago

@WillisShek could this also be solved with #3116?

AsuraKev commented 9 months ago

I notice some difference in terms of query cancellation between react-query and RTK Q.

In react query whenever a query consumes abortsignal, the query is automatically aborted on unmount or a new request is started while the prev one is running.

RTK Q doesnt seem to have this built in instead we ll have to use Lazy query and plus a useeffect to handle this :/

phryneas commented 9 months ago

@AsuraKev Yes, we don't do that by default because we think in most cases it's not a good idea to do this. The request has been sent out, the server has started working on a response, we probably already received part of the response. Why throw that away instead of accepting and caching it?

AsuraKev commented 9 months ago

Hmm sometimes user clicks fast and if we don’t cancel prev requests the requests could arrive out of order. Also raising abort signal means our API could terminate server processing as well which means we are not wasting resources on the server side.

So I ll take it that if we wanna automatic cancel running query, the only way to achieve this is via lazy query?

thanks 😀

markerikson commented 9 months ago

@AsuraKev : even if the requests arrive out of order, the component that's asking for the data will only see the cache entry that it's asking for. Same as if there's 100 cache entries for different arguments to useGetPokemonQuery(somePokemon), but only the value for useGetPokemonQuery('pikachu') is being used in the component.

AsuraKev commented 9 months ago

I see, thanks for clarifying 😃

ensconced commented 6 days ago

It would be great (to have the option) to abort requests when the response is no longer needed (i.e. when the params have changed, or there are no subscribers). In some cases, mine included, processing the request on the backend can be very expensive in terms of time and/or money and aborting the request would allow the backend to detect the connection having been closed, and to stop the expensive processing.

EDIT - this is a slightly different request so I have opened a new issue