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

update legacy react lifecycle methods to indicate unsafe status #610

Merged
merged 1 commit into from
Aug 11, 2020

Conversation

tklever
Copy link
Contributor

@tklever tklever commented Aug 6, 2020

Following the react migration path laid out here https://reactjs.org/blog/2018/03/27/update-on-async-rendering.html

Which problem is this PR solving?

Short description of the changes

  • Adding UNSAFE to legacy lifecycle methods for further review and update

Following the react migration path laid out here https://reactjs.org/blog/2018/03/27/update-on-async-rendering.html

Signed-off-by: Tim Klever <Tim.V.Klever@aexp.com>
@yurishkuro
Copy link
Member

Excuse my lack of React knowledge, but what would this accomplish? Wouldn't this result in the methods not being called when needed?

@codecov
Copy link

codecov bot commented Aug 6, 2020

Codecov Report

Merging #610 into master will increase coverage by 0.01%.
The diff coverage is n/a.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #610      +/-   ##
==========================================
+ Coverage   92.69%   92.70%   +0.01%     
==========================================
  Files         227      227              
  Lines        5911     5911              
  Branches     1491     1491              
==========================================
+ Hits         5479     5480       +1     
+ Misses        391      390       -1     
  Partials       41       41              
Impacted Files Coverage Δ
packages/jaeger-ui/src/components/App/Page.tsx 100.00% <ø> (ø)
...aeger-ui/src/components/DeepDependencies/index.tsx 100.00% <ø> (ø)
...components/DependencyGraph/DependencyForceGraph.js 85.00% <ø> (ø)
.../jaeger-ui/src/components/DependencyGraph/index.js 100.00% <ø> (ø)
...src/components/TracePage/ArchiveNotifier/index.tsx 0.00% <ø> (ø)
...ents/TracePage/TracePageHeader/SpanGraph/index.tsx 70.58% <ø> (ø)
...ents/TracePage/TraceStatistics/DetailTableData.tsx 82.60% <ø> (ø)
...onents/TracePage/TraceStatistics/MainTableData.tsx 70.96% <ø> (ø)
...eViewer/TimelineHeaderRow/TimelineViewingLayer.tsx 91.52% <ø> (+1.69%) ⬆️
...ePage/TraceTimelineViewer/VirtualizedTraceView.tsx 92.62% <ø> (ø)
... and 2 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 8279218...b76d30e. Read the comment docs.

@tklever
Copy link
Contributor Author

tklever commented Aug 6, 2020

Excuse my lack of React knowledge, but what would this accomplish? Wouldn't this result in the methods not being called when needed?

Actually, somewhat the opposite. In the future, the UNSAFE_* versions of these methods are the only ones that will be called.

According to to the "Gradual Migration Plan" at https://reactjs.org/blog/2018/03/27/update-on-async-rendering.html, in react 16.3, these method names are simply aliases (although, IMO the UNSAFE_* is a nice visual indicator that there is some debt hanging around if nothing else).

In later versions of 16, the legacy methods (like componentDidMount), will throw a deprecated warning in development mode, whereas the UNSAFE_* ones will not. UNSAFE_* in this case acknowledging, "yes we're doing it the old way, and it's with intent"

In React 17+, the legacy methods (like componentDidMount) will not be called at all. Only the UNSAFE_* version will be executed in the component lifecycle.

If you're looking for a short list of benefits as I see them, this change allows the project to upgrade React through 16.X and into 17.X without any major code changes, while still giving a pretty clear indicator that there is potential refactoring to be done. This would allow component refactoring to happen at a schedule decoupled from any needed React upgrades.

@yurishkuro
Copy link
Member

👍

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.

@everett980 any concerns?

@yurishkuro
Copy link
Member

@rubenvp8510 would you be able to review?

@rubenvp8510
Copy link
Collaborator

This LGTM as a starting point to address the migration of those methods.

@yurishkuro
Copy link
Member

Thanks!

@yurishkuro yurishkuro merged commit 6340b9a into jaegertracing:master Aug 11, 2020
@tklever tklever deleted the label-legacy-lifecycle branch August 11, 2020 16:50
vvvprabhakar pushed a commit to vvvprabhakar/jaeger-ui that referenced this pull request Jul 4, 2021
…ertracing#610)

Following the react migration path laid out here https://reactjs.org/blog/2018/03/27/update-on-async-rendering.html

Signed-off-by: Tim Klever <Tim.V.Klever@aexp.com>
Signed-off-by: vvvprabhakar <vvvprabhakar@gmail.com>
vvvprabhakar pushed a commit to vvvprabhakar/jaeger-ui that referenced this pull request Jul 4, 2021
…ertracing#610)

Following the react migration path laid out here https://reactjs.org/blog/2018/03/27/update-on-async-rendering.html

Signed-off-by: Tim Klever <Tim.V.Klever@aexp.com>
Signed-off-by: vvvprabhakar <vvvprabhakar@gmail.com>
vvvprabhakar pushed a commit to vvvprabhakar/jaeger-ui that referenced this pull request Jul 5, 2021
…ertracing#610)

Following the react migration path laid out here https://reactjs.org/blog/2018/03/27/update-on-async-rendering.html

Signed-off-by: Tim Klever <Tim.V.Klever@aexp.com>
Signed-off-by: vvvprabhakar <vvvprabhakar@gmail.com>
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.

3 participants