autorefresh tasks for a courier #1840
Conversation
vladimir-8
changed the title
This reverts commit a41cd7e.
src/redux/api/baseQuery.js
Outdated
|
|
||
| // checking whether the mutex is locked | ||
| if (mutex.isLocked()) { | ||
| // wait until the mutex is available without locking it |
There was a problem hiding this comment.
if i understand well we could add the following comment :
// wait for the mutex release, i.e. wait that another request that needed a fresh token gets the new token for us and update the state accordingly
?
| subscriptionGenerateOrders: builder.mutation({ | ||
| query: date => ({ | ||
| url: 'api/recurrence_rules/generate_orders', | ||
| params: { | ||
| date: date.format('YYYY-MM-DD'), | ||
| }, | ||
| method: 'POST', | ||
| body: {}, | ||
| }), | ||
| }), | ||
| getMyTasks: builder.query({ | ||
| query: date => `api/me/tasks/${date.format('YYYY-MM-DD')}`, | ||
| }), | ||
| getOrderTiming: builder.query({ | ||
| query: nodeId => `${nodeId}/timing`, | ||
| }), | ||
| getOrderValidate: builder.query({ | ||
| query: nodeId => `${nodeId}/validate`, | ||
| }), | ||
| updateOrder: builder.mutation({ | ||
| query: ({ nodeId, ...patch }) => ({ | ||
| url: nodeId, | ||
| method: 'PUT', | ||
| body: patch, | ||
| }), |
There was a problem hiding this comment.
I am surprised by the functions that are not getMyTasks
- they are not linked to the PR
- they are not explictly called? or maybe they will be called on onFocus/onOnline... aso, but it seems weird to me for example for
updateOrder? - we should build apiSlice differently according to the user roles? (getMytasks is for courier, generateOrders for dispatchers, updateorder for admin/restaurant owner ?
There was a problem hiding this comment.
It's actually a copy-paste of https://github.com/coopcycle/coopcycle-web/blob/master/js/app/api/slice.js with getMyTasks added. I mentioned elsewhere that, ideally, I see it in a library, but for now, I think we should try to make them compatible so it's easy to copy-paste already implemented endpoints between the web and the app
- not directly
- no, they are not used at the moment, but will be soon. In a timing modal, for example
- I thought about it, but rkt query actually advises against it (see Tip): https://redux-toolkit.js.org/rtk-query/api/createApi, so I decided to stick to the standard approach
There was a problem hiding this comment.
- i am not sure we understand the same, i was thinking to generate dynamically the slice upon login so you don't load the automatic refetch for tasks for a customer for example
What would happen with the current code if a simple customer logs in for example? will it try to refetch tasks (hence 401) on focus? I can test that later
There was a problem hiding this comment.
No, at least as I understand. Refetch happens only in TasksPage component
setupListenersReactNative https://github.com/coopcycle/coopcycle-app/blob/f7667c26a9c822427f6ef30aea845baf59f7436a/src/redux/setupListenersReactNative.js only tell rtk about the state changes, but they don't trigger any requests until they are configured elsewhere
There was a problem hiding this comment.
hello,
i tested the behavior locally and everything seems alright 👍 it is a huge improvment 👍 and now we have that we can add easily same behavior for restaurant part and solve that much problem
my main interrogation is the apiSlice.endpoints. to me you are mixing endpoints that belongs to different categories of users, and instead we should have one slice for restaurant owner, one for riders, one for dispatcher, one for admin... and combine them according to the user's role?
also can we have fine-grained control on what will be refetched onFocus when we use the global setupListeners ? (i can check myslef if you dont know)
Each component can define the requests that they want to get refetched. It's defined here, if that's what you are asking: |
|
ah ok i had a misunderstanding of the RTK doc I think, because when I read it I thought the behavior will be enabled by default for all endpoints when calling maybe change your comment |
| render() { | ||
| const { tasks, tasksWithColor, selectedDate } = this.props; | ||
| const { isFetching, refetch } = useGetMyTasksQuery(selectedDate, { | ||
| refetchOnFocus: true, |
There was a problem hiding this comment.
@vladimir-8 we should enable refetchOnReconnect here as well no ?
There was a problem hiding this comment.
We can do it, why not
| const { tasks, tasksWithColor, selectedDate } = this.props; | ||
| const { isFetching, refetch } = useGetMyTasksQuery(selectedDate, { | ||
| refetchOnFocus: true, | ||
| }); |
There was a problem hiding this comment.
@vladimir-8 also more out of curiosity
with apiSlice i get the impression there is only one centralized thing that handles the fetching, here for the rider tasks
but what if i call the hook from several screens with different configurations (like not the same refetchOnfocus)? each screen/hook usage has its own config or they override each other (I guess that would be bad :rire:)
There was a problem hiding this comment.
As I understand, all screens with refetchOnfocus == true will try to fetch when they get focused, so we should be careful with using this option, otherwise we end up with a lot of requests. If refetchOnfocus == false then there will be only one request and all other screens should get the data from the cache: https://redux-toolkit.js.org/rtk-query/usage/cache-behavior#default-cache-behavior
I think, ideally, it would be cool to set a lifetime on each item in the cache, like 60 seconds, in that case we don't need refetchOnfocus we will just drop the old cached data and trigger a new request. I don't see any build-in ways to do it, so it seems we need to modify https://redux-toolkit.js.org/rtk-query/api/createApi#onquerystarted method ourselves
There was a problem hiding this comment.
There is keepUnusedDataFor https://redux-toolkit.js.org/rtk-query/usage/cache-behavior#reducing-subscription-time-with-keepunuseddatafor option, but I guess a component does not unsubscribe when it loses focus. If it's the case, then it will not work for us
Fixes: #1807