Add debounce for input material controls #1813
Conversation
There was a problem hiding this comment.
I did not test the changes yet but conceptionally this looks as expected.
However we don't need any of the useRef, useMemo and useCallbacks. They complicate the code and don't prevent any rerenderings.
The renderers are already memoized, so if nothing changed they should not even render. If something changed they need to rerender anyway, so memoizing some of the props for their children will not prevent their rerender as something else changes too.
The only exception is the debounced_setter. This needs to be placed in a useMemo, or even better in a to guarantee it's always the same and debouncing works.useState
Edit: useState is not a good choice as the setter depends on handleChange and needs to react on changes there. So useMemo is best.
| const eventToValue = (ev: any) => ev.target.value; | ||
| export const useDebouncedChange = (handleChange: (path: string, value: any) => void, defaultValue: any, data: any, path: string, eventToValueFunction: (ev: any) => any = eventToValue, timeout = 300): [any, React.ChangeEventHandler, () => void] => { | ||
| const [input, setInput] = useState(data || defaultValue); | ||
| const debouncedUpdate = useCallback(debounce((newValue: string) => handleChange(path, newValue), timeout), [handleChange]); |
There was a problem hiding this comment.
This should be a useMemo as we are calculating a function and not creating the function ourselves in place. A proper ESLint setup would complain here ;)
| const debouncedUpdate = useCallback(debounce((newValue: string) => handleChange(path, newValue), timeout), [handleChange]); | |
| const debouncedUpdate = useMemo(() => debounce((newValue: string) => handleChange(path, newValue), timeout), [handleChange]); |
There was a problem hiding this comment.
we are actually creating a function, a function that is used to call handleChange
There was a problem hiding this comment.
Yes that's true, and technically that's completely fine.
I just noticed the pattern because ESLint usually complaints about it ("exhaustive-deps") as it can't properly analyze the function and therefore doesn't give you the usual guarantees. As a workaround we can use useMemo. We just don't have ESLint yet configured. I'm fine leaving it like this, so we can also change this later when we finally use ESLint.
There was a problem hiding this comment.
Now that we're talking about it: timeout is missing in the dependencies as well as path.
- add a custom hook which handles debounce - add example to test form performance Fix eclipsesource#1645
Fix #1645