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.

Sign up for GitHub

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

Fix hideFilter called repeatedly only registers the last call #6226

Merged
merged 3 commits into from
Apr 30, 2021
Merged

Fix hideFilter called repeatedly only registers the last call #6226

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
e1ad074
Fix hideFilter called repeatedly only registers the last call
fzaninotto Apr 29, 2021
0dac74b
Fix e2e test
fzaninotto Apr 29, 2021
03d4931
Replace global with ref
fzaninotto Apr 30, 2021
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
44 changes: 44 additions & 0 deletions packages/ra-core/src/controller/useListController.spec.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -350,6 +350,50 @@ describe('useListController', () => {
});
});
});

it('should support to sync calls', async () => {
const { getByLabelText, queryByText } = renderWithRedux(
<ListController {...defaultProps}>
{({ displayedFilters, showFilter }) => (
<>
<button
aria-label="Show filters"
onClick={() => {
showFilter('filter1.subdata', 'bob');
showFilter('filter2', '');
}}
/>
{Object.keys(displayedFilters).map(
(displayedFilter, index) => (
<div key={index}>{displayedFilter}</div>
)
)}
</>
)}
</ListController>,
{
admin: {
resources: {
posts: {
list: {
params: {
filter: { q: 'hello' },
},
cachedRequests: {},
},
},
},
},
}
);

fireEvent.click(getByLabelText('Show filters'));

await waitFor(() => {
expect(queryByText('filter1.subdata')).not.toBeNull();
expect(queryByText('filter2')).not.toBeNull();
});
});
});

