-
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): Refactor react-query by redux-toolkit query #23760
chore(sqllab): Refactor react-query by redux-toolkit query #23760
Conversation
957d0e2
to
227dea5
Compare
Codecov Report
@@ Coverage Diff @@
## master #23760 +/- ##
==========================================
+ Coverage 68.18% 68.19% +0.01%
==========================================
Files 1941 1941
Lines 75241 75286 +45
Branches 8158 8178 +20
==========================================
+ Hits 51306 51345 +39
+ Misses 21852 21846 -6
- Partials 2083 2095 +12
Flags with carried forward coverage won't be shown. Click here to find out more.
... and 3 files with indirect coverage changes 📣 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.
Thanks for the PR @justinpark. I left some first-pass comments.
superset-frontend/src/SqlLab/components/SqlEditorLeftBar/SqlEditorLeftBar.test.jsx
Outdated
Show resolved
Hide resolved
superset-frontend/src/SqlLab/components/SqlEditorLeftBar/SqlEditorLeftBar.test.jsx
Outdated
Show resolved
Hide resolved
9dda4c6
to
139ce55
Compare
0a465d5
to
8714143
Compare
f1297d8
to
d622a62
Compare
aaa3ca2
to
c61da36
Compare
@michael-s-molina I fixed all addressed issues. Please take a look |
export const storeWithApi = configureStore({ | ||
reducer: { | ||
[api.reducerPath]: api.reducer, | ||
}, | ||
middleware: getDefaultMiddleware => | ||
getDefaultMiddleware().concat(api.middleware), | ||
devTools: false, | ||
}); |
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.
export const storeWithApi = configureStore({ | |
reducer: { | |
[api.reducerPath]: api.reducer, | |
}, | |
middleware: getDefaultMiddleware => | |
getDefaultMiddleware().concat(api.middleware), | |
devTools: false, | |
}); | |
const createStore = (initialState: object = {}, reducers: object = {}) => | |
configureStore({ | |
preloadedState: initialState, | |
reducer: { | |
...(reducers || reducerIndex), | |
[api.reducerPath]: api.reducer, | |
}, | |
middleware: getDefaultMiddleware => | |
getDefaultMiddleware().concat(api.middleware), | |
devTools: false, | |
}); | |
export const defaultStore = createStore(); |
const mockStore = | ||
store ?? | ||
configureStore({ | ||
preloadedState: initialState || {}, | ||
preloadedState: initialState, | ||
devTools: false, | ||
reducer: reducers || reducerIndex, | ||
reducer: { | ||
...(reducers || reducerIndex), | ||
[api.reducerPath]: api.reducer, | ||
}, | ||
middleware: getDefaultMiddleware => | ||
getDefaultMiddleware().concat(api.middleware), | ||
}); |
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.
const mockStore = store ?? createStore(initialState, reducers);
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. Thank you @justinpark for the refactor and for addressing the comments!
bc81c3e
to
cc5324f
Compare
SUMMARY
As a follow-up of discussion, this commit migrates the existing react-query api to redux-toolkit query.
tl;dr
BEFORE/AFTER SCREENSHOTS OR ANIMATED GIF
N/A
TESTING INSTRUCTIONS
test all sqllab functions that run as expected before
ADDITIONAL INFORMATION