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 to react-router 6 #301

Merged
merged 3 commits into from
Apr 8, 2020
Merged

Conversation

Fullstop000
Copy link
Contributor

UCP #249

Update to react-router6

Signed-off-by: Fullstop000 fullstop1005@gmail.com

@sre-bot
Copy link
Collaborator

sre-bot commented Mar 30, 2020

@Fullstop000, you already got 300 points from easy level tasks when all pull requests merged. And you will not get score from this PR.

@sre-bot
Copy link
Collaborator

sre-bot commented Mar 30, 2020

Thanks for your contribution. If your PR get merged, you will be rewarded 100 points.

@baurine
Copy link
Collaborator

baurine commented Mar 30, 2020

hi @Fullstop000 , thanks for your contribution, I will test and review soon.

@baurine
Copy link
Collaborator

baurine commented Mar 30, 2020

Hi @Fullstop000 , after my test, I found some errors, statements / diagnose / search logs / profile pages can't show normally, they are all empty, and the console outputs following errors:

Uncaught Error: Route(...): Nothing was returned from render. This usually means a return statement is missing. Or, to render nothing, return null.

image

@Fullstop000
Copy link
Contributor Author

Got something pained after upgrading to react-router v6
image

@baurine
Copy link
Collaborator

baurine commented Mar 30, 2020

Got something pained after upgrading to react-router v6
image

yep, that's a problem, we need to have a look.

@Fullstop000
Copy link
Contributor Author

And I got this warning only when unmount AppStatement
image

@baurine
Copy link
Collaborator

baurine commented Mar 31, 2020

And I got this warning only when unmount AppStatement
image

I will have a look today.

@baurine
Copy link
Collaborator

baurine commented Mar 31, 2020

There is some discussion in facebook/react#18178 , I tried to upgrade the react to 16.13.1 , it doesn't dismiss the warning.

It should be caused by the new version react-router, above warning doesn't happen for react-router v5, someone has already reported in remix-run/react-router#7199 as well.

So I think let's wait a few days more to see whether react-router will fix it, if it doesn't, we can merge it first and upgrade it in the future.

@Fullstop000
Copy link
Contributor Author

So I think let's wait a few days more to see whether react-router will fix it, if it doesn't, we can merge it first and upgrade it in the future.

Sounds reasonable :)

@baurine
Copy link
Collaborator

baurine commented Apr 7, 2020

hi @Fullstop000 , react-router fixed above warning in https://github.com/ReactTraining/react-router/releases/tag/v6.0.0-alpha.3 , you can have a try, but unfortunately they removed <Redirect/> component as well which we used, not sure whether it is easy to migrate, if it doesn't, I suggest let update react-router v6 until it is stable, @breeswish , how do you think about it?

@Fullstop000
Copy link
Contributor Author

Maybe using a history API instead <Redirect/> ? I'll have a try later today.

@breezewish
Copy link
Member

@baurine I'm fine with both. This task is not ergant.

Signed-off-by: Fullstop000 <fullstop1005@gmail.com>
Signed-off-by: Fullstop000 <fullstop1005@gmail.com>
Signed-off-by: Fullstop000 <fullstop1005@gmail.com>
@Fullstop000
Copy link
Contributor Author

@baurine The second warning has been fixed and the first warning only shows on the initial render.

@baurine baurine linked an issue Apr 8, 2020 that may be closed by this pull request
Copy link
Collaborator

@baurine baurine left a comment

Choose a reason for hiding this comment

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

LGTM

@baurine baurine merged commit ca10ca2 into pingcap:master Apr 8, 2020
@sre-bot
Copy link
Collaborator

sre-bot commented Apr 8, 2020

Team .* complete task #249 and get 100 score, currerent score 1955

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.

UCP: Update to React Router v6
4 participants