CUMULUS-3862: Upgrade React Router to v6 #1148
Conversation
There was a problem hiding this comment.
@jjmccoy leaving the first part of this review. I've looked at 74/100 files and I want to get something up as it's taking a while to go through everything. Still going through the remaining 26 files and I'll post another review for those.
Also I see there are Cypress tests failing. Are those flukes or likely to require more changes?
| "final-form": "^4.20.4", | ||
| "global": "^4.3.1", | ||
| "history": "^4.10.1", | ||
| "history": "^4.7.2", |
There was a problem hiding this comment.
What's the reasoning for downgrading?
There was a problem hiding this comment.
When I ran '--legacy-peer-deps' for 'connected-react-router' to work with the React and React Router upgrade it downgraded it. The 'history' prop in the Redux store and dependency will be removed in the next ticket when the React upgrade is complete because it will no longer be supported.
There was a problem hiding this comment.
I'm not sure I follow. Is history a peer dependency of react-router? But we also import history as a direct dependency? Running npm install --legacy-peer-deps ignores peerDependency installation and uses the version of the module in the dependencies here? "history": "^4.7.2",?
There was a problem hiding this comment.
history is a peer dependency of connected-react-router. connected-react-router is our problem child because is no longer supported with React 18 and React Router v6 but to keep the app working when while we gradually updated we ran the npm --legacy-peer-deps for connected-react-router because the Redux store uses it and the next and final ticket for the React update is the Redux and Redux Toolkit upgrade. This allows us to use the legacy peer dependencies for connected-react-router and not run into an issue with React Router v6 and have it yell at you about incompatible dependencies. When I first upgraded React Router, I removed connected-react-router and history because it's not supported or needed. Then I realized that we are going to need to keep connected-react-router because of how our Redux store is set up. I ran npm install connected-react-router --legacy-peer-deps` and they both were installed and everything worked fine.
Just tried this:
If I install "history": "^4.10.1" then I get this error:
ERROR in ./node_modules/history/es/index.js
Module build failed: Error: ENOENT: no such file or directory, open '.../cumulus-dashboard/node_modules/history/es/index.js'
@ ./app/src/js/store/configureStore.js 2:0-66 13:47-64 13:71-91
@ ./app/src/js/App.js 4:0-55 45:27-44
@ ./app/src/index.js 5:0-27 15:45-48
If I install "history": "^4.7.2" all is well 🤷♀️
There was a problem hiding this comment.
So the follow-on work to upgrade Redux will also allow us to bump this dependency? Is that captured in the ticket?
There was a problem hiding this comment.
No, it will be deleted during that work. No longer supported when we upgrade Redux.
| withConflicts = [], | ||
| onlyInCumulus = [], | ||
| onlyInOrca = [] | ||
| } = granules; |
There was a problem hiding this comment.
Wouldn't granules always be an empty object here?
There was a problem hiding this comment.
It looks like we are assigning a default value to granules, @jjmccoy correct me if I am wrong. and since export is just a declaration, you are basically saying if granules is empty, just assign empty list to withConflicts, etc
There was a problem hiding this comment.
@acyu Yes. The granule a value but if it doesn't when checks for conflicts then it's an empty array for those. If there are no conflicts at all or in Cumulus or in Orca then nothing should show for granule count in the table.
| </div> | ||
| const rulesOps = { | ||
| list: rules.list, | ||
| enabled: rules.enabled || {}, |
There was a problem hiding this comment.
Is an object the correct type to assign if rules.enabled is false? Shouldn't it be false? Same question for the next two...
| search: locationQueryParams.search[path] || getPersistentQueryParams(routeLocation), | ||
| })} | ||
| to={location} | ||
| onClick={(e) => { |
There was a problem hiding this comment.
Does this fix an issue? Was there a conflict here where e.g. clicking the link was doing something unexpected that needed to be prevented?
app/src/js/components/oauth.js
Outdated
| let state = {}; | ||
| try { | ||
| state = JSON.parse(decodeURIComponent(stateParam)); | ||
| console.log('Parsed state:', state); |
There was a problem hiding this comment.
Does the dash have debugging set up? Can we have log stuff like this only if e.g. the user has debugging set to "1"?
There was a problem hiding this comment.
Not that I know of. I don't see much on the dashboard but we should have it though. I wanted to make sure since this is authentication. I will set a variable for that level.
There was a problem hiding this comment.
Ok, in that case we should probably remove this logging. I'm not sure if we want to dump the state to the console in prod...
app/src/js/components/oauth.js
Outdated
| state = JSON.parse(decodeURIComponent(stateParam)); | ||
| console.log('Parsed state:', state); | ||
| } catch (error) { | ||
| console.error('Error parsing state:', error); |
There was a problem hiding this comment.
What would cause an error parsing the state? An invalid param? If that does happen, should the user be presented with an error?
There was a problem hiding this comment.
Yes, if the state parameter contains an invalid URL or param. I would think so but that would be another mechanism outside of console debugging.
There was a problem hiding this comment.
Hmm does the dash have other ways of presenting errors like this? I'm more comfortable logging errors than what we're doing on line 38 so it's not a deal-breaker but prefer more user-focused error messaging (e.g. modal) if we really need to display an error.
app/src/js/withUrlHelper.js
Outdated
| @@ -0,0 +1,87 @@ | |||
| /* eslint-disable import/no-cycle */ | |||
There was a problem hiding this comment.
I don't think we need this, do we? There shouldn't be cyclic dependencies if this is a module we're creating and we've only imported third-party modules?
There was a problem hiding this comment.
I will test again and see.
There was a problem hiding this comment.
Removed. Tested. Looks good. Change committed.
There was a problem hiding this comment.
Nice. And this may be an example of an unused disable. Perhaps we can turn that warning back on if we remove these?
|
|
||
| it('displays a compatible Cumulus API Version number', () => { | ||
| const apiVersionNumber = 'a.b.c'; | ||
| const apiVersionNumber = 'a.b.c-d.e'; |
There was a problem hiding this comment.
Did we change the way the version number is displayed?
There was a problem hiding this comment.
Yes, it was changed in a previous ticket.
There was a problem hiding this comment.
Is that previous ticket a part of this PR? I guess my question is: why is the change here?
There was a problem hiding this comment.
The previous ticket is not part of this PR. The test kept failing at this step. My commit even states it was to fix test errors.
There was a problem hiding this comment.
Hmm so... what happened? The test was failing on main?
There was a problem hiding this comment.
When I ran cypress main_page_spec.js was failing and it was because of apiVersionNumber had a dash in it.
| search: {} | ||
| }; | ||
|
|
||
| const urlHelper = { |
There was a problem hiding this comment.
Why do we need to mock the urlHelper? What would be the problem if we imported the actual module?
There was a problem hiding this comment.
I thought it was the better way to integrate it with what was here since the Collection List component uses urlHelper as a prop.
There was a problem hiding this comment.
What would happen if we imported and used the real urlHelper instead of a mock?
| workflowOptions={[]} | ||
| /> | ||
| </MemoryRouter> | ||
| <MemoryRouter initialEntries={['/granules/my-granule-id']}> |
There was a problem hiding this comment.
This looks like something we could store as a variable and use both here and in the previous assertion?
| }))(ReconciliationReport) | ||
| ); | ||
|
|
||
| export default connect(mapStateToProps)(ReconciliationReport); |
There was a problem hiding this comment.
Trying to understand why doesn't reconciliation-report need to use withUrlHelper, it looks like it's one of the route element defined in the index
There was a problem hiding this comment.
I didn't update any components that didn't have the urlHelper for routing originally.
Summary: React Router from v5 to v6
Addresses CUMULUS-3862: Upgrade React Router from v5 to v6
Changes
PR Checklist