-
Notifications
You must be signed in to change notification settings - Fork 13.4k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
fix: Select a query from History and Run #15891
fix: Select a query from History and Run #15891
Conversation
Codecov Report
@@ Coverage Diff @@
## master #15891 +/- ##
==========================================
+ Coverage 77.08% 77.11% +0.02%
==========================================
Files 984 985 +1
Lines 51788 51849 +61
Branches 7037 7068 +31
==========================================
+ Hits 39923 39983 +60
- Misses 11640 11641 +1
Partials 225 225
Flags with carried forward coverage won't be shown. Click here to find out more.
Continue to review full report at Codecov.
|
static getDerivedStateFromProps(props, state) { | ||
if (props.queryEditor.sql !== state.sql) { | ||
return { | ||
sql: props.queryEditor.sql, | ||
}; | ||
} | ||
return null; | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
needing to use this function seems a bit concerning... i'm referencing here: https://reactjs.org/docs/react-component.html#static-getderivedstatefromprops:
If you want to “reset” some state when a prop changes, consider either making a component fully controlled or fully uncontrolled with a key instead.
I wonder if we can just use the sql from the queryEditor
for everything here instead of using the state.sql
string?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
i think this is a good idea: I removed all the usage for state.sql
and always use props.queryEditor.sql
.
/testenv up |
@etr2460 Ephemeral environment spinning up at http://34.220.228.128:8080. Credentials are |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
lgtm (and i couldn't break it in the test env). thanks for the fix and the iteration!
Ephemeral environment shutdown and build artifacts deleted. |
This reverts commit 8c7e09e.
This reverts commit 8c7e09e
…che#15992) This reverts commit 8c7e09e.
* fix: Run query picked from History * remove sql from component local state
…che#15992) This reverts commit 8c7e09e.
* fix: Run query picked from History * remove sql from component local state
…che#15992) This reverts commit 8c7e09e.
* fix: Run query picked from History * remove sql from component local state
…che#15992) This reverts commit 8c7e09e.
* fix: Run query picked from History * remove sql from component local state
…che#15992) This reverts commit 84da654.
SUMMARY
Fix an issue that SQL Lab didn't run correct sql after user pick a query from History. The root cause is that after update selected sql in redux store, component state is out of sync from props.
The proposed solution for this PR is to remove
sql
from component local state, and always use props which sync from redux store.BEFORE/AFTER SCREENSHOTS OR ANIMATED GIF
Step to reproduce:
TESTING INSTRUCTIONS
CI and manual test.
@jinghua-qa It's a little troublesome to add unit test for this bug. Could you add a test case for this flow?
ADDITIONAL INFORMATION
cc @etr2460 @ktmud