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

Upgrade to React 18 #1173

Merged

Conversation

Sergio-Mira
Copy link
Contributor

Upgrade to React 18.

Which problem is this PR solving?

Short description of the changes

  • Upgrade to React 18

@codecov
Copy link

codecov bot commented Feb 4, 2023

Codecov Report

Base: 95.30% // Head: 95.41% // Increases project coverage by +0.10% 🎉

Coverage data is based on head (5b1c153) compared to base (d0915ea).
Patch coverage: 0.00% of modified lines in pull request are covered.

Additional details and impacted files
@@            Coverage Diff             @@
##             main    #1173      +/-   ##
==========================================
+ Coverage   95.30%   95.41%   +0.10%     
==========================================
  Files         243      243              
  Lines        7561     7562       +1     
  Branches     1895     1895              
==========================================
+ Hits         7206     7215       +9     
+ Misses        348      340       -8     
  Partials        7        7              
Impacted Files Coverage Δ
packages/jaeger-ui/src/index.js 0.00% <0.00%> (ø)
...nents/TracePage/TraceTimelineViewer/SpanBarRow.tsx 94.44% <0.00%> (-5.56%) ⬇️
...mponents/TracePage/TraceStatistics/tableValues.tsx 97.58% <0.00%> (+3.79%) ⬆️

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

☔ View full report at Codecov.
📢 Do you have feedback about the report comment? Let us know in this issue.

"react-circular-progressbar": "^2.0.3",
"react-dimensions": "^1.3.0",
"react-dom": "^16.14.0",
"react": "^18.2.0",
Copy link
Member

Choose a reason for hiding this comment

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

Wow, that looks suspiciously easy, were there really no breaking changes?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The breaking changes of lifecycle etc have been postponed to upcoming versions of React 18 and are still warnings afaik, and that is why its an easier upgrade. As far as I understand and can test the project still can use dependencies which are not yet fully React 18 compatible (like react-vis in this case).

Would love to hear how to move this PR forward and what we can do to test it, what edge cases of libs we think might break.

(I've also submitted changes to migrate from old lifecycle events in react-vis here uber/react-vis#1473)

Signed-off-by: Sergio-Mira <sergio.mira@zendesk.com>
@Sergio-Mira Sergio-Mira force-pushed the sergio-mira/upgrade-to-react-18 branch from f766abd to 5b1c153 Compare February 4, 2023 22:36
@@ -20,7 +20,7 @@ rafPolyfill();

/* eslint-disable import/no-extraneous-dependencies */
const Enzyme = require('enzyme');
const EnzymeAdapter = require('enzyme-adapter-react-16');
const EnzymeAdapter = require('@wojtekmaj/enzyme-adapter-react-17');
Copy link
Member

Choose a reason for hiding this comment

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

I can see this is another future PITA to be solved -- https://testing-library.com/docs/react-testing-library/migrate-from-enzyme/

Copy link
Member

@yurishkuro yurishkuro left a comment

Choose a reason for hiding this comment

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

did a brief test of different views, don't see any immediate issues, so let's give it a try

@yurishkuro yurishkuro merged commit fca5f89 into jaegertracing:main Feb 5, 2023
@metasal1
Copy link

Any plans to merge?

@yurishkuro
Copy link
Member

@metasal1 merge what? This PR was merged in Feb

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Upgrade React version - LFX Mentorship
3 participants