describe('getListControllerProps', () => {
Expand Down
97 changes: 54 additions & 43 deletions packages/ra-core/src/controller/useListParams.spec.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -229,7 +229,7 @@ describe('useListParams', () => {
});
});
});
it('should initialize filters', () => {
it('should initialize filters', async () => {
const TestedComponent = () => {
const [, { showFilter }] = useListParams({
resource: 'foo',
Expand All @@ -250,22 +250,24 @@ describe('useListParams', () => {
</TestContext>
</Router>
);
expect(history.push).toBeCalledWith({
search:
'?' +
stringify({
displayedFilters: JSON.stringify({ foo: true }),
filter: JSON.stringify({ foo: 'bar' }),
sort: 'id',
order: 'ASC',
page: 1,
perPage: 10,
}),
state: { _scrollToTop: false },
await waitFor(() => {
expect(history.push).toBeCalledWith({
search:
'?' +
stringify({
displayedFilters: JSON.stringify({ foo: true }),
filter: JSON.stringify({ foo: 'bar' }),
sort: 'id',
order: 'ASC',
page: 1,
perPage: 10,
}),
state: { _scrollToTop: false },
});
});
});

it('should initialize displayed filters on compound filters', () => {
it('should initialize displayed filters on compound filters', async () => {
const TestedComponent = () => {
const [, { showFilter }] = useListParams({
resource: 'foo',
Expand All @@ -286,22 +288,26 @@ describe('useListParams', () => {
</TestContext>
</Router>
);
expect(history.push).toBeCalledWith({
search:
'?' +
stringify({
displayedFilters: JSON.stringify({ 'foo.bar': true }),
filter: JSON.stringify({ foo: { bar: 'baz' } }),
sort: 'id',
order: 'ASC',
page: 1,
perPage: 10,
}),
state: { _scrollToTop: false },
await waitFor(() => {
expect(history.push).toBeCalledWith({
search:
'?' +
stringify({
displayedFilters: JSON.stringify({
'foo.bar': true,
}),
filter: JSON.stringify({ foo: { bar: 'baz' } }),
sort: 'id',
order: 'ASC',
page: 1,
perPage: 10,
}),
state: { _scrollToTop: false },
});
});
});

it('should initialize filters on compound filters', () => {
it('should initialize filters on compound filters', async () => {
const TestedComponent = () => {
const [, { showFilter }] = useListParams({
resource: 'foo',
Expand All @@ -322,18 +328,22 @@ describe('useListParams', () => {
</TestContext>
</Router>
);
expect(history.push).toBeCalledWith({
search:
'?' +
stringify({
displayedFilters: JSON.stringify({ 'foo.bar': true }),
filter: JSON.stringify({ foo: { bar: 'baz' } }),
sort: 'id',
order: 'ASC',
page: 1,
perPage: 10,
}),
state: { _scrollToTop: false },
await waitFor(() => {
expect(history.push).toBeCalledWith({
search:
'?' +
stringify({
displayedFilters: JSON.stringify({
'foo.bar': true,
}),
filter: JSON.stringify({ foo: { bar: 'baz' } }),
sort: 'id',
order: 'ASC',
page: 1,
perPage: 10,
}),
state: { _scrollToTop: false },
});
});
});
});
Expand All @@ -351,7 +361,7 @@ describe('useListParams', () => {
return <button onClick={handleClick}>update</button>;
};

test('should synchronize parameters with location and redux state when sync is enabled', async () => {
it('should synchronize parameters with location and redux state when sync is enabled', async () => {
const history = createMemoryHistory();
jest.spyOn(history, 'push');
let dispatch;
Expand All @@ -366,9 +376,10 @@ describe('useListParams', () => {
);

fireEvent.click(getByText('update'));

expect(history.push).toHaveBeenCalled();
expect(dispatch).toHaveBeenCalled();
await waitFor(() => {
expect(history.push).toHaveBeenCalled();
expect(dispatch).toHaveBeenCalled();
});
});

test('should not synchronize parameters with location and redux state when sync is not enabled', async () => {
Expand Down
75 changes: 33 additions & 42 deletions packages/ra-core/src/controller/useListParams.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,13 +1,14 @@
import { useCallback, useMemo, useEffect, useState } from 'react';
import { useCallback, useMemo, useEffect, useState, useRef } from 'react';
import { useSelector, useDispatch, shallowEqual } from 'react-redux';
import { parse, stringify } from 'query-string';
import lodashDebounce from 'lodash/debounce';
import set from 'lodash/set';
import pickBy from 'lodash/pickBy';
import { useHistory, useLocation } from 'react-router-dom';

import queryReducer, {
SET_FILTER,
HIDE_FILTER,
SHOW_FILTER,
SET_PAGE,
SET_PER_PAGE,
SET_SORT,
Expand All @@ -16,7 +17,6 @@ import queryReducer, {
import { changeListParams, ListParams } from '../actions/listActions';
import { SortPayload, ReduxState, FilterPayload } from '../types';
import removeEmpty from '../util/removeEmpty';
import removeKey from '../util/removeKey';

interface ListParamsOptions {
resource: string;
Expand Down Expand Up @@ -125,6 +125,7 @@ const useListParams = ({
: defaultParams,
shallowEqual
);
const tempParams = useRef<ListParams>();

const requestSignature = [
location.search,
Expand Down Expand Up @@ -163,21 +164,31 @@ const useListParams = ({
}, []); // eslint-disable-line

const changeParams = useCallback(action => {
Copy link
Member Author

Choose a reason for hiding this comment

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

Here is the fix: since changeParams can be called several times in the same tick, we keep the result if the reducer locally so that we can start from it if another call comes immediately after. And we change the location (or the local state) only at the next tick, when all the sync changes were applied.

const newParams = queryReducer(query, action);
if (syncWithLocation) {
history.push({
search: `?${stringify({
...newParams,
filter: JSON.stringify(newParams.filter),
displayedFilters: JSON.stringify(
newParams.displayedFilters
),
})}`,
state: { _scrollToTop: action.type === SET_PAGE },
});
dispatch(changeListParams(resource, newParams));
if (!tempParams.current) {
// no other changeParams action dispatched this tick
tempParams.current = queryReducer(query, action);
// schedule side effects for next tick
setTimeout(() => {
if (syncWithLocation) {
history.push({
search: `?${stringify({
...tempParams.current,
filter: JSON.stringify(tempParams.current.filter),
displayedFilters: JSON.stringify(
tempParams.current.displayedFilters
),
})}`,
state: { _scrollToTop: action.type === SET_PAGE },
});
dispatch(changeListParams(resource, tempParams.current));
} else {
setLocalParams(tempParams.current);
}
tempParams.current = undefined;
}, 0);
} else {
setLocalParams(newParams);
// side effects already scheduled, just change the params
tempParams.current = queryReducer(tempParams.current, action);
}
}, requestSignature); // eslint-disable-line react-hooks/exhaustive-deps

Expand Down Expand Up @@ -229,38 +240,18 @@ const useListParams = ({
);

const hideFilter = useCallback((filterName: string) => {
// we don't use lodash.set() for displayed filters
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 moved the logic to the queryReducer for easier testability, but that has no effect on the bug.

// to avoid problems with compound filter names (e.g. 'author.name')
const displayedFilters = Object.keys(displayedFilterValues).reduce(
(filters, filter) => {
return filter !== filterName
? { ...filters, [filter]: true }
: filters;
},
{}
);
const filter = removeEmpty(removeKey(filterValues, filterName));
changeParams({
type: SET_FILTER,
payload: { filter, displayedFilters },
type: HIDE_FILTER,
payload: filterName,
});
}, requestSignature); // eslint-disable-line react-hooks/exhaustive-deps

const showFilter = useCallback((filterName: string, defaultValue: any) => {
// we don't use lodash.set() for displayed filters
Copy link
Member Author

Choose a reason for hiding this comment

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

Same, this change is just cosmetic

// to avoid problems with compound filter names (e.g. 'author.name')
const displayedFilters = {
...displayedFilterValues,
[filterName]: true,
};
const filter = defaultValue
? set(filterValues, filterName, defaultValue)
: filterValues;
changeParams({
type: SET_FILTER,
type: SHOW_FILTER,
payload: {
filter,
displayedFilters,
filterName,
defaultValue,
},
});
}, requestSignature); // eslint-disable-line react-hooks/exhaustive-deps
Expand Down
Loading