You are viewing a plain text version of this content. The canonical link for it is here.
Posted to notifications@superset.apache.org by "justinpark (via GitHub)" <gi...@apache.org> on 2023/03/22 18:49:55 UTC
[GitHub] [superset] justinpark opened a new pull request, #23460: refactor: introduce redux toolkit
justinpark opened a new pull request, #23460:
URL: https://github.com/apache/superset/pull/23460
### SUMMARY
Following up the discussion of #23257,
this commit introduces the redux-toolkit as a prerequisite for rdk(redux toolkit) query.
In order to consolidate the multiple state management solution, superset team decided to leverage the redux with toolkit in order to organize the existing redux state.
Redux toolkit also includes the rdk query package which can replace the existing react-query hooks (which is a similar solution to minimize the state of async api request) with a single solution.
After this commit merged, we can replace existing react-query by rdk-query too.
### BEFORE/AFTER SCREENSHOTS OR ANIMATED GIF
N/A
### TESTING INSTRUCTIONS
N/A
### ADDITIONAL INFORMATION
- [ ] Has associated issue:
- [ ] Required feature flags:
- [ ] Changes UI
- [ ] Includes DB Migration (follow approval process in [SIP-59](https://github.com/apache/superset/issues/13351))
- [ ] Migration is atomic, supports rollback & is backwards-compatible
- [ ] Confirm DB migration upgrade and downgrade tested
- [ ] Runtime estimates and downtime expectations provided
- [ ] Introduces new feature or API
- [ ] Removes existing feature or API
cc: @michael-s-molina @ktmud
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
[GitHub] [superset] justinpark commented on pull request #23460: refactor: introduce redux toolkit
Posted by "justinpark (via GitHub)" <gi...@apache.org>.
justinpark commented on PR #23460:
URL: https://github.com/apache/superset/pull/23460#issuecomment-1498117691
> justinpark `warnAfter` does not change from error to warning. It just adjusts the warnings threshold. State changes will still throw errors and break functionality. I'm working on Iteration 3 which will probably be enough to merge this PR given that all major flows will be preserved.
Got it. Thanks for confirm. Will wait until iteration is merged
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
[GitHub] [superset] michael-s-molina commented on pull request #23460: refactor: introduce redux toolkit
Posted by "michael-s-molina (via GitHub)" <gi...@apache.org>.
michael-s-molina commented on PR #23460:
URL: https://github.com/apache/superset/pull/23460#issuecomment-1485027129
> In order to properly use redux-toolkit going forward, I believe it is more appropriate to first address the current errors rather than excluding additional middleware (immutability, serializability).
No problem. I can help you test and identify the errors, and submit fixes. Let's keep this PR open and address the issues first then.
> The redux toolkit, which is being introduced to simplify the redux structure, seems to be causing more complex dependency issues like this.
@justinpark RTK is actually pointing out bugs that currently exist in our Redux use but are not noticed. I really like the fact that now we can see these errors and actually fix them. This will improve quality.
> Additionally, the redux toolkit query, which was introduced to replace react-query, not only has these dependency issues, but also increases the complexity of writing test code, as discovered during the current react-query migration work.
Even if the complexity is a little higher, I would vote for library consistency here. Over the years we had a proliferation of libraries that many times offered the same functionality or were not consistent with each other. That resulted in many migrations over the years and wasted resources. I think it's an acceptable trade off.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
[GitHub] [superset] michael-s-molina commented on pull request #23460: refactor: introduce redux toolkit
Posted by "michael-s-molina (via GitHub)" <gi...@apache.org>.
michael-s-molina commented on PR #23460:
URL: https://github.com/apache/superset/pull/23460#issuecomment-1497995428
> @michael-s-molina Since we changed the violation notice of immutability and serializability from error to warn, your iterations won't be a hard blocker.
@justinpark `warnAfter` does not change from error to warning. It just adjusts the warnings threshold. State changes will still throw errors and break functionality. I'm working on Iteration 3 which will probably be enough to merge this PR given that all major flows will be preserved.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
[GitHub] [superset] justinpark merged pull request #23460: refactor: introduce redux toolkit
Posted by "justinpark (via GitHub)" <gi...@apache.org>.
justinpark merged PR #23460:
URL: https://github.com/apache/superset/pull/23460
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
[GitHub] [superset] rusackas commented on pull request #23460: refactor: introduce redux toolkit
Posted by "rusackas (via GitHub)" <gi...@apache.org>.
rusackas commented on PR #23460:
URL: https://github.com/apache/superset/pull/23460#issuecomment-1502298030
Slapping a `hold!` label on this since it sounds like https://github.com/apache/superset/pull/23637 should be merged first.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
[GitHub] [superset] justinpark commented on pull request #23460: refactor: introduce redux toolkit
Posted by "justinpark (via GitHub)" <gi...@apache.org>.
justinpark commented on PR #23460:
URL: https://github.com/apache/superset/pull/23460#issuecomment-1505607976
@rusackas since #23637 merged, can you release the hold?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
[GitHub] [superset] michael-s-molina commented on pull request #23460: refactor: introduce redux toolkit
Posted by "michael-s-molina (via GitHub)" <gi...@apache.org>.
michael-s-molina commented on PR #23460:
URL: https://github.com/apache/superset/pull/23460#issuecomment-1481048636
Thank you so much @justinpark for this PR!
I'm tagging some folks here to help with testing and also for awareness given that Redux Toolkit improves the way to access the store and can really help with new features.
@villebro @geido @kgabryje @eschutho @betodealmeida
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
[GitHub] [superset] justinpark commented on pull request #23460: refactor: introduce redux toolkit
Posted by "justinpark (via GitHub)" <gi...@apache.org>.
justinpark commented on PR #23460:
URL: https://github.com/apache/superset/pull/23460#issuecomment-1492689023
> > In order to properly use redux-toolkit going forward, I believe it is more appropriate to first address the current errors rather than excluding additional middleware (immutability, serializability).
>
> No problem. I can help you test and identify the errors, and submit fixes. Let's keep this PR open and address the issues first then.
>
> > The redux toolkit, which is being introduced to simplify the redux structure, seems to be causing more complex dependency issues like this.
>
> justinpark RTK is actually pointing out bugs that currently exist in our Redux use but are not noticed. I really like the fact that now we can see these errors and actually fix them. This will improve quality.
>
> > Additionally, the redux toolkit query, which was introduced to replace react-query, not only has these dependency issues, but also increases the complexity of writing test code, as discovered during the current react-query migration work.
>
> Even if the complexity is a little higher, I would vote for library consistency here. Over the years we had a proliferation of libraries that many times offered the same functionality or were not consistent with each other. That resulted in many migrations over the years and wasted resources. I think it's an acceptable trade off.
Got your point. Okay I'll go ahead of the rtk query migration.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
[GitHub] [superset] michael-s-molina commented on a diff in pull request #23460: refactor: introduce redux toolkit
Posted by "michael-s-molina (via GitHub)" <gi...@apache.org>.
michael-s-molina commented on code in PR #23460:
URL: https://github.com/apache/superset/pull/23460#discussion_r1157310516
##########
superset-frontend/src/views/store.ts:
##########
@@ -135,13 +120,32 @@ export const store: Store = createStore(
* setupStore with disableDebugger true enables the menu.tsx component to avoid connecting
* to redux debugger so the application can connect to redux debugger
*/
-export function setupStore(disableDegugger = false): Store {
- return createStore(
- rootReducer,
- {},
- compose(
- applyMiddleware(thunk, logger),
- initEnhancer(false, undefined, disableDegugger),
- ),
- );
+export function setupStore({
+ disableDebugger = false,
+ initialState = {},
+ rootReducers = reducers,
+ ...overrides
+}: {
+ disableDebugger?: boolean;
+ initialState?: ConfigureStoreOptions['preloadedState'];
+ rootReducers?: ConfigureStoreOptions['reducer'];
+} & Partial<ConfigureStoreOptions> = {}): Store {
+ return configureStore({
+ preloadedState: initialState,
+ reducer: {
+ ...rootReducers,
+ },
+ middleware: getDefaultMiddleware =>
+ getDefaultMiddleware({
+ serializableCheck: {
+ // Ignores AbortController instances
+ ignoredActionPaths: [/queryController/g],
+ ignoredPaths: [/queryController/g],
+ },
+ }).concat(logger),
Review Comment:
```suggestion
getDefaultMiddleware({
getDefaultMiddleware({
immutableCheck: {
warnAfter: 200,
},
serializableCheck: {
// Ignores AbortController instances
ignoredActionPaths: [/queryController/g],
ignoredPaths: [/queryController/g],
warnAfter: 200,
},
}).concat(logger),
```
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
[GitHub] [superset] michael-s-molina commented on pull request #23460: refactor: introduce redux toolkit
Posted by "michael-s-molina (via GitHub)" <gi...@apache.org>.
michael-s-molina commented on PR #23460:
URL: https://github.com/apache/superset/pull/23460#issuecomment-1483220926
For those reviewing the PR, keep in mind that [getDefaultMiddleware](https://redux-toolkit.js.org/api/getDefaultMiddleware#included-default-middleware) includes the [immutabilityMiddleware](https://redux-toolkit.js.org/api/immutabilityMiddleware) and [serializabilityMiddleware](https://redux-toolkit.js.org/api/serializabilityMiddleware) by default in development environments (it does not include them in production).
This will produce warnings and errors such as:
<img width="859" alt="Screenshot 2023-03-24 at 15 08 09" src="https://user-images.githubusercontent.com/70410625/227605859-d6c7ae4a-5f1f-4550-889c-718cfea703db.png">
<img width="1785" alt="Screenshot 2023-03-24 at 14 58 01" src="https://user-images.githubusercontent.com/70410625/227604715-db7a43c0-1006-430d-9be7-9e83f15984b3.png">
These errors are expected, and a great addition to our quality checks, and we should work to resolve them incrementally.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
[GitHub] [superset] michael-s-molina commented on pull request #23460: refactor: introduce redux toolkit
Posted by "michael-s-molina (via GitHub)" <gi...@apache.org>.
michael-s-molina commented on PR #23460:
URL: https://github.com/apache/superset/pull/23460#issuecomment-1483328195
@justinpark The immutability middleware is finding problems in many places.
https://user-images.githubusercontent.com/70410625/227622214-4da88861-7051-4f22-8e4e-21477955593e.mov
<br>
I see two possible solutions:
- Try to fix the errors in this PR, so that when it's merged, there's no disruption
- Fix the errors incrementally
- Disable the immutability middleware by default for now
- Work on fixes for the problems (by enabling the middleware locally when working on PRs)
- After all the problems are fixed, enable it by default again
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
[GitHub] [superset] justinpark commented on pull request #23460: refactor: introduce redux toolkit
Posted by "justinpark (via GitHub)" <gi...@apache.org>.
justinpark commented on PR #23460:
URL: https://github.com/apache/superset/pull/23460#issuecomment-1494968438
@michael-s-molina I rebase the branch including #23522
could you verify the issue resolved?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
[GitHub] [superset] michael-s-molina commented on a diff in pull request #23460: refactor: introduce redux toolkit
Posted by "michael-s-molina (via GitHub)" <gi...@apache.org>.
michael-s-molina commented on code in PR #23460:
URL: https://github.com/apache/superset/pull/23460#discussion_r1153400183
##########
superset-frontend/src/views/store.ts:
##########
@@ -135,13 +120,25 @@ export const store: Store = createStore(
* setupStore with disableDebugger true enables the menu.tsx component to avoid connecting
* to redux debugger so the application can connect to redux debugger
*/
-export function setupStore(disableDegugger = false): Store {
- return createStore(
- rootReducer,
- {},
- compose(
- applyMiddleware(thunk, logger),
- initEnhancer(false, undefined, disableDegugger),
- ),
- );
+export function setupStore({
+ disableDebugger = false,
+ initialState = {},
+ rootReducers = reducers,
+ ...overrides
+}: {
+ disableDebugger?: boolean;
+ initialState?: ConfigureStoreOptions['preloadedState'];
+ rootReducers?: ConfigureStoreOptions['reducer'];
+} & Partial<ConfigureStoreOptions> = {}): Store {
+ return configureStore({
+ preloadedState: initialState,
+ reducer: {
+ ...rootReducers,
+ },
+ middleware: getDefaultMiddleware => getDefaultMiddleware().concat(logger),
Review Comment:
```suggestion
middleware: getDefaultMiddleware =>
getDefaultMiddleware({
serializableCheck: {
// Ignores AbortController instances
ignoredActionPaths: [/queryController/g],
ignoredPaths: [/queryController/g],
},
}).concat(logger),
```
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
[GitHub] [superset] codecov[bot] commented on pull request #23460: refactor: introduce redux toolkit
Posted by "codecov[bot] (via GitHub)" <gi...@apache.org>.
codecov[bot] commented on PR #23460:
URL: https://github.com/apache/superset/pull/23460#issuecomment-1480322178
## [Codecov](https://codecov.io/gh/apache/superset/pull/23460?src=pr&el=h1&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) Report
> Merging [#23460](https://codecov.io/gh/apache/superset/pull/23460?src=pr&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) (d9b5b8e) into [master](https://codecov.io/gh/apache/superset/commit/8ecf0b1c2009c86e56ba7930842f985c5f7f984a?el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) (8ecf0b1) will **increase** coverage by `0.00%`.
> The diff coverage is `67.56%`.
> :exclamation: Current head d9b5b8e differs from pull request most recent head 81ff46c. Consider uploading reports for the commit 81ff46c to get more accurate results
```diff
@@ Coverage Diff @@
## master #23460 +/- ##
=======================================
Coverage 67.63% 67.64%
=======================================
Files 1908 1909 +1
Lines 73733 73749 +16
Branches 7989 7994 +5
=======================================
+ Hits 49871 49887 +16
- Misses 21814 21815 +1
+ Partials 2048 2047 -1
```
| Flag | Coverage Δ | |
|---|---|---|
| javascript | `53.83% <60.71%> (+0.01%)` | :arrow_up: |
Flags with carried forward coverage won't be shown. [Click here](https://docs.codecov.io/docs/carryforward-flags?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#carryforward-flags-in-the-pull-request-comment) to find out more.
| [Impacted Files](https://codecov.io/gh/apache/superset/pull/23460?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) | Coverage Δ | |
|---|---|---|
| [...ugins/preset-chart-xy/src/components/Line/Line.tsx](https://codecov.io/gh/apache/superset/pull/23460?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-c3VwZXJzZXQtZnJvbnRlbmQvcGx1Z2lucy9wcmVzZXQtY2hhcnQteHkvc3JjL2NvbXBvbmVudHMvTGluZS9MaW5lLnRzeA==) | `0.00% <0.00%> (ø)` | |
| [...preset-chart-xy/src/utils/createMarginSelector.tsx](https://codecov.io/gh/apache/superset/pull/23460?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-c3VwZXJzZXQtZnJvbnRlbmQvcGx1Z2lucy9wcmVzZXQtY2hhcnQteHkvc3JjL3V0aWxzL2NyZWF0ZU1hcmdpblNlbGVjdG9yLnRzeA==) | `0.00% <0.00%> (ø)` | |
| [superset-frontend/src/SqlLab/App.jsx](https://codecov.io/gh/apache/superset/pull/23460?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-c3VwZXJzZXQtZnJvbnRlbmQvc3JjL1NxbExhYi9BcHAuanN4) | `0.00% <0.00%> (ø)` | |
| [superset-frontend/src/views/menu.tsx](https://codecov.io/gh/apache/superset/pull/23460?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-c3VwZXJzZXQtZnJvbnRlbmQvc3JjL3ZpZXdzL21lbnUudHN4) | `0.00% <0.00%> (ø)` | |
| [superset/db\_engine\_specs/base.py](https://codecov.io/gh/apache/superset/pull/23460?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-c3VwZXJzZXQvZGJfZW5naW5lX3NwZWNzL2Jhc2UucHk=) | `90.86% <80.00%> (-0.09%)` | :arrow_down: |
| [superset-frontend/src/views/store.ts](https://codecov.io/gh/apache/superset/pull/23460?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-c3VwZXJzZXQtZnJvbnRlbmQvc3JjL3ZpZXdzL3N0b3JlLnRz) | `78.26% <88.88%> (+11.59%)` | :arrow_up: |
| [...perset-ui-core/src/chart/components/SuperChart.tsx](https://codecov.io/gh/apache/superset/pull/23460?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-c3VwZXJzZXQtZnJvbnRlbmQvcGFja2FnZXMvc3VwZXJzZXQtdWktY29yZS9zcmMvY2hhcnQvY29tcG9uZW50cy9TdXBlckNoYXJ0LnRzeA==) | `97.22% <100.00%> (ø)` | |
| [...et-ui-core/src/chart/components/SuperChartCore.tsx](https://codecov.io/gh/apache/superset/pull/23460?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-c3VwZXJzZXQtZnJvbnRlbmQvcGFja2FnZXMvc3VwZXJzZXQtdWktY29yZS9zcmMvY2hhcnQvY29tcG9uZW50cy9TdXBlckNoYXJ0Q29yZS50c3g=) | `100.00% <100.00%> (ø)` | |
| [superset-frontend/src/SqlLab/reducers/index.js](https://codecov.io/gh/apache/superset/pull/23460?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-c3VwZXJzZXQtZnJvbnRlbmQvc3JjL1NxbExhYi9yZWR1Y2Vycy9pbmRleC5qcw==) | `100.00% <100.00%> (ø)` | |
| [superset/db\_engine\_specs/postgres.py](https://codecov.io/gh/apache/superset/pull/23460?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-c3VwZXJzZXQvZGJfZW5naW5lX3NwZWNzL3Bvc3RncmVzLnB5) | `97.10% <100.00%> (+0.08%)` | :arrow_up: |
... and [2 files with indirect coverage changes](https://codecov.io/gh/apache/superset/pull/23460/indirect-changes?src=pr&el=tree-more&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation)
:mega: We’re building smart automated test selection to slash your CI/CD build times. [Learn more](https://about.codecov.io/iterative-testing/?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation)
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
[GitHub] [superset] kgabryje commented on pull request #23460: refactor: introduce redux toolkit
Posted by "kgabryje (via GitHub)" <gi...@apache.org>.
kgabryje commented on PR #23460:
URL: https://github.com/apache/superset/pull/23460#issuecomment-1481276883
Love it! Introducing redux toolkit was loooong overdue
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
[GitHub] [superset] justinpark commented on pull request #23460: refactor: introduce redux toolkit
Posted by "justinpark (via GitHub)" <gi...@apache.org>.
justinpark commented on PR #23460:
URL: https://github.com/apache/superset/pull/23460#issuecomment-1498119520
rebased master including iteration 2 (#23535)
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
[GitHub] [superset] justinpark commented on a diff in pull request #23460: refactor: introduce redux toolkit
Posted by "justinpark (via GitHub)" <gi...@apache.org>.
justinpark commented on code in PR #23460:
URL: https://github.com/apache/superset/pull/23460#discussion_r1146625637
##########
superset-frontend/spec/fixtures/mockStore.js:
##########
@@ -32,38 +29,38 @@ import { dashboardFilters } from './mockDashboardFilters';
import { nativeFilters, dataMaskWith2Filters } from './mockNativeFilters';
export const storeWithState = state =>
- createStore(rootReducer, state, compose(applyMiddleware(thunk)));
+ setupStore({
+ disableDegugger: true,
Review Comment:
good catch
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
[GitHub] [superset] justinpark commented on pull request #23460: refactor: introduce redux toolkit
Posted by "justinpark (via GitHub)" <gi...@apache.org>.
justinpark commented on PR #23460:
URL: https://github.com/apache/superset/pull/23460#issuecomment-1483936602
> justinpark The immutability middleware is finding problems in many places.
>
>
> I see two possible solutions:
>
> * Try to fix the errors in this PR, so that when it's merged, there's no disruption
> * Fix the errors incrementally
>
> * Disable the immutability middleware by default for now
> * Work on fixes for the problems (by enabling the middleware locally when working on PRs)
> * After all the problems are fixed, enable it by default again
@michael-s-molina thanks for the detail testing and catching the side effects.
In order to properly use redux-toolkit going forward, I believe it is more appropriate to first address the current errors rather than excluding additional middleware (immutability, serializability). This is because future planned features, such as rdk query, may be dependent on such middlewares.
This has raised questions about whether it is the right direction to use the **redux-toolkit query** before proceeding with bug fixes. The redux toolkit, which is being introduced to simplify the redux structure, seems to be causing more complex dependency issues like this.
Additionally, the redux toolkit query, which was introduced to replace react-query, not only has these dependency issues, but also increases the complexity of writing test code, as discovered during the current react-query migration work. (I will follow up more detail about the complexity in test codes next week)
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
[GitHub] [superset] michael-s-molina commented on pull request #23460: refactor: introduce redux toolkit
Posted by "michael-s-molina (via GitHub)" <gi...@apache.org>.
michael-s-molina commented on PR #23460:
URL: https://github.com/apache/superset/pull/23460#issuecomment-1488771101
@justinpark I started submitting PRs to fix state mutations and help us merge this PR. I'm referencing this PR when submitting the fixes so you can follow them.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
[GitHub] [superset] michael-s-molina commented on pull request #23460: refactor: introduce redux toolkit
Posted by "michael-s-molina (via GitHub)" <gi...@apache.org>.
michael-s-molina commented on PR #23460:
URL: https://github.com/apache/superset/pull/23460#issuecomment-1495959220
@justinpark The errors fixed on iteration 1 were resolved. [Iteration 2](https://github.com/apache/superset/pull/23535) is currently waiting for reviews.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
[GitHub] [superset] justinpark commented on pull request #23460: refactor: introduce redux toolkit
Posted by "justinpark (via GitHub)" <gi...@apache.org>.
justinpark commented on PR #23460:
URL: https://github.com/apache/superset/pull/23460#issuecomment-1497958602
@michael-s-molina Since we changed the violation notice of immutability and serializability from error to warn, your iterations won't be a hard blocker.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
[GitHub] [superset] michael-s-molina commented on a diff in pull request #23460: refactor: introduce redux toolkit
Posted by "michael-s-molina (via GitHub)" <gi...@apache.org>.
michael-s-molina commented on code in PR #23460:
URL: https://github.com/apache/superset/pull/23460#discussion_r1146058596
##########
superset-frontend/spec/fixtures/mockStore.js:
##########
@@ -32,38 +29,38 @@ import { dashboardFilters } from './mockDashboardFilters';
import { nativeFilters, dataMaskWith2Filters } from './mockNativeFilters';
export const storeWithState = state =>
- createStore(rootReducer, state, compose(applyMiddleware(thunk)));
+ setupStore({
+ disableDegugger: true,
Review Comment:
```suggestion
disableDebugger: true,
```
##########
superset-frontend/spec/fixtures/mockStore.js:
##########
@@ -32,38 +29,38 @@ import { dashboardFilters } from './mockDashboardFilters';
import { nativeFilters, dataMaskWith2Filters } from './mockNativeFilters';
export const storeWithState = state =>
- createStore(rootReducer, state, compose(applyMiddleware(thunk)));
+ setupStore({
+ disableDegugger: true,
+ initialState: state,
+ });
export const getMockStore = overrideState =>
- createStore(
- rootReducer,
- { ...mockState, ...overrideState },
- compose(applyMiddleware(thunk)),
- );
+ setupStore({
+ disableDegugger: true,
+ initialState: { ...mockState, ...overrideState },
+ });
export const mockStore = getMockStore();
export const getMockStoreWithTabs = () =>
- createStore(
- rootReducer,
- {
+ setupStore({
+ disableDegugger: true,
Review Comment:
```suggestion
disableDebugger: true,
```
##########
superset-frontend/spec/fixtures/mockStore.js:
##########
@@ -32,38 +29,38 @@ import { dashboardFilters } from './mockDashboardFilters';
import { nativeFilters, dataMaskWith2Filters } from './mockNativeFilters';
export const storeWithState = state =>
- createStore(rootReducer, state, compose(applyMiddleware(thunk)));
+ setupStore({
+ disableDegugger: true,
+ initialState: state,
+ });
export const getMockStore = overrideState =>
- createStore(
- rootReducer,
- { ...mockState, ...overrideState },
- compose(applyMiddleware(thunk)),
- );
+ setupStore({
+ disableDegugger: true,
+ initialState: { ...mockState, ...overrideState },
+ });
export const mockStore = getMockStore();
export const getMockStoreWithTabs = () =>
- createStore(
- rootReducer,
- {
+ setupStore({
+ disableDegugger: true,
+ initialState: {
...mockState,
dashboardLayout: dashboardLayoutWithTabs,
dashboardFilters: {},
},
- compose(applyMiddleware(thunk)),
- );
+ });
export const getMockStoreWithChartsInTabsAndRoot = () =>
- createStore(
- rootReducer,
- {
+ setupStore({
+ disableDegugger: true,
Review Comment:
```suggestion
disableDebugger: true,
```
##########
superset-frontend/spec/fixtures/mockStore.js:
##########
@@ -101,7 +98,10 @@ export const stateWithFilters = {
// one chart with a filter that has been rejected,
// and one chart with no filters set.
export const getMockStoreWithFilters = () =>
- createStore(rootReducer, stateWithFilters);
+ setupStore({
+ disableDegugger: true,
Review Comment:
```suggestion
disableDebugger: true,
```
##########
superset-frontend/src/views/store.ts:
##########
@@ -135,13 +120,25 @@ export const store: Store = createStore(
* setupStore with disableDebugger true enables the menu.tsx component to avoid connecting
* to redux debugger so the application can connect to redux debugger
*/
-export function setupStore(disableDegugger = false): Store {
- return createStore(
- rootReducer,
- {},
- compose(
- applyMiddleware(thunk, logger),
- initEnhancer(false, undefined, disableDegugger),
- ),
- );
+export function setupStore({
+ disableDegugger = false,
+ initialState = {},
+ rootReducers = reducers,
+ ...overrides
+}: {
+ disableDegugger?: boolean;
+ initialState?: ConfigureStoreOptions['preloadedState'];
+ rootReducers?: ConfigureStoreOptions['reducer'];
+} & Partial<ConfigureStoreOptions> = {}): Store {
+ return configureStore({
+ preloadedState: initialState,
+ reducer: {
+ ...rootReducers,
+ },
+ middleware: getDefaultMiddleware => getDefaultMiddleware().concat(logger),
+ devTools: process.env.WEBPACK_MODE === 'development' && !disableDegugger,
Review Comment:
```suggestion
devTools: process.env.WEBPACK_MODE === 'development' && !disableDebugger,
```
##########
superset-frontend/src/views/store.ts:
##########
@@ -135,13 +120,25 @@ export const store: Store = createStore(
* setupStore with disableDebugger true enables the menu.tsx component to avoid connecting
* to redux debugger so the application can connect to redux debugger
*/
-export function setupStore(disableDegugger = false): Store {
- return createStore(
- rootReducer,
- {},
- compose(
- applyMiddleware(thunk, logger),
- initEnhancer(false, undefined, disableDegugger),
- ),
- );
+export function setupStore({
+ disableDegugger = false,
+ initialState = {},
+ rootReducers = reducers,
+ ...overrides
+}: {
+ disableDegugger?: boolean;
Review Comment:
```suggestion
disableDebugger?: boolean;
```
##########
superset-frontend/src/views/store.ts:
##########
@@ -135,13 +120,25 @@ export const store: Store = createStore(
* setupStore with disableDebugger true enables the menu.tsx component to avoid connecting
* to redux debugger so the application can connect to redux debugger
*/
-export function setupStore(disableDegugger = false): Store {
- return createStore(
- rootReducer,
- {},
- compose(
- applyMiddleware(thunk, logger),
- initEnhancer(false, undefined, disableDegugger),
- ),
- );
+export function setupStore({
+ disableDegugger = false,
Review Comment:
```suggestion
disableDebugger = false,
```
##########
superset-frontend/spec/fixtures/mockStore.js:
##########
@@ -32,38 +29,38 @@ import { dashboardFilters } from './mockDashboardFilters';
import { nativeFilters, dataMaskWith2Filters } from './mockNativeFilters';
export const storeWithState = state =>
- createStore(rootReducer, state, compose(applyMiddleware(thunk)));
+ setupStore({
+ disableDegugger: true,
+ initialState: state,
+ });
export const getMockStore = overrideState =>
- createStore(
- rootReducer,
- { ...mockState, ...overrideState },
- compose(applyMiddleware(thunk)),
- );
+ setupStore({
+ disableDegugger: true,
Review Comment:
```suggestion
disableDebugger: true,
```
##########
superset-frontend/spec/fixtures/mockStore.js:
##########
@@ -132,7 +132,10 @@ export const stateWithNativeFilters = {
};
export const getMockStoreWithNativeFilters = () =>
- createStore(rootReducer, stateWithNativeFilters);
+ setupStore({
+ disableDegugger: true,
Review Comment:
```suggestion
disableDebugger: true,
```
##########
superset-frontend/src/views/menu.tsx:
##########
@@ -33,7 +33,7 @@ import { setupStore } from './store';
// Disable connecting to redux debugger so that the React app injected
// Below the menu like SqlLab or Explore can connect its redux store to the debugger
-const store = setupStore(true);
+const store = setupStore({ disableDegugger: true });
Review Comment:
```suggestion
const store = setupStore({ disableDebugger: true });
```
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org