-
Notifications
You must be signed in to change notification settings - Fork 14k
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
chore(sqllab): Remove schemaOptions from redux store #23257
chore(sqllab): Remove schemaOptions from redux store #23257
Conversation
Codecov Report
@@ Coverage Diff @@
## master #23257 +/- ##
==========================================
+ Coverage 65.74% 67.51% +1.76%
==========================================
Files 1907 1907
Lines 73431 73429 -2
Branches 7973 7975 +2
==========================================
+ Hits 48280 49576 +1296
+ Misses 23103 21803 -1300
- Partials 2048 2050 +2
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 |
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 with just one tiny question
})), | ||
}), | ||
[schemaOptions], | ||
); |
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.
Do we really need a useMemo
here? It may look cleaner to keep the words logic with other autocomplete words.
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.
Yes. useMemo will reduce the cost of schemaWords
mapping from schemaOptions.
(For example, airbnb has > 10,000 schema options so useMemo
should be needed)
Hi @justinpark @ktmud. We have ongoing discussions to update Redux to the latest version which could potentially introduce a conflict with react-query given that Redux RTK is in the same domain space. Maybe it would be a good idea to discuss this in a SIP before moving forward? |
@michael-s-molina glad to hear that there's moving forward to redux-toolkit and plan to introduce the Redux RTK. |
@justinpark We have been discussing that our current Redux version precedes Redux Toolkit which makes the developer experience way more complex than it needs to be. Our plan was to upgrade Redux to the latest version as the first step to start organizing the Redux state, which would already help with new code. Can you help us migrate Redux to the new version to continue your improvements using RTK instead of React Query? If not, we can talk with @john-bodley and maybe I can help with the upgrade. |
@michael-s-molina If we've already decided to move on RTK, i'm happy to change React query by RTK. |
Nice! Thank you for the improvement. |
@rusackas I'm not sure if @justinpark was planning to merge this PR given that he's working on the RTK alternative. @justinpark should we revert or it's ok? |
Oops, was on a roll merging mergeable things. Sorry if I got a little trigger happy. |
@michael-s-molina this should be okay since I'm tracking of react-query migration by rdk-query. |
SUMMARY
As a followup step of the
react-query
migration (#21240), this commit migrates the schemaOptions state from redux to react-query to reduce the complexity of the queryEditor state.(The
schemaOptions
state only used in the editor for autocomplete so useQuery is a lighter way to share the state)BEFORE/AFTER SCREENSHOTS OR ANIMATED GIF
TESTING INSTRUCTIONS
Go to sqllab and select a database to get the associated schemas
Type some part of the schema name on the editor and check the autocomplete including the fetched schemas
ADDITIONAL INFORMATION
cc: @ktmud