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

Add opt-in client-side caching layer to save on network requests #4386

Merged
merged 29 commits into from
Mar 4, 2020

Conversation

fzaninotto
Copy link
Member

@fzaninotto fzaninotto commented Feb 5, 2020

See rationale in #4385

What started as a simple enhancement resulted into a pretty large refactoring and optimization.

  • useGetList now stores data in the list reducer (no more in customQueries)
  • useGetList now has one cache for each set of parameter. This allows optimistic rendering (and cache) of lists when changing pagination, filters, and sorting
  • useListController now uses useGetList
  • useListController uses resources.[resourceName].list.ids when resources.[resourceName].list.cachedRequest[requestSignature] isn't defined (avoids showing an empty list when changing params)
  • No more Refresh when leaving optimistic mode. Instead, all dataProvider queries that were cancelled while in optimistic mode are replayed when leaving
  • When a dataProvider success response contains a validUntil key, the relevant cache in the Redux store is maked with this validity date
  • When a useDataProvider call has a valid cached response, the call to dataProvider is skipped altogether
  • Write requests and refresh reset the cache validity and force real calls to the dataProvider

The current implementation is opt-in (and backwards compatible): the dataProvider responses must include a validUntil key to enable the cache.

@fzaninotto
Copy link
Member Author

As there is a refresh side effect after the update of the editController, the cache currently does not work after an edition.

@fzaninotto fzaninotto changed the base branch from master to next February 17, 2020 07:33
The removes the need for a refresh() once the optimistic mode finishes, and allows better usage of the cache even in optimistic mode
@fzaninotto
Copy link
Member Author

fzaninotto commented Feb 17, 2020

Fixed the problem with the forced refresh after an undoable call, reverts #4058

This changes the way the list is displayed when updating params (sort, pargination, filter): the list empties first, to show that the request is loading, then displayes the updated results.
Now we can share cache between getList and getMatching!
@fzaninotto
Copy link
Member Author

Now we cache the list of ids per request, that means that we can use the cache (and optimistic rendering) when going back to a list (paginatinon, sort, and filters included) that was seen before.

@fzaninotto fzaninotto changed the title [POC] Use cache Add opt-in client-side caching layer to save on network requests Feb 18, 2020
@fzaninotto
Copy link
Member Author

The requests made by the <ReferenceInput> component aren't cached because their controller still uses the CrudGetMatchingAccumulate action creator. That means that the dataProvider.getList() it makes to fetch the possible references is done by the old fetch saga (called by the accumulate saga) instead of the useDataProvider hook. As the cache is only implemented in the new hook, <ReferenceInput> doesn't use the cache.

We need to migrate the useReferenceInputController code so that it doesn't use the accumulate saga anymore.

