Update refresh strategy to avoid empty page while refreshing #6054
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
fzaninotto
commented
Mar 18, 2021
| */ | ||
| const useRefresh = () => { | ||
| const dispatch = useDispatch(); | ||
| return useCallback( | ||
| (doRefresh = true) => doRefresh && dispatch(refreshView()), |
There was a problem hiding this comment.
this doRefresh parameter was undocumented and unused. Also, it's useless - one can replace it easily:
-refresh(doRefresh);
+doRefresh && refresh()
fzaninotto
commented
Mar 18, 2021
| */ | ||
| const useRefresh = () => { | ||
| const dispatch = useDispatch(); | ||
| return useCallback( | ||
| (doRefresh = true) => doRefresh && dispatch(refreshView()), | ||
| (hard?: boolean) => { | ||
| dispatch(hard ? refreshView() : softRefreshView()); |
There was a problem hiding this comment.
I wondered if there should be two different actions or one action with a variable payload. I'm open to discussion on this point
There was a problem hiding this comment.
I think a single action is enough, with a type option which can be either hard or soft (for now)
fzaninotto
commented
Mar 18, 2021
| @@ -35,6 +39,17 @@ const cachedRequestsReducer: Reducer<State> = ( | |||
| // force refresh | |||
| return initialState; | |||
| } | |||
| if (action.type === SOFT_REFRESH_VIEW) { | |||
There was a problem hiding this comment.
This is the key difference: a soft_refresh doesn't empty the cached requests, only invalidates them
djhi
reviewed
Mar 18, 2021
I don't have any in mind, but as it was the previous default, I think we must keep that possibility. |
djhi
approved these changes
Mar 26, 2021
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Problem
On pages built by hand using the
useQueryWithStore-based hooks (e.g.useGetList), a refresh triggered by the Refresh button or by a side effect causes the page content to briefly disappear. This is confusing.The root cause is that a refresh triggers many effects:
key={version})useDataProvider-based hooks used in the screen (because theversionis in the effect signature)The 3rd effect causes the page blank: for a brief moment, the store has no data.
Solution
The 3rd effect is too strong. We don't need to actually empty the store to empty the application cache. We just need to remove the validity dates for all recorded data.
So I replaced the refresh by a softRefresh, which does 1, 2 but not 3 - instead, it only empties the application cache.
Default refresh calls (side effects, RefreshButton) now use the softRefresh. Users can still call the ancient ('hard') refresh by dispatching the right action.
This change isn't completely backwards compatible, but I fail to see legitimate use of the old (broken) code. See my comments in the code.