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

Update refresh strategy to avoid empty page while refreshing #6054

Merged
merged 2 commits into from
Mar 26, 2021
Merged

Conversation

fzaninotto
Copy link
Member

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.

refrsh_broken

The root cause is that a refresh triggers many effects:

  1. it forces a rerender of the root component (because we use key={version})
  2. it forces a refetch of all the useDataProvider-based hooks used in the screen (because the version is in the effect signature)
  3. it empties the Redux store to force a refetch even though the application cache is on (the application cache skips the call to the actual dataProvider when a valid response from a previous cal lis in the Redux store)

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.

refresh_fixed

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.

@fzaninotto fzaninotto added the RFR Ready For Review label Mar 18, 2021
*/
const useRefresh = () => {
const dispatch = useDispatch();
return useCallback(
(doRefresh = true) => doRefresh && dispatch(refreshView()),
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this doRefresh parameter was undocumented and unused. Also, it's useless - one can replace it easily:

-refresh(doRefresh);
+doRefresh && refresh()

*/
const useRefresh = () => {
const dispatch = useDispatch();
return useCallback(
(doRefresh = true) => doRefresh && dispatch(refreshView()),
(hard?: boolean) => {
dispatch(hard ? refreshView() : softRefreshView());
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I wondered if there should be two different actions or one action with a variable payload. I'm open to discussion on this point

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think a single action is enough, with a type option which can be either hard or soft (for now)

@@ -35,6 +39,17 @@ const cachedRequestsReducer: Reducer<State> = (
// force refresh
return initialState;
}
if (action.type === SOFT_REFRESH_VIEW) {
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is the key difference: a soft_refresh doesn't empty the cached requests, only invalidates them

Copy link
Collaborator

@djhi djhi left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do we still have use cases for hard refreshes ?

@fzaninotto
Copy link
Member Author

Do we still have use cases for hard refreshes ?

I don't have any in mind, but as it was the previous default, I think we must keep that possibility.

@fzaninotto fzaninotto added this to the 3.14 milestone Mar 18, 2021
@djhi djhi merged commit 47f3c5d into next Mar 26, 2021
@djhi djhi deleted the soft-refresh branch March 26, 2021 15:02
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
RFR Ready For Review
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants