-
Notifications
You must be signed in to change notification settings - Fork 10
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
[risk=low][RW-13601] Upgrade react-router to 6 #8780
base: main
Are you sure you want to change the base?
Conversation
Route -> CompatRoute use Navigate instead of Redirect
@@ -43,7 +43,7 @@ | |||
"@types/react-dom": "18.2.0", | |||
"@types/react-modal": "^3.16.3", | |||
"@types/react-onclickoutside": "^6.7.10", | |||
"@types/react-router-dom": "^5.3.2", | |||
"@types/react-router-dom": "^5.3.3", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
just matches the version we were already using
import * as fp from 'lodash/fp'; | ||
|
||
import { AuditAction, AuditLogEntry } from 'generated/fetch'; | ||
|
||
import { AuditActionCardListView } from 'app/components/admin/audit-card-list-view'; | ||
import { Navigate } from 'app/components/app-router'; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
note that this is not the same component. It's a custom wrapper that we wrote.
However: it was only used here, and I tested that the functionality didn't change by using the (new) native React Router component
@@ -2,21 +2,12 @@ import '@testing-library/jest-dom'; | |||
|
|||
import * as React from 'react'; | |||
import { MemoryRouter } from 'react-router'; | |||
import { Redirect, Switch } from 'react-router-dom'; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Navigate is the replacement for Redirect
return { | ||
__esModule: true, | ||
...originalModule, | ||
BrowserRouter: ({ children }) => <div>{children}</div>, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
don't need this anymore because BrowserRouter is gone. My understanding is that it was a leftover shim from the Angular days
|
||
describe('AppRouter', () => { | ||
const component = (initialEntries: string[], initialIndex: number) => { | ||
const TestAppRouter = () => ( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
no changes, just componentizing it. (hide whitespace)
@@ -77,43 +65,9 @@ export const withFullHeight = | |||
); | |||
}; | |||
|
|||
// This function is invoked if react-router `<Prompt>` is rendered by a component that wants the user to |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
unused. No components use <Prompt>
|
||
export const AppRouter = ({ children }): React.ReactElement => { | ||
return ( | ||
<BrowserRouter getUserConfirmation={getUserConfirmation}> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
don't need this anymore, because it was only used to wrap getUserConfirmation()
@@ -145,17 +99,12 @@ export const AppRoute = ({ | |||
fp.find(({ allowed }) => !allowed(), guards) || {}; | |||
|
|||
return ( | |||
<Route exact={exact} path={path}> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
changing Route to CompatRoute is part of the migration path.
CompatRoute has both v5 and v6 apis, so we can convert at whatever pace is appropriate, then convert to a v6 Route when done
); | ||
}; | ||
|
||
export const Navigate = ({ to }): React.ReactElement => { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
only used by AuditPageComponent, and we were not using the wrapped functionality (location) so we can use the native version instead
@@ -95,19 +96,22 @@ describe('NotebookLauncher', () => { | |||
)}/${notebookName}`; | |||
const history = createMemoryHistory({ initialEntries: [notebookInitialUrl] }); | |||
|
|||
const notebookComponent = () => { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
misc cleanup
DOES NOT WORK
PR checklist
[risk=no|low|moderate|severe]
in the PR title as outlined in CONTRIBUTING.md.