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

HPC-9965 Upgrade react-router-dom to react-router v7.0.2 #480

Open
wants to merge 8 commits into
base: develop
Choose a base branch
from

Conversation

Onitoxan
Copy link
Contributor

@Onitoxan Onitoxan commented Dec 9, 2024

Nature of PR

  • Bug-fix
  • Hot-fix
  • Feature
  • Testing
  • 🟢 Package update
  • CI

Description

(Link for upgrading documention to v7)

In this PR we focus on upgrading react-router-dom to its newest version v7.0.2.During this upgrade this package has been replaced in favor of react-router. Of the mentioned necessary changes we had no need to do none of them since they didn't affect us really, except for v7_startTransition in hpc-ftsadmin.

In async-autocomplete-field.tsx we had an useEffect() with the wrong variables inside of the dependency array. We decided to write all of them in order for eslint to not raise a warning of used variables in useEffect() that were not in the dependency array. After further investigation this extra properties didn't cause a re-render what entered in conflict with v7_startTransition, since with this flag react-router uses useTransition() instead of useState() what makes React categorize the importance of an action. The solution I provided, was going to my latest working branch of hpc-ftsadmin and took the values that I had in this dependencies array since I already encountered errors on this component and provided a fix.

Beside this change there is another "important" change that is to change the tsconfig.base.json and adding this new module resolution :

"moduleResolution": "bundler",

We were using node that anyways we would have had to update to node16 or nodenext since we support modern versions of Node. But since we used Nx and webpack I could activate bundler mode that give us more flexibility like not having to add all the file extensions of imports what would have resulted in a "big refactor". This was mainly done to allow us to import from react-router/dom, while using node we could not resolve this import.

I took the opportunity to implement a modern implementation of how they propose to create routers since v6 in hpc-ftsadmin what will allow us to use the data API that will be necessary in the development of FTS Admin v2.

How to test changes

To test this change I would recommend:

  1. Going through the documentation (I provided a link on previous section) and verify the steps that were followed.
  2. Verifying using "moduleResolution": "bundler" doesn't raise any problems.
  3. That routes didn't break in hpc-ftsadmin or hpc-cdm.
  4. Verify ReactRouter7Adapter is well written.

https://reactrouter.com/upgrading/v6 no use of relative paths, `React.lazy` or `RouterProvide`, no changes needed
It was necessary to modify the dependency array of async-autocomplete-field for it to work
`react-router-dom` is no longer necessary, we can just import `react-router` for a more simplified package
This is a necessary step to being able to resolve import from `react-router/dom`
@Onitoxan Onitoxan added the ready for review All comments have been addressed, and the Pull Request is ready for review label Dec 9, 2024
@Onitoxan Onitoxan requested a review from a team as a code owner December 9, 2024 11:15
Copy link

@unocha-hpc unocha-hpc left a comment

Choose a reason for hiding this comment

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

Checks have passed and this pull request is ready for manual review

Copy link
Contributor

@Pl217 Pl217 left a comment

Choose a reason for hiding this comment

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

Let's also migrate CDM to use RouterProvider from v7. We're in a monorepo, which allows easier sharing of code & libraries, so let's not create a divide, since refactoring isn't too hard in this case.

Comment on lines +17680 to +17681
"node_modules/react-router-dom/node_modules/react-router": {
"version": "6.28.0",
Copy link
Contributor

Choose a reason for hiding this comment

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

We are still installing v6 of react-router-dom and v6 of react-router underneath, since it is marked as peerDependencies of use-query-params. If you check lines added to node_modules/react-router-dom, they are:

"optional": true,
"peer": true,

since this peer dependency is marked as optional in use-query-params. We need to see how to instruct npm installation to remove it from package-lock.json so that all installations ignore v6 and use v7 (since use-query-params requires >=5), which is the direct dependency of this project.

You can also consider removing use-query-params.

Copy link
Contributor

Choose a reason for hiding this comment

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

If we don't remove use-query-params, you should indicate that this file is copied+adapted code from here. I was able to find it because you pasted the comment block too.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
ready for review All comments have been addressed, and the Pull Request is ready for review
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants