Skip to content
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

refactor: introduce redux toolkit #23460

Merged
merged 6 commits into from
Apr 12, 2023

Conversation

justinpark
Copy link
Member

@justinpark justinpark commented Mar 22, 2023

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.

This commit also use the configureStore method of the @reduxjs/toolkit package, which replaces createStore since Redux Toolkit is our (redux official channel) recommended approach for writing Redux logic today, including store setup, reducers, data fetching, and more.

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)
    • 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

@codecov
Copy link

codecov bot commented Mar 22, 2023

Codecov Report

Merging #23460 (43eddbc) into master (3cff2b0) will increase coverage by 0.00%.
The diff coverage is 60.75%.

❗ Current head 43eddbc differs from pull request most recent head 5193ecb. Consider uploading reports for the commit 5193ecb to get more accurate results

@@           Coverage Diff           @@
##           master   #23460   +/-   ##
=======================================
  Coverage   67.72%   67.72%           
=======================================
  Files        1916     1917    +1     
  Lines       74033    74053   +20     
  Branches     8040     8045    +5     
=======================================
+ Hits        50137    50156   +19     
- Misses      21848    21850    +2     
+ Partials     2048     2047    -1     
Flag Coverage Δ
javascript 53.97% <60.71%> (+0.01%) ⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

Impacted Files Coverage Δ
...ugins/preset-chart-xy/src/components/Line/Line.tsx 0.00% <0.00%> (ø)
...preset-chart-xy/src/utils/createMarginSelector.tsx 0.00% <0.00%> (ø)
superset-frontend/src/SqlLab/App.jsx 0.00% <0.00%> (ø)
superset-frontend/src/views/menu.tsx 0.00% <0.00%> (ø)
superset/db_engine_specs/bigquery.py 69.54% <45.45%> (-0.18%) ⬇️
superset/utils/csv.py 96.66% <50.00%> (-1.61%) ⬇️
superset-frontend/src/views/store.ts 78.26% <88.88%> (+11.59%) ⬆️
superset/views/core.py 74.75% <88.88%> (+0.16%) ⬆️
...perset-ui-core/src/chart/components/SuperChart.tsx 100.00% <100.00%> (ø)
...et-ui-core/src/chart/components/SuperChartCore.tsx 100.00% <100.00%> (ø)
... and 3 more

... and 2 files with indirect coverage changes

📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more

superset-frontend/spec/fixtures/mockStore.js Outdated Show resolved Hide resolved
superset-frontend/spec/fixtures/mockStore.js Outdated Show resolved Hide resolved
superset-frontend/spec/fixtures/mockStore.js Outdated Show resolved Hide resolved
superset-frontend/spec/fixtures/mockStore.js Outdated Show resolved Hide resolved
superset-frontend/spec/fixtures/mockStore.js Outdated Show resolved Hide resolved
superset-frontend/spec/fixtures/mockStore.js Outdated Show resolved Hide resolved
superset-frontend/src/views/menu.tsx Outdated Show resolved Hide resolved
superset-frontend/src/views/store.ts Outdated Show resolved Hide resolved
superset-frontend/src/views/store.ts Outdated Show resolved Hide resolved
superset-frontend/src/views/store.ts Outdated Show resolved Hide resolved
@michael-s-molina
Copy link
Member

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

@kgabryje
Copy link
Member

Love it! Introducing redux toolkit was loooong overdue

@michael-s-molina
Copy link
Member

For those reviewing the PR, keep in mind that getDefaultMiddleware includes the immutabilityMiddleware and serializabilityMiddleware by default in development environments (it does not include them in production).

This will produce warnings and errors such as:

Screenshot 2023-03-24 at 15 08 09

Screenshot 2023-03-24 at 14 58 01

These errors are expected, and a great addition to our quality checks, and we should work to resolve them incrementally.

@michael-s-molina
Copy link
Member

@justinpark The immutability middleware is finding problems in many places.

Screen.Recording.2023-03-24.at.16.23.53.mov

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

@justinpark
Copy link
Member Author

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)

@michael-s-molina
Copy link
Member

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.

@michael-s-molina
Copy link
Member

michael-s-molina commented Mar 29, 2023

@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 along.

@justinpark
Copy link
Member Author

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.

@justinpark justinpark force-pushed the refactor--redux-toolkit-intro branch from 8f7a421 to 197bd34 Compare April 3, 2023 20:54
@justinpark
Copy link
Member Author

@michael-s-molina I rebase the branch including #23522
could you verify the issue resolved?

@michael-s-molina
Copy link
Member

@justinpark The errors fixed on iteration 1 were resolved. Iteration 2 is currently waiting for reviews.

@justinpark
Copy link
Member Author

@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.

@michael-s-molina
Copy link
Member

@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.

@justinpark
Copy link
Member Author

justinpark commented Apr 5, 2023

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 all iterations are completed

@justinpark justinpark force-pushed the refactor--redux-toolkit-intro branch from a5ff247 to 5193ecb Compare April 5, 2023 20:34
@justinpark
Copy link
Member Author

rebased master including iteration 2 (#23535)

Copy link
Member

@rusackas rusackas left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM! Seems cleaner and safer already :D

@rusackas
Copy link
Member

Slapping a hold! label on this since it sounds like #23637 should be merged first.

Copy link
Member

@michael-s-molina michael-s-molina left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@michael-s-molina michael-s-molina removed the hold! On hold label Apr 12, 2023
@justinpark justinpark merged commit 75021a1 into apache:master Apr 12, 2023
justinpark added a commit to airbnb/superset-fork that referenced this pull request Apr 27, 2023
Co-authored-by: Michael S. Molina <70410625+michael-s-molina@users.noreply.github.com>
sebastianliebscher pushed a commit to sebastianliebscher/superset that referenced this pull request Apr 28, 2023
Co-authored-by: Michael S. Molina <70410625+michael-s-molina@users.noreply.github.com>
@mistercrunch mistercrunch added 🏷️ bot A label used by `supersetbot` to keep track of which PR where auto-tagged with release labels 🚢 3.0.0 labels Mar 13, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
🏷️ bot A label used by `supersetbot` to keep track of which PR where auto-tagged with release labels size/L 🚢 3.0.0
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants