fix(sqllab): async query broken due to #21320 #21667
Conversation
| }; | ||
|
|
||
| // hack the useMemo hook to imitate componentWillMount | ||
| useMemo(() => { |
There was a problem hiding this comment.
Nice catch on finding cause of this bug!
I think the useEffect hook be more appropriate here and not be considered a hack, but recommended practice from React.
When using a
useEffect() => {
}, [])
`
With empty array it tells the functional component to only run the effect one time.
Notes section of: https://reactjs.org/docs/hooks-effect.html#tip-optimizing-performance-by-skipping-effects
"If you want to run an effect and clean it up only once (on mount and unmount), you can pass an empty array ([]) as a second argument. This tells React that your effect doesn’t depend on any values from props or state, so it never needs to re-run. This isn’t handled as a special case — it follows directly from how the dependencies array always works."
Basically same exact code just swapping out the useMemo with useEffect to align with React's best practices and remove the hack comment
There was a problem hiding this comment.
@eric-briscoe am I correct
useMemo(() => {}, []); is componentWillMount event.
useEffect(() => {}, []); is componentDidMount event.
They are two different events.
There was a problem hiding this comment.
@EugeneTorap useEffect is intended to replace all of the Class lifecycle methods so with functional components React is saying you no longer would try to differentiate between componentDidMount, componentDidUpdate, and componentWillUnmount, just use useEffect.
Tip If you’re familiar with React class lifecycle methods, you can think of useEffect Hook as componentDidMount, componentDidUpdate, and componentWillUnmount combined. https://reactjs.org/docs/hooks-effect.html
useMemo is for a very specific purpose and even React recommends not using it until you have a working component and start experience performance issues. useMemo runs during render and is not intended to produce side effects.
Remember that the function passed to useMemo runs during rendering. Don’t do anything there that you wouldn’t normally do while rendering. For example, side effects belong in useEffect, not useMemo. https://reactjs.org/docs/hooks-reference.html#usememo
Codecov Report
@@ Coverage Diff @@
## master #21667 +/- ##
==========================================
- Coverage 66.82% 66.82% -0.01%
==========================================
Files 1798 1799 +1
Lines 68827 68824 -3
Branches 7333 7315 -18
==========================================
- Hits 45997 45994 -3
+ Misses 20951 20945 -6
- Partials 1879 1885 +6
Flags with carried forward coverage won't be shown. Click here to find out more.
📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more |
|
/testenv up |
|
@lyndsiWilliams Ephemeral environment spinning up at http://35.161.89.79:8080. Credentials are |
|
@justinpark Can we try to cover it with RTL test? |
|
Ephemeral environment shutdown and build artifacts deleted. |
mistercrunch
added
🏷️ bot
SUMMARY
This commit fixes the async query broken issue on #21656
In #21320, it updated the following code
which skips the following
startQuerywith empty args operation.This different payload broke the async query request.
This commit reverts back this origin logic.
BEFORE/AFTER SCREENSHOTS OR ANIMATED GIF
TESTING INSTRUCTIONS
enable database option to async query execution
run the query
ADDITIONAL INFORMATION