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

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

Open
ivan-jelev opened this issue Feb 28, 2023 · 16 comments
Open

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

ivan-jelev opened this issue Feb 28, 2023 · 16 comments
Milestone

Comments

@ivan-jelev
Copy link

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"

@ivan-jelev ivan-jelev changed the title RTK-Query: expose "abort" from useQuery hook [RTK Query] Expose "abort" from useQuery hook Feb 28, 2023
@phryneas
Copy link
Member

phryneas commented Mar 5, 2023

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
Copy link

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
Copy link

berci-i commented May 16, 2023

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
Copy link
Member

@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
Copy link

berci-i commented May 16, 2023

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
Copy link
Member

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
Copy link

berci-i commented May 17, 2023

@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
Copy link
Member

Yup, something like that.

@WillisShek
Copy link

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
Copy link
Member

@WillisShek could this also be solved with #3116?

@markerikson markerikson added this to the 1.9.x milestone Aug 16, 2023
@AsuraKev
Copy link

AsuraKev commented Nov 28, 2023

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
Copy link
Member

@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
Copy link

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
Copy link
Collaborator

@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
Copy link

I see, thanks for clarifying 😃

@markerikson markerikson modified the milestones: 1.9.x, 2.x bugfixes Dec 6, 2023
@ensconced
Copy link

ensconced commented Sep 9, 2024

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

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

No branches or pull requests

8 participants