-
Notifications
You must be signed in to change notification settings - Fork 13.1k
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: upgrade react to 17.0.2 #22708
base: master
Are you sure you want to change the base?
Conversation
8aee9de
to
1353c4c
Compare
Hi @lilykuang! I see you removed |
@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 bvaughn/react-virtualized#1740 is merged |
They don't update the lib during 2 years. I've started rewriting FilterableTable in order to use our new AntD table. |
sounds good. thank you for the heads up @EugeneTorap |
0df16b3
to
24e5d6f
Compare
24e5d6f
to
9ca5034
Compare
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 😉 |
727707e
to
014b1a5
Compare
014b1a5
to
9a59bd3
Compare
/testenv up |
@lilykuang Ephemeral environment spinning up at http://54.191.98.204:8080. Credentials are |
Codecov ReportAttention:
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
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. |
@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 😉 |
@@ -39,6 +39,6 @@ | |||
"peerDependencies": { | |||
"@superset-ui/chart-controls": "*", | |||
"@superset-ui/core": "*", | |||
"react": "^15 || ^16" | |||
"react": "^15 || ^16 || ^17" |
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.
Maybe we should drop 15?
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.
💯
@@ -35,7 +35,7 @@ | |||
"@superset-ui/chart-controls": "*", | |||
"@superset-ui/core": "*", | |||
"mapbox-gl": "*", | |||
"react": "^15 || ^16" | |||
"react": "^15 || ^16 || ^17" |
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.
... same with the rest of these. We've been on React 16 long enough that I would hope we can drop 15 as a peer dependency? ¯\_(ツ)_/¯
/testenv up |
@kgabryje Ephemeral environment spinning up at http://34.221.110.31:8080. Credentials are |
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:
So I think we need to resolve some prerequisites before continuing the work. @kgabryje will know more. |
Any hacks to support conflicting reacts? Plugins-in-iframe-mode? |
hey @michael-s-molina @kgabryje, any news in this area? |
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 :) |
SUMMARY
BEFORE/AFTER SCREENSHOTS OR ANIMATED GIF
TESTING INSTRUCTIONS
ADDITIONAL INFORMATION