Skip to content

Commit

Permalink
chore: Improves the Select component to avoid additional queries when…
Browse files Browse the repository at this point in the history
… all values have been loaded (apache#16712)

* chore: Improves the Select component to avoid additional queries when all values have been loaded

* Handles the logic in handlePaginateFetch and removes the use effect
  • Loading branch information
michael-s-molina authored Sep 15, 2021
1 parent ab764bf commit 18cc31c
Show file tree
Hide file tree
Showing 2 changed files with 52 additions and 9 deletions.
24 changes: 24 additions & 0 deletions superset-frontend/src/components/Select/Select.test.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -605,6 +605,30 @@ test('async - does not fire a new request for the same search input', async () =
expect(loadOptions).toHaveBeenCalledTimes(1);
});

test('async - does not fire a new request if all values have been fetched', async () => {
const mock = jest.fn(loadOptions);
const search = 'George';
const pageSize = OPTIONS.length;
render(<Select {...defaultProps} options={mock} pageSize={pageSize} />);
await open();
expect(mock).toHaveBeenCalledTimes(1);
await type(search);
expect(await findSelectOption(search)).toBeInTheDocument();
expect(mock).toHaveBeenCalledTimes(1);
});

test('async - fires a new request if all values have not been fetched', async () => {
const mock = jest.fn(loadOptions);
const search = 'George';
const pageSize = OPTIONS.length / 2;
render(<Select {...defaultProps} options={mock} pageSize={pageSize} />);
await open();
expect(mock).toHaveBeenCalledTimes(1);
await type(search);
expect(await findSelectOption(search)).toBeInTheDocument();
expect(mock).toHaveBeenCalledTimes(2);
});

/*
TODO: Add tests that require scroll interaction. Needs further investigation.
- Fetches more data when scrolling and more data is available
Expand Down
37 changes: 28 additions & 9 deletions superset-frontend/src/components/Select/Select.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -210,6 +210,7 @@ const Select = ({
const [page, setPage] = useState(0);
const [totalCount, setTotalCount] = useState(0);
const [loadingEnabled, setLoadingEnabled] = useState(!lazyLoading);
const [allValuesLoaded, setAllValuesLoaded] = useState(false);
const fetchedQueries = useRef(new Map<string, number>());
const mappedMode = isSingleMode
? undefined
Expand Down Expand Up @@ -333,25 +334,35 @@ const Select = ({
});

const handleData = (data: OptionsType) => {
let mergedData: OptionsType = [];
if (data && Array.isArray(data) && data.length) {
const dataValues = new Set();
data.forEach(option =>
dataValues.add(String(option.value).toLocaleLowerCase()),
);

// merges with existing and creates unique options
setSelectOptions(prevOptions => [
...prevOptions.filter(
previousOption =>
!dataValues.has(String(previousOption.value).toLocaleLowerCase()),
),
...data,
]);
setSelectOptions(prevOptions => {
mergedData = [
...prevOptions.filter(
previousOption =>
!dataValues.has(String(previousOption.value).toLocaleLowerCase()),
),
...data,
];
return mergedData;
});
}
return mergedData;
};

const handlePaginatedFetch = useMemo(
() => (value: string, page: number, pageSize: number) => {
if (allValuesLoaded) {
setIsLoading(false);
setIsTyping(false);
return;
}
const key = `${value};${page};${pageSize}`;
const cachedCount = fetchedQueries.current.get(key);
if (cachedCount) {
Expand All @@ -364,17 +375,24 @@ const Select = ({
const fetchOptions = options as OptionsPagePromise;
fetchOptions(value, page, pageSize)
.then(({ data, totalCount }: OptionsTypePage) => {
handleData(data);
const mergedData = handleData(data);
fetchedQueries.current.set(key, totalCount);
setTotalCount(totalCount);
if (
!fetchOnlyOnSearch &&
value === '' &&
mergedData.length >= totalCount
) {
setAllValuesLoaded(true);
}
})
.catch(onError)
.finally(() => {
setIsLoading(false);
setIsTyping(false);
});
},
[options],
[allValuesLoaded, fetchOnlyOnSearch, options],
);

const handleOnSearch = useMemo(
Expand Down Expand Up @@ -493,6 +511,7 @@ const Select = ({
setSelectOptions(
options && Array.isArray(options) ? options : EMPTY_OPTIONS,
);
setAllValuesLoaded(false);
}, [options]);

useEffect(() => {
Expand Down

0 comments on commit 18cc31c

Please sign in to comment.