From f0e60a15561da7460bb82c93194961a4231e0ec6 Mon Sep 17 00:00:00 2001 From: fzaninotto Date: Thu, 11 Feb 2021 18:41:50 +0100 Subject: [PATCH 1/7] Fix List error when deleting record with non-robut field --- .../src/controller/useListController.ts | 60 +----- packages/ra-core/src/dataProvider/index.ts | 2 + .../src/dataProvider/useGetMainList.tsx | 180 ++++++++++++++++++ 3 files changed, 192 insertions(+), 50 deletions(-) create mode 100644 packages/ra-core/src/dataProvider/useGetMainList.tsx diff --git a/packages/ra-core/src/controller/useListController.ts b/packages/ra-core/src/controller/useListController.ts index 9eff724477e..c2bab23e8d0 100644 --- a/packages/ra-core/src/controller/useListController.ts +++ b/packages/ra-core/src/controller/useListController.ts @@ -1,15 +1,13 @@ import { isValidElement, ReactElement, useEffect, useMemo } from 'react'; import inflection from 'inflection'; import { Location } from 'history'; -import { useSelector } from 'react-redux'; -import get from 'lodash/get'; import { useCheckMinimumRequiredProps } from './checkMinimumRequiredProps'; import useListParams from './useListParams'; import useRecordSelection from './useRecordSelection'; import useTranslate from '../i18n/useTranslate'; import useNotify from '../sideEffect/useNotify'; -import useGetList from '../dataProvider/useGetList'; +import { useGetMainList } from '../dataProvider/useGetMainList'; import { SORT_ASC } from '../reducer/admin/resource/list/queryReducer'; import { CRUD_GET_LIST } from '../actions'; import defaultExporter from '../export/defaultExporter'; @@ -18,7 +16,6 @@ import { SortPayload, RecordMap, Identifier, - ReduxState, Record, Exporter, } from '../types'; @@ -53,8 +50,6 @@ const defaultSort = { order: SORT_ASC, }; -const defaultData = {}; - export interface ListControllerProps { basePath: string; currentSort: SortPayload; @@ -153,7 +148,9 @@ const useListController = ( * We want the list of ids to be always available for optimistic rendering, * and therefore we need a custom action (CRUD_GET_LIST) that will be used. */ - const { ids, total, error, loading, loaded } = useGetList( + const { ids, data, total, error, loading, loaded } = useGetMainList< + RecordType + >( resource, { page: query.page, @@ -181,41 +178,12 @@ const useListController = ( } ); - const data = useSelector( - (state: ReduxState): RecordMap => - get( - state.admin.resources, - [resource, 'data'], - defaultData - ) as RecordMap - ); - - // When the user changes the page/sort/filter, this controller runs the - // useGetList hook again. While the result of this new call is loading, - // the ids and total are empty. To avoid rendering an empty list at that - // moment, we override the ids and total with the latest loaded ones. - const defaultIds = useSelector((state: ReduxState): Identifier[] => - get(state.admin.resources, [resource, 'list', 'ids'], []) - ); - const defaultTotal = useSelector((state: ReduxState): number => - get(state.admin.resources, [resource, 'list', 'total']) - ); - - // Since the total can be empty during the loading phase - // We need to override that total with the latest loaded one - // This way, the useEffect bellow won't reset the page to 1 - const finalTotal = typeof total === 'undefined' ? defaultTotal : total; - - const finalIds = typeof total === 'undefined' ? defaultIds : ids; - - const totalPages = useMemo(() => { - return Math.ceil(finalTotal / query.perPage) || 1; - }, [query.perPage, finalTotal]); + const totalPages = Math.ceil(total / query.perPage) || 1; useEffect(() => { if ( query.page <= 0 || - (!loading && query.page > 1 && (finalIds || []).length === 0) + (!loading && query.page > 1 && ids.length === 0) ) { // Query for a page that doesn't exist, set page to 1 queryModifiers.setPage(1); @@ -224,15 +192,7 @@ const useListController = ( // It occurs when deleting the last element of the last page queryModifiers.setPage(totalPages); } - }, [ - loading, - query.page, - finalIds, - queryModifiers, - total, - totalPages, - defaultIds, - ]); + }, [loading, query.page, ids, queryModifiers, total, totalPages]); const currentSort = useMemo( () => ({ @@ -262,8 +222,8 @@ const useListController = ( filterValues: query.filterValues, hasCreate, hideFilter: queryModifiers.hideFilter, - ids: finalIds, - loaded: loaded || defaultIds.length > 0, + ids, + loaded: loaded || ids.length > 0, loading, onSelect: selectionModifiers.select, onToggleItem: selectionModifiers.toggle, @@ -277,7 +237,7 @@ const useListController = ( setPerPage: queryModifiers.setPerPage, setSort: queryModifiers.setSort, showFilter: queryModifiers.showFilter, - total: finalTotal, + total: total, }; }; diff --git a/packages/ra-core/src/dataProvider/index.ts b/packages/ra-core/src/dataProvider/index.ts index e55780331d3..697e61c8f61 100644 --- a/packages/ra-core/src/dataProvider/index.ts +++ b/packages/ra-core/src/dataProvider/index.ts @@ -13,6 +13,7 @@ import useQueryWithStore, { QueryOptions } from './useQueryWithStore'; import withDataProvider from './withDataProvider'; import useGetOne, { UseGetOneHookValue } from './useGetOne'; import useGetList from './useGetList'; +import { useGetMainList } from './useGetMainList'; import useGetMany from './useGetMany'; import useGetManyReference from './useGetManyReference'; import useGetMatching from './useGetMatching'; @@ -45,6 +46,7 @@ export { useQuery, useGetOne, useGetList, + useGetMainList, useGetMany, useGetManyReference, useGetMatching, diff --git a/packages/ra-core/src/dataProvider/useGetMainList.tsx b/packages/ra-core/src/dataProvider/useGetMainList.tsx new file mode 100644 index 00000000000..4560a4ce3b4 --- /dev/null +++ b/packages/ra-core/src/dataProvider/useGetMainList.tsx @@ -0,0 +1,180 @@ +import { useMemo, useRef } from 'react'; +import get from 'lodash/get'; + +import { + PaginationPayload, + SortPayload, + ReduxState, + Identifier, + Record, + RecordMap, +} from '../types'; +import useQueryWithStore from './useQueryWithStore'; + +const defaultIds = []; +const defaultData = {}; + +/** + * Call the dataProvider.getList() method and return the resolved result + * as well as the loading state. + * + * Uses a special cache to avoid showing an empty list while re-fetching the + * list after changing params. + * + * The return value updates according to the request state: + * + * - start: { loading: true, loaded: false } + * - success: { data: [data from store], ids: [ids from response], total: [total from response], loading: false, loaded: true } + * - error: { error: [error from response], loading: false, loaded: true } + * + * This hook will return the cached result when called a second time + * with the same parameters, until the response arrives. + * + * @param {string} resource The resource name, e.g. 'posts' + * @param {Object} pagination The request pagination { page, perPage }, e.g. { page: 1, perPage: 10 } + * @param {Object} sort The request sort { field, order }, e.g. { field: 'id', order: 'DESC' } + * @param {Object} filter The request filters, e.g. { title: 'hello, world' } + * @param {Object} options Options object to pass to the dataProvider. May include side effects to be executed upon success or failure, e.g. { onSuccess: { refresh: true } } + * + * @returns The current request state. Destructure as { data, total, ids, error, loading, loaded }. + * + * @example + * + * import { useGetMainList } from 'react-admin'; + * + * const LatestNews = () => { + * const { data, ids, loading, error } = useGetMainList( + * 'posts', + * { page: 1, perPage: 10 }, + * { field: 'published_at', order: 'DESC' } + * ); + * if (loading) { return ; } + * if (error) { return

ERROR

; } + * return
    {ids.map(id => + *
  • {data[id].title}
    • + * )}
; + * }; + */ +export const useGetMainList = ( + resource: string, + pagination: PaginationPayload, + sort: SortPayload, + filter: object, + options?: any +): { + data?: RecordMap; + ids?: Identifier[]; + total?: number; + error?: any; + loading: boolean; + loaded: boolean; +} => { + const requestSignature = JSON.stringify({ pagination, sort, filter }); + const memo = useRef>({}); + const { + data: { finalIds, finalTotal, allRecords }, + error, + loading, + loaded, + } = useQueryWithStore( + { type: 'getList', resource, payload: { pagination, sort, filter } }, + options, + // ids and data selector + (state: ReduxState): DataSelectorResult => { + const ids = get(state.admin.resources, [ + resource, + 'list', + 'cachedRequests', + requestSignature, + 'ids', + ]); + const total = get(state.admin.resources, [ + resource, + 'list', + 'cachedRequests', + requestSignature, + 'total', + ]); + // When the user changes the page/sort/filter, the list of ids from + // the cached requests is empty. To avoid rendering an empty list + // at that moment, we override the ids and total with the latest + // loaded ones. + const mainIds = get(state.admin.resources, [ + resource, + 'list', + 'ids', + ]); + // Since the total can be empty during the loading phase + // We need to override that total with the latest loaded one + const mainTotal = get(state.admin.resources, [ + resource, + 'list', + 'total', + ]); + // can be null for a page that was never loaded. Useful for the isDataLoaded callback + const finalIds = typeof ids === 'undefined' ? mainIds : ids; + // can be null for a page that was never loaded. + const finalTotal = typeof total === 'undefined' ? mainTotal : total; + const allRecords = get( + state.admin.resources, + [resource, 'data'], + defaultData + ); + // poor man's useMemo inside a hook using a ref + if ( + memo.current.finalIds !== finalIds || + memo.current.finalTotal !== finalTotal || + memo.current.allRecords !== allRecords + ) { + const result = { + finalIds, + finalTotal, + allRecords, + }; + memo.current = { finalIds, finalTotal, allRecords, result }; + } + return memo.current.result; + }, + () => null, + isDataLoaded + ); + + const data = useMemo( + () => + typeof finalIds === 'undefined' + ? defaultData + : finalIds + .map(id => allRecords[id]) + .reduce((acc, record) => { + if (!record) return acc; + acc[record.id] = record; + return acc; + }, {}), + [finalIds, allRecords] + ); + + return { + data, + ids: typeof finalIds === 'undefined' ? defaultIds : finalIds, + total: finalTotal, + error, + loading, + loaded, + }; +}; + +interface DataSelectorResult { + finalIds?: Identifier[]; + finalTotal: number; + allRecords: RecordMap; +} + +interface Memo { + finalIds?: Identifier[]; + finalTotal?: number; + allRecords?: RecordMap; + result?: DataSelectorResult; +} + +const isDataLoaded = (data: DataSelectorResult) => + typeof data.finalIds !== 'undefined'; From 9f9a7b315b0246c578e3bb7542bcf58e522097ac Mon Sep 17 00:00:00 2001 From: fzaninotto Date: Thu, 11 Feb 2021 19:15:48 +0100 Subject: [PATCH 2/7] Add more comments --- .../src/controller/useListController.spec.tsx | 2 +- .../src/dataProvider/useGetMainList.tsx | 19 +++++++++++-------- .../src/reducer/admin/resource/list/ids.ts | 4 +++- 3 files changed, 15 insertions(+), 10 deletions(-) diff --git a/packages/ra-core/src/controller/useListController.spec.tsx b/packages/ra-core/src/controller/useListController.spec.tsx index fa0c6c7f230..ad91bb2f948 100644 --- a/packages/ra-core/src/controller/useListController.spec.tsx +++ b/packages/ra-core/src/controller/useListController.spec.tsx @@ -153,7 +153,6 @@ describe('useListController', () => { const props = { ...defaultProps, debounce: 200, - crudGetList: jest.fn(), children, }; @@ -171,6 +170,7 @@ describe('useListController', () => { params: {}, cachedRequests: {}, ids: [], + selectedIds: [], }, }, }, diff --git a/packages/ra-core/src/dataProvider/useGetMainList.tsx b/packages/ra-core/src/dataProvider/useGetMainList.tsx index 4560a4ce3b4..c8b1d022455 100644 --- a/packages/ra-core/src/dataProvider/useGetMainList.tsx +++ b/packages/ra-core/src/dataProvider/useGetMainList.tsx @@ -87,14 +87,15 @@ export const useGetMainList = ( 'cachedRequests', requestSignature, 'ids', - ]); + ]); // default value undefined const total = get(state.admin.resources, [ resource, 'list', 'cachedRequests', requestSignature, 'total', - ]); + ]); // default value undefined + // When the user changes the page/sort/filter, the list of ids from // the cached requests is empty. To avoid rendering an empty list // at that moment, we override the ids and total with the latest @@ -103,18 +104,21 @@ export const useGetMainList = ( resource, 'list', 'ids', - ]); + ]); // default value [] (see list.ids reducer) + // Since the total can be empty during the loading phase // We need to override that total with the latest loaded one const mainTotal = get(state.admin.resources, [ resource, 'list', 'total', - ]); - // can be null for a page that was never loaded. Useful for the isDataLoaded callback + ]); // default value null (see list.total reducer) + + // Is [] for a page that was never loaded const finalIds = typeof ids === 'undefined' ? mainIds : ids; - // can be null for a page that was never loaded. + // Is null for a page that was never loaded. const finalTotal = typeof total === 'undefined' ? mainTotal : total; + const allRecords = get( state.admin.resources, [resource, 'data'], @@ -176,5 +180,4 @@ interface Memo { result?: DataSelectorResult; } -const isDataLoaded = (data: DataSelectorResult) => - typeof data.finalIds !== 'undefined'; +const isDataLoaded = (data: DataSelectorResult) => data.finalTotal == null; // null or undefined diff --git a/packages/ra-core/src/reducer/admin/resource/list/ids.ts b/packages/ra-core/src/reducer/admin/resource/list/ids.ts index 86f5bea5431..83927cda3fe 100644 --- a/packages/ra-core/src/reducer/admin/resource/list/ids.ts +++ b/packages/ra-core/src/reducer/admin/resource/list/ids.ts @@ -22,6 +22,8 @@ type ActionTypes = meta: any; }; +const initialState = []; + /** * List of the ids of the latest loaded page, regardless of params * @@ -35,7 +37,7 @@ type ActionTypes = * */ const idsReducer: Reducer = ( - previousState = [], + previousState = initialState, action: ActionTypes ) => { if (action.meta && action.meta.optimistic) { From b1c41346e03c28fe9a4b47473c6174ace1694c2d Mon Sep 17 00:00:00 2001 From: fzaninotto Date: Thu, 11 Feb 2021 19:26:26 +0100 Subject: [PATCH 3/7] Fix unit test --- packages/ra-core/src/controller/useListController.spec.tsx | 1 + 1 file changed, 1 insertion(+) diff --git a/packages/ra-core/src/controller/useListController.spec.tsx b/packages/ra-core/src/controller/useListController.spec.tsx index ad91bb2f948..3b8d70f30b3 100644 --- a/packages/ra-core/src/controller/useListController.spec.tsx +++ b/packages/ra-core/src/controller/useListController.spec.tsx @@ -171,6 +171,7 @@ describe('useListController', () => { cachedRequests: {}, ids: [], selectedIds: [], + total: null, }, }, }, From 4625dbe2dc1a957491694cdb417f59907660c7b6 Mon Sep 17 00:00:00 2001 From: fzaninotto Date: Thu, 11 Feb 2021 20:22:33 +0100 Subject: [PATCH 4/7] avoid rerender --- packages/ra-core/src/dataProvider/useGetMainList.tsx | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/packages/ra-core/src/dataProvider/useGetMainList.tsx b/packages/ra-core/src/dataProvider/useGetMainList.tsx index c8b1d022455..d8511f7720f 100644 --- a/packages/ra-core/src/dataProvider/useGetMainList.tsx +++ b/packages/ra-core/src/dataProvider/useGetMainList.tsx @@ -153,7 +153,7 @@ export const useGetMainList = ( if (!record) return acc; acc[record.id] = record; return acc; - }, {}), + }, defaultData), [finalIds, allRecords] ); From 240209f102165c81e42454ec27c9ae63a5e73cd7 Mon Sep 17 00:00:00 2001 From: fzaninotto Date: Thu, 11 Feb 2021 21:27:13 +0100 Subject: [PATCH 5/7] Fix logic error --- packages/ra-core/src/dataProvider/useGetMainList.tsx | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/packages/ra-core/src/dataProvider/useGetMainList.tsx b/packages/ra-core/src/dataProvider/useGetMainList.tsx index d8511f7720f..ce14fd93645 100644 --- a/packages/ra-core/src/dataProvider/useGetMainList.tsx +++ b/packages/ra-core/src/dataProvider/useGetMainList.tsx @@ -180,4 +180,4 @@ interface Memo { result?: DataSelectorResult; } -const isDataLoaded = (data: DataSelectorResult) => data.finalTotal == null; // null or undefined +const isDataLoaded = (data: DataSelectorResult) => data.finalTotal != null; // null or undefined From 1bb2675648e12f4b1ddeb76edbcdef2e48f41907 Mon Sep 17 00:00:00 2001 From: fzaninotto Date: Thu, 11 Feb 2021 22:26:59 +0100 Subject: [PATCH 6/7] Fix request leak --- packages/ra-core/src/dataProvider/useGetMainList.tsx | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/packages/ra-core/src/dataProvider/useGetMainList.tsx b/packages/ra-core/src/dataProvider/useGetMainList.tsx index ce14fd93645..251094c8d15 100644 --- a/packages/ra-core/src/dataProvider/useGetMainList.tsx +++ b/packages/ra-core/src/dataProvider/useGetMainList.tsx @@ -153,7 +153,7 @@ export const useGetMainList = ( if (!record) return acc; acc[record.id] = record; return acc; - }, defaultData), + }, {}), [finalIds, allRecords] ); From 0ef90e9c8ed25aab3f37668d04cff31536d1245d Mon Sep 17 00:00:00 2001 From: fzaninotto Date: Thu, 11 Feb 2021 22:27:35 +0100 Subject: [PATCH 7/7] Add unit test --- .../src/controller/useListController.spec.tsx | 71 ++++++++++++++++++- .../admin/resource/list/cachedRequests.ts | 13 ++++ 2 files changed, 82 insertions(+), 2 deletions(-) diff --git a/packages/ra-core/src/controller/useListController.spec.tsx b/packages/ra-core/src/controller/useListController.spec.tsx index 3b8d70f30b3..ea60619364d 100644 --- a/packages/ra-core/src/controller/useListController.spec.tsx +++ b/packages/ra-core/src/controller/useListController.spec.tsx @@ -1,9 +1,10 @@ import * as React from 'react'; import expect from 'expect'; -import { fireEvent, waitFor } from '@testing-library/react'; +import { fireEvent, waitFor, act } from '@testing-library/react'; import lolex from 'lolex'; import TextField from '@material-ui/core/TextField/TextField'; +import { DataProviderContext } from '../dataProvider'; import ListController from './ListController'; import { getListControllerProps, @@ -41,6 +42,73 @@ describe('useListController', () => { debounce: 200, }; + describe('data', () => { + it('should be synchronized with ids after delete', async () => { + const FooField = ({ record }) => {record.foo}; + const dataProvider = { + getList: () => + Promise.resolve({ + data: [ + { id: 1, foo: 'foo1' }, + { id: 2, foo: 'foo2' }, + ], + total: 2, + }), + }; + const { dispatch, queryByText } = renderWithRedux( + + + {({ data, ids }) => ( + <> + {ids.map(id => ( + + ))} + + )} + + , + { + admin: { + resources: { + comments: { + list: { + params: {}, + cachedRequests: {}, + ids: [], + selectedIds: [], + total: null, + }, + data: {}, + }, + }, + }, + } + ); + await act(async () => await new Promise(r => setTimeout(r))); + + // delete one post + act(() => { + dispatch({ + type: 'RA/CRUD_DELETE_OPTIMISTIC', + payload: { id: 1 }, + meta: { + resource: 'comments', + fetch: 'DELETE', + optimistic: true, + }, + }); + }); + await act(async () => await new Promise(r => setTimeout(r))); + + expect(queryByText('foo1')).toBeNull(); + expect(queryByText('foo2')).not.toBeNull(); + }); + }); + describe('setFilters', () => { let clock; let fakeComponent = ({ setFilters, filterValues }) => ( @@ -178,7 +246,6 @@ describe('useListController', () => { }, } ); - const crudGetListCalls = dispatch.mock.calls.filter( call => call[0].type === 'RA/CRUD_GET_LIST' ); diff --git a/packages/ra-core/src/reducer/admin/resource/list/cachedRequests.ts b/packages/ra-core/src/reducer/admin/resource/list/cachedRequests.ts index fb550b19d8c..01b99efcb66 100644 --- a/packages/ra-core/src/reducer/admin/resource/list/cachedRequests.ts +++ b/packages/ra-core/src/reducer/admin/resource/list/cachedRequests.ts @@ -35,6 +35,19 @@ const cachedRequestsReducer: Reducer = ( // force refresh return initialState; } + if (action.meta && action.meta.optimistic) { + if ( + action.meta.fetch === CREATE || + action.meta.fetch === DELETE || + action.meta.fetch === DELETE_MANY || + action.meta.fetch === UPDATE || + action.meta.fetch === UPDATE_MANY + ) { + // force refresh of all lists because we don't know where the + // new/deleted/updated record(s) will appear in the list + return initialState; + } + } if (!action.meta || action.meta.fetchStatus !== FETCH_END) { // not a return from the dataProvider return previousState;