Skip to content

Commit

Permalink
reduce number of rerender from searchbar on load due to useRecentSear…
Browse files Browse the repository at this point in the history
…ches (#110)

SearchBar requires multiple re-renders due to useEffect call in useRecentSearches to setup RecentSearches instance. The changes in this pr ensures a RecentSearches is set on the first render and only re-render if there is an update to `recentSearchesLimit`. 

Findings when attempt to memoize hooks and testing in SearchBar:

Just using `useCallback` and `useMemo` doesn't really avoid unnecessary re-renders in our components. By default, the child components will always re-render if the parent component has triggered a render ([1](https://stackoverflow.com/a/40820657)). But, this can be override with shouldComponentUpdate for class-based component or with `React.memo(SomeComponent)` for functional component. `React.memo` prevents unnecessary re-renders if there's no change in the props (and no state update in the component itself), and we can use useMemo/useCallback to avoid creating new object when passing props to component each time.

I attempted to use React.memo on DropdownInput for SearchBar (had to wrap a lot of things in useCallback), but there's a lot of props and state changes from SearchBar interactions that almost certainly would require a re-render of the subcomponents. Clicking in/out of searchbar, typing/submitting/clearing inputs in some way generally trigger some setState functions due to new results and/or make updates to the different contexts that would affect the Dropdown components. Additionally, some advise that components like Dropdown with [child prop shouldn't use React.memo ](facebook/react#14463 (comment) the children prop almost always changes.

I believe there may **not** be a lot of benefits for doing useCallback throughout SearchBar to avoid re-renders in its subcomponents. It kind of make the codebase more complicated with the overhead and less readable, unless there's an actual performance bottleneck to address. So I'm a bit hesitant to add more useMemo/useCallback in our hooks and components if we don't need to.

J=SLAP-1965
TEST=manual

log number of renders in SearchBar before and after useRecentSearch changes. See that SearchBar have less re-renders on initial load.
  • Loading branch information
yen-tt authored Apr 14, 2022
1 parent 8e96c64 commit bb4f65b
Showing 1 changed file with 17 additions and 8 deletions.
25 changes: 17 additions & 8 deletions src/hooks/useRecentSearches.ts
Original file line number Diff line number Diff line change
@@ -1,12 +1,18 @@
import { useCallback, useEffect, useState } from 'react';
import { useCallback, useEffect, useRef, useState } from 'react';
import RecentSearches, { ISearch } from 'recent-searches';

export const RECENT_SEARCHES_KEY = '__yxt_recent_searches__';

export function useRecentSearches(
recentSearchesLimit: number
): [ISearch[]|undefined, (input: string) => void, () => void] {
const [ recentSearches, setRecentSeaches ] = useState<RecentSearches>();
const recentSearchesLimitRef = useRef(recentSearchesLimit);
const [ recentSearches, setRecentSeaches ] = useState<RecentSearches>(
new RecentSearches({
limit: recentSearchesLimit,
namespace: RECENT_SEARCHES_KEY
})
);

const clearRecentSearches = useCallback(() => {
localStorage.removeItem(RECENT_SEARCHES_KEY);
Expand All @@ -17,15 +23,18 @@ export function useRecentSearches(
localStorage.removeItem(RECENT_SEARCHES_KEY);
}, [recentSearchesLimit]);

const setRecentSearch = (input: string) => {
const setRecentSearch = useCallback((input: string) => {
recentSearches?.setRecentSearch(input);
};
}, [recentSearches]);

useEffect(() => {
setRecentSeaches(new RecentSearches({
limit: recentSearchesLimit,
namespace: RECENT_SEARCHES_KEY
}));
if (recentSearchesLimit !== recentSearchesLimitRef.current) {
setRecentSeaches(new RecentSearches({
limit: recentSearchesLimit,
namespace: RECENT_SEARCHES_KEY
}));
recentSearchesLimitRef.current = recentSearchesLimit;
}
}, [recentSearchesLimit]);

return [recentSearches?.getRecentSearches(), setRecentSearch, clearRecentSearches];
Expand Down

0 comments on commit bb4f65b

Please sign in to comment.