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 react-router-dom to 5.x #411

Closed
SYU15 opened this issue Sep 26, 2019 · 8 comments
Closed

Upgrade react-router-dom to 5.x #411

SYU15 opened this issue Sep 26, 2019 · 8 comments

Comments

@SYU15
Copy link
Contributor

SYU15 commented Sep 26, 2019

We are currently on version 4.3.1 of react-router-dom and are seeing a series of deprecation warnings like these in the application:

Warning: componentWillMount has been renamed, and is not recommended for use. See https://fb.me/react-async-component-lifecycle-hooks for details.

* Move code with side effects to componentDidMount, and set initial state in the constructor.
* Rename componentWillMount to UNSAFE_componentWillMount to suppress this warning in non-strict mode. In React 17.x, only the UNSAFE_ name will work. To rename all deprecated lifecycles to their new names, you can run `npx react-codemod rename-unsafe-lifecycles` in your project source folder.

Please update the following components: BrowserRouter, Route, Router

These deprecation issues are fixed the the most recent major version release 5.x, however there could very well be some breaking changes in this that will require us to refactor our code, so this upgrade will require a bit of investigation/testing before we check it in.

@Mr0grog Mr0grog added this to Inbox in Web Monitoring via automation Sep 26, 2019
@Mr0grog Mr0grog moved this from Inbox to Ready in Web Monitoring Sep 26, 2019
@jatinAroraGit
Copy link
Contributor

Hey there,
I would like to work on this issue.
Can you please assign this to me ?
Thanks.

@SYU15
Copy link
Contributor Author

SYU15 commented Sep 27, 2019

Sounds good @jatinAroraGit! Lmk if you have any questions.

@jatinAroraGit
Copy link
Contributor

@SYU15 I updated the react-router-dom and there have been no warnings or errors in the build process. On running the "npm test" command, it still passed all the tests. I would dive more into the documentation of react-router-dom and see what changes have they introduced into v5.1.0 and possible effects.
Is there any way to provide a demo login and password to test the application on localhost so I can test manually for any changes after the update in the working of the app ?
Thanks.

@SYU15
Copy link
Contributor Author

SYU15 commented Sep 28, 2019

Thanks for your help on this @jatinAroraGit! @Mr0grog actually just checked in a helpful section to get you up and running with the app in staging: https://github.com/edgi-govdata-archiving/web-monitoring-ui/blob/master/README.md#installation

Can you follow the steps and let me know if you run into any issues? It would be great if you can confirm that the warnings no longer are thrown in the dev console.

@jatinAroraGit
Copy link
Contributor

Hey @SYU15 ,
After updating react-router-dom, the dev console logs depreciation warnings related to react-displace and react-aria-modal components. These components seem to still use ComponentWillMount instead of ComponentDidMount.
react-aria-modal: Uses ComponentWillMount to throw errors related to not having title text and title Id.
react-displaced: Uses ComponentWillMount to render diffrent type of HTML based on the value of options,renderTo.

Here is a image showing the warning:
react update err
Another thing to note is that the depreciation warnings are only thrown at the login page, the application if loaded straight to http://localhost:3001/pages , will not throw any depreciation warnings in the console.
So far I have not noticed any abnormal behavior in app after the react-router-dom update.
Do you want me to create a pull request of the changes ?

@SYU15
Copy link
Contributor Author

SYU15 commented Oct 1, 2019

Great! Thanks a bunch for working on this! Will take a look at those deprecation warnings you pointed out and maybe will create an issue for them.

Go ahead and make a PR for the upgrade :)

@Mr0grog
Copy link
Member

Mr0grog commented Oct 3, 2019

After updating react-router-dom, the dev console logs depreciation warnings related to react-displace and react-aria-modal components.

That’s OK — we can do a second PR for them. One thing at a time. :)

the depreciation warnings are only thrown at the login page, the application if loaded straight to http://localhost:3001/pages , will not throw any depreciation warnings in the console.

That makes sense — the React runtime won’t know there’s an issue until it’s given a component to render, and we don’t render react-aria-modal or react-displace until we show that login screen. It’s probably been logging that error all along, but we only noticed the router-related warning since they look so similar.

@Mr0grog
Copy link
Member

Mr0grog commented Oct 10, 2019

Hi @jatinAroraGit, I’m just checking in — are you still working on this?

jatinAroraGit pushed a commit to jatinAroraGit/web-monitoring-ui that referenced this issue Oct 11, 2019
Web Monitoring automation moved this from Ready to Done! Oct 14, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
Web Monitoring
  
Done!
Development

No branches or pull requests

3 participants