chore: upgrade react to 17.0.2

[lilykuang]

SUMMARY

• upgrade react to 17.0.2

BEFORE/AFTER SCREENSHOTS OR ANIMATED GIF

TESTING INSTRUCTIONS

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

[EugeneTorap]

Hi @lilykuang! I see you removed react-virtualized from package.json. Should we rewrite existed code which uses this lib?

[lilykuang]

@EugeneTorap I removed react-virtualized for testing purpose. I am actually waiting to see if react-virtualized will release a new version since the support for v17 and v18 https://github.com/bvaughn/react-virtualized/pull/1740 is merged

[EugeneTorap]

They don't update the lib during 2 years. I've started rewriting FilterableTable in order to use our new AntD table.

[lilykuang]

sounds good. thank you for the heads up @EugeneTorap

[villebro]

@lilykuang I see there's apparently only two failing tests left here. As this upgrade is very critical for dependency management, I propose just disabling those two tests for now if we can ensure the underlying components are working with the new version. Thoughts @michael-s-molina @kgabryje @eschutho ?

[michael-s-molina]

It would be great if we could merge this PR as part of the 4.0 initiative, during the breaking window. During that period, we'll merge many PRs that introduce breaking changes and will need to do a full test of the application, which would be a great opportunity to test the impact of this PR.

If you agree, just add a card to the 4.0 project board about upgrading React and mark this PR with the v4.0 label 😉

[lilykuang]

/testenv up

[github-actions[bot]]

@lilykuang Ephemeral environment spinning up at http://54.191.98.204:8080. Credentials are admin/admin. Please allow several minutes for bootstrapping and startup.

[codecov[bot]]

Codecov Report

Attention: 4 lines in your changes are missing coverage. Please review.

Comparison is base (5e85f5c) 69.18% compared to head (ae78073) 69.18%. Report is 4 commits behind head on master.

Files	Patch %	Lines
...dashboard/components/SliceHeaderControls/index.tsx	50.00%	0 Missing and 2 partials :warning:
superset-frontend/src/pages/Dashboard/index.tsx	0.00%	1 Missing :warning:
...erset-frontend/src/pages/DatasetCreation/index.tsx	50.00%	0 Missing and 1 partial :warning:

Additional details and impacted files

@@            Coverage Diff             @@
##           master   #22708      +/-   ##
==========================================
- Coverage   69.18%   69.18%   -0.01%     
==========================================
  Files        1945     1945              
  Lines       75971    75984      +13     
  Branches     8467     8475       +8     
==========================================
+ Hits        52559    52566       +7     
- Misses      21225    21228       +3     
- Partials     2187     2190       +3     

Flag	Coverage Δ
javascript	56.51% <42.85%> (-0.01%)	:arrow_down:

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

:umbrella: View full report in Codecov by Sentry.
:loudspeaker: Have feedback on the report? Share it here.

[eschutho]

@michael-s-molina @lilykuang Do we know for sure if there are any breaking changes for this PR or are we thinking of putting it into 4.0 just to be extra cautious?

[michael-s-molina]

@michael-s-molina @lilykuang Do we know for sure if there are any breaking changes for this PR or are we thinking of putting it into 4.0 just to be extra cautious?

We added the v4.0 label to merge it during the breaking window and reuse the tests/stabilization period given that this impacts the whole application and will require a full test. Is just to be extra cautious and optimize efforts 😉

[kgabryje]

/testenv up

[github-actions[bot]]

@kgabryje Ephemeral environment spinning up at http://34.221.110.31:8080. Credentials are admin/admin. Please allow several minutes for bootstrapping and startup.

[mistercrunch]

Wondering if we should re-open this against the main fork so more people can collab on it. I don't mind doing the rebasing and re-opening if people are going to push on this.

[michael-s-molina]

Wondering if we should re-open this against the main fork so more people can collab on it. I don't mind doing the rebasing and re-opening if people are going to push on this.

@mistercrunch This was punted to 5.0 because of the following:

Histogram and Event flow charts use old, unsupported library @data-ui, which is not compatible with React 17. Since there are no easy replacements, we need to rewrite those plugins using a different library or deprecate them before moving on with React 17 upgrade.

So I think we need to resolve some prerequisites before continuing the work. @kgabryje will know more.

[mistercrunch]

Any hacks to support conflicting reacts? Plugins-in-iframe-mode?

:( https://github.com/williaster/data-ui/issues/201

[amadejzv]

hey @michael-s-molina @kgabryje, any news in this area?

[rusackas]

Plugins-in-iframe-mode?

There are actually a lot of reasons to do this... but I've tried it begore, and it requires some major refactoring. It ain't gonna be easy until we've reached the end of the theming road. Hopefully we'll get there soon :)