Add debounce to query update #88
Conversation
src/components/dagshub/data-engine/queryBuilder/QueryBuilderContext.tsx
Outdated
Show resolved
Hide resolved
src/components/dagshub/data-engine/queryBuilder/QueryBuilderContext.tsx
Outdated
Show resolved
Hide resolved
| checkIfConditionIsDisplayableInSimpleMode(queryInput.query) | ||
| ); | ||
| const [isSimpleMode, setIsSimpleMode] = useState<boolean>(isDisplayableInSimpleMode); |
There was a problem hiding this comment.
useState hooks should be in the top of the hook, heres why:
https://legacy.reactjs.org/docs/hooks-rules.html#only-call-hooks-at-the-top-level
There was a problem hiding this comment.
I'm not sure I understand. My use-state hooks are at the top. Before them there are just functions that the state uses, so they must be defined before the state itself, for them to be recognized.
According to the article, it doesn't seem to break any rule cause there is no prior return statement within the component itself (unless I miss something), and the hooks are not within any function/condition.
@elad-n Let me know if I miss something, and what I need to do
| function onToggleQueryMode() { | ||
| setIsSimpleMode(!isSimpleMode); | ||
| setIsSimpleMode((isSimpleMode) => { |
There was a problem hiding this comment.
| setIsSimpleMode((isSimpleMode) => { | |
| setIsSimpleMode(!isSimpleMode) |
I also didnt get the correlation between the isDisplayinSimpleMode / isSimpleMode. it cant be the same value ?
There was a problem hiding this comment.
@elad-n - Regarding your suggested change, @simonlsk told me that when I want to change the state, I should not use the variable from the state definition (ie. setBool(!bool)), but use the prev from the function instead, so there won't be race conditioning
@elad-n And regarding your question - isDisplayinSimpleMode just holds whether it is possible to switch the toggle to compound mode, but it doesn't say anything about the current query builder mode state, unlike isSimpleMode, which is the state itself. So they are not the same.
There was a problem hiding this comment.
In that case I think both will work correctly, but following the official docs using previous state is preferable if the set function can be called multiple times.
I personally prefer erring on the safe side an always use previous state.
src/components/dagshub/data-engine/queryBuilder/QueryBuilderContext.tsx
Outdated
Show resolved
Hide resolved
| } | ||
|
|
||
| useEffect(() => { | ||
| setMetadataFieldsList(metadataFields); | ||
| }, [metadataFields]); | ||
| setIsSimpleMode((isSimpleMode) => { |
There was a problem hiding this comment.
| setIsSimpleMode((isSimpleMode) => { | |
| const updatedIsSimpleMode = isSimpleMode ? false : isDisplayableInSimpleMode) | |
| setIsSimpleMode(updatedIsSimpleMode) |
There was a problem hiding this comment.
I guess its the same comment like before: #88 (comment)
Fix the issue with the toggle
Add debounce to onChange func
Fix the issue with infinite warnings on the Mui autocomplete- "value provided to Autocomplete is invalid"