packages/ra-core/src/controller/useListController.ts Outdated Show resolved Hide resolved
switch (type) {
case 'getList':
return (
resourceState &&
Copy link
Contributor

Choose a reason for hiding this comment

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

could use lodash/get here:

get(resourceState, ['list', 'validity', JSON.stringify(payload as GetListParams)]);

Copy link
Member Author

Choose a reason for hiding this comment

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

Due to prettier, the code isn't super shorter, and due to lodash, it isn't more readable:

            // with lodash
            return (
                get(resourceState, [
                    'list',
                    'validity',
                    JSON.stringify(payload as GetListParams),
                ]) > now
            );

            // without lodash
            return (
                resourceState &&
                resourceState.list &&
                resourceState.list.validity &&
                resourceState.list.validity[
                    JSON.stringify(payload as GetListParams)
                ] > now
            );

In this case, I prefer a bit more verbosity with a bit less indirection.

case 'getList': {
const data = resourceState.data;
const requestSignature = JSON.stringify(payload);
const ids = resourceState.list.idsForQuery[requestSignature];
Copy link
Contributor

Choose a reason for hiding this comment

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

Strange, are we sure resourceState.list.idsForQuery is always set ?

Copy link
Member Author

Choose a reason for hiding this comment

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

yes: if the query key was set in validity, it means it was done in idsForQuery, too.


const initialState = {};

const validityReducer: Reducer<ValidityRegistry> = (
Copy link
Contributor

Choose a reason for hiding this comment

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

I think this would be better grouped with idsForQuery and totalForQuery to avoid having the same serialized query repeated three times though the state.

oldValidityRegistry: ValidityRegistry
): ValidityRegistry => {
const validityRegistry = { ...oldValidityRegistry };
ids.forEach(id => {
Copy link
Contributor

Choose a reason for hiding this comment

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

return ids.reduce((validityRegistry, id) => ({
    ...validityRegistry,
    [id]: validUntil
}), oldValidityRegistry);

Copy link
Member Author

Choose a reason for hiding this comment

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

The reduce version isn't shorter, and clones the registry once for every id. The original version clones the registry only once. I think the reduce version is slower and takes more memory. As it doesn't have the benefit of brevity, let's keep some imperative programming here.

// without reduce
const addIds = (
    ids: Identifier[] = [],
    validUntil: Date,
    oldValidityRegistry: ValidityRegistry
): ValidityRegistry =>
{
    const validityRegistry = { ...oldValidityRegistry };
    ids.forEach(id => {
        validityRegistry[id] = validUntil;
    });
    return validityRegistry;
};

// with reduce
const addIds = (
    ids: Identifier[] = [],
    validUntil: Date,
    oldValidityRegistry: ValidityRegistry
): ValidityRegistry =>
    ids.reduce(
        (validityRegistry, id) => ({
            ...validityRegistry,
            [id]: validUntil,
        }),
        oldValidityRegistry
    );

@fzaninotto fzaninotto added the RFR Ready For Review label Mar 2, 2020
@fzaninotto
Copy link
Member Author

switching to RFR

docs/Caching.md Outdated Show resolved Hide resolved
ids: ids || [],
hideFilter: queryModifiers.hideFilter,
ids: typeof ids === 'undefined' ? defaultIds : ids,
loaded: loaded || defaultIds.length > 0,
Copy link
Contributor

Choose a reason for hiding this comment

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

Why not using defaultTotal here ?

Copy link
Member Author

Choose a reason for hiding this comment

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

What's wrong with defaultIds?

Copy link
Contributor

Choose a reason for hiding this comment

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

Nothing wrong. It's just that defaultTotal > 0 seems lighter.

@fzaninotto fzaninotto added this to the 3.3.0 milestone Mar 4, 2020
@fzaninotto fzaninotto merged commit dbf3b97 into next Mar 4, 2020
@fzaninotto fzaninotto deleted the use-cache branch March 4, 2020 09:18
@fancyaction
Copy link
Contributor

The requests made by the <ReferenceInput> component aren't cached because their controller still uses the CrudGetMatchingAccumulate action creator. That means that the dataProvider.getList() it makes to fetch the possible references is done by the old fetch saga (called by the accumulate saga) instead of the useDataProvider hook. As the cache is only implemented in the new hook, <ReferenceInput> doesn't use the cache.

We need to migrate the useReferenceInputController code so that it doesn't use the accumulate saga anymore.

What's the status on this? Is this being tracked and/or worked on?

I hit a serious issue this week with our app using a new form that can load in with 20+ child line inputs (via ArrayInput/SimpleFormIterator). I implemented the new caching features but it's not a network issue -- it's a scripting issue and it looks like it's due to the old fetch saga. It really sinks the app and makes a borderline unusable user experience. I think the above issue might address what I'm dealing with. If I can find some free time, I plan to try to look into updating the useReferenceInputController and try my best.

@fzaninotto
Copy link
Member Author

This problem is still present, not being worked on, and tracked by issue #4531 that I just created thanks to your comment.

If you feel like giving it a try, we'd appreciate the help!

@gomcodoctor
Copy link

look like requestSignature used in useGetList for cachedRequests is different than one sent in action in requestPayload

const requestSignature = JSON.stringify({ pagination, sort, filter });

in requestSignature filter variable do not have navigation and sort data, but in requestPayload its there.
due to this useGetList will never return cached data

@fzaninotto
Copy link
Member Author

@gomcodoctor I don't know what you're talking about. The request signatures computed in useGetList and in the cachedRequests reducer are the same for me. If you've found a bug, please open a new issue with a way to reproduce it.

@gomcodoctor
Copy link

I will create a new issue.. I think it's bug

@AymanElarian
Copy link

AymanElarian commented Aug 26, 2020

The requests made by the <ReferenceInput> component aren't cached because their controller still uses the CrudGetMatchingAccumulate action creator. That means that the dataProvider.getList() it makes to fetch the possible references is done by the old fetch saga (called by the accumulate saga) instead of the useDataProvider hook. As the cache is only implemented in the new hook, <ReferenceInput> doesn't use the cache.
We need to migrate the useReferenceInputController code so that it doesn't use the accumulate saga anymore.

What's the status on this? Is this being tracked and/or worked on?

I hit a serious issue this week with our app using a new form that can load in with 20+ child line inputs (via ArrayInput/SimpleFormIterator). I implemented the new caching features but it's not a network issue -- it's a scripting issue and it looks like it's due to the old fetch saga. It really sinks the app and makes a borderline unusable user experience. I think the above issue might address what I'm dealing with. If I can find some free time, I plan to try to look into updating the useReferenceInputController and try my best.

@fancyaction
did you found any solution yet , I face the same issue ?

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.

6 participants