-
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?
Changes from 2 commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -1,10 +1,10 @@ | ||
import * as React from 'react'; | ||
import { Navigate } from 'react-router-dom-v5-compat'; | ||
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 commentThe 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 |
||
import { | ||
Button, | ||
StyledExternalLink, | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -2,7 +2,8 @@ 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 commentThe reason will be displayed to describe this comment to others. Learn more. Navigate is the replacement for Redirect |
||
import { Switch } from 'react-router-dom'; | ||
import { Navigate } from 'react-router-dom-v5-compat'; | ||
|
||
import { render, screen } from '@testing-library/react'; | ||
import { AppRoute, AppRouter, Guard } from 'app/components/app-router'; | ||
|
@@ -17,6 +18,17 @@ jest.mock('react-router-dom', () => { | |
}; | ||
}); | ||
|
||
// unsure if this is needed | ||
jest.mock('react-router-dom-v5-compat', () => { | ||
const originalModule = jest.requireActual('react-router-dom-v5-compat'); | ||
|
||
return { | ||
__esModule: true, | ||
...originalModule, | ||
BrowserRouter: ({ children }) => <div>{children}</div>, | ||
}; | ||
}); | ||
|
||
class TestComponent extends React.Component<{ text: String }> { | ||
render() { | ||
return <span>{this.props.text}</span>; | ||
|
@@ -43,107 +55,97 @@ const renderingFalseGuard: Guard = { | |
renderBlocked: () => <span>guard component</span>, | ||
}; | ||
|
||
const makeAppRouter = () => { | ||
return ( | ||
<AppRouter> | ||
<Switch> | ||
<AppRoute exact path='/unprotected-route'> | ||
<TestComponent text={'Unprotected Route'} /> | ||
</AppRoute> | ||
<AppRoute exact path='/punting'> | ||
<TestComponent text={'Punting'} /> | ||
</AppRoute> | ||
<AppRoute exact path='/unreachable-path' guards={[alwaysFalseGuard]}> | ||
<TestComponent text={'Unreachable Path'} /> | ||
</AppRoute> | ||
<AppRoute exact path='/protected-route' guards={[alwaysTrueGuard]}> | ||
<TestComponent text={'Protected Route'} /> | ||
</AppRoute> | ||
<AppRoute | ||
exact | ||
path='/block-render-route' | ||
guards={[renderingFalseGuard]} | ||
> | ||
<TestComponent text={'Rendering Route'} /> | ||
</AppRoute> | ||
<AppRoute | ||
exact | ||
path='/other-protected-route' | ||
guards={[alwaysTrueGuard]} | ||
> | ||
<TestComponent text={'Other Protected Route'} /> | ||
</AppRoute> | ||
<AppRoute | ||
exact | ||
path='/nested-protected-route' | ||
guards={[alwaysTrueGuard, otherAlwaysTrueGuard]} | ||
> | ||
<TestComponent text={'Nested Protected Route'} /> | ||
</AppRoute> | ||
<AppRoute | ||
exact | ||
path='/nested-unreachable-path' | ||
guards={[alwaysTrueGuard, alwaysFalseGuard]} | ||
> | ||
<TestComponent text={'Unreachable Path'} /> | ||
</AppRoute> | ||
<AppRoute exact path='/not-found'> | ||
<TestComponent text={'Not Found'} /> | ||
</AppRoute> | ||
<AppRoute exact path='*'> | ||
<Redirect to='/not-found' /> | ||
</AppRoute> | ||
</Switch> | ||
</AppRouter> | ||
); | ||
}; | ||
|
||
describe('AppRouter', () => { | ||
const component = (initialEntries: string[], initialIndex: number) => { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. initialIndex was not used |
||
const TestAppRouter = () => ( | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. no changes, just componentizing it. (hide whitespace) |
||
<AppRouter> | ||
<Switch> | ||
<AppRoute exact path='/unprotected-route'> | ||
<TestComponent text={'Unprotected Route'} /> | ||
</AppRoute> | ||
<AppRoute exact path='/punting'> | ||
<TestComponent text={'Punting'} /> | ||
</AppRoute> | ||
<AppRoute exact path='/unreachable-path' guards={[alwaysFalseGuard]}> | ||
<TestComponent text={'Unreachable Path'} /> | ||
</AppRoute> | ||
<AppRoute exact path='/protected-route' guards={[alwaysTrueGuard]}> | ||
<TestComponent text={'Protected Route'} /> | ||
</AppRoute> | ||
<AppRoute exact path='/block-render-route' guards={[renderingFalseGuard]}> | ||
<TestComponent text={'Rendering Route'} /> | ||
</AppRoute> | ||
<AppRoute exact path='/other-protected-route' guards={[alwaysTrueGuard]}> | ||
<TestComponent text={'Other Protected Route'} /> | ||
</AppRoute> | ||
<AppRoute | ||
exact | ||
path='/nested-protected-route' | ||
guards={[alwaysTrueGuard, otherAlwaysTrueGuard]} | ||
> | ||
<TestComponent text={'Nested Protected Route'} /> | ||
</AppRoute> | ||
<AppRoute | ||
exact | ||
path='/nested-unreachable-path' | ||
guards={[alwaysTrueGuard, alwaysFalseGuard]} | ||
> | ||
<TestComponent text={'Unreachable Path'} /> | ||
</AppRoute> | ||
<AppRoute exact path='/not-found'> | ||
<TestComponent text={'Not Found'} /> | ||
</AppRoute> | ||
<AppRoute exact path='*'> | ||
<Navigate to='/not-found' /> | ||
</AppRoute> | ||
</Switch> | ||
</AppRouter> | ||
); | ||
|
||
describe(AppRouter.name, () => { | ||
const component = (initialEntries: string[]) => { | ||
return render( | ||
<MemoryRouter initialEntries={initialEntries} initialIndex={initialIndex}> | ||
{makeAppRouter()} | ||
<MemoryRouter initialEntries={initialEntries}> | ||
<TestAppRouter /> | ||
</MemoryRouter> | ||
); | ||
}; | ||
|
||
it('allows anyone into unprotected route', () => { | ||
component(['/unprotected-route'], 0); | ||
component(['/unprotected-route']); | ||
expect(screen.getByText('Unprotected Route')).toBeInTheDocument(); | ||
}); | ||
|
||
it('punts when user fails guard', () => { | ||
component(['/unreachable-path'], 0); | ||
component(['/unreachable-path']); | ||
expect(screen.getByText('Punting')).toBeInTheDocument(); | ||
}); | ||
|
||
it('renders when user passes guard', () => { | ||
component(['/protected-route'], 0); | ||
component(['/protected-route']); | ||
expect(screen.getByText('Protected Route')).toBeInTheDocument(); | ||
}); | ||
|
||
it('renders for sibling routes under a guard', () => { | ||
component(['/other-protected-route'], 0); | ||
component(['/other-protected-route']); | ||
expect(screen.getByText('Other Protected Route')).toBeInTheDocument(); | ||
}); | ||
|
||
it('renders for nested protectedRoutes', () => { | ||
component(['/nested-protected-route'], 0); | ||
component(['/nested-protected-route']); | ||
expect(screen.getByText('Nested Protected Route')).toBeInTheDocument(); | ||
}); | ||
|
||
it('punts on failed nested protected route', () => { | ||
component(['/nested-unreachable-path'], 0); | ||
component(['/nested-unreachable-path']); | ||
expect(screen.getByText('Punting')).toBeInTheDocument(); | ||
}); | ||
|
||
it('redirects to not found page when no route is matched', async () => { | ||
component(['/wharrgarbl'], 0); | ||
component(['/wharrgarbl']); | ||
expect(await screen.findByText('Not Found')).toBeInTheDocument(); | ||
}); | ||
|
||
it('renders content when guard fails', () => { | ||
component(['/block-render-route'], 0); | ||
component(['/block-render-route']); | ||
expect(screen.getByText('guard component')).toBeInTheDocument(); | ||
}); | ||
}); |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -1,15 +1,12 @@ | ||
import * as React from 'react'; | ||
import { useEffect } from 'react'; | ||
import * as ReactDOM from 'react-dom'; | ||
import { BrowserRouter, Link, useLocation, useParams } from 'react-router-dom'; | ||
import { | ||
BrowserRouter, | ||
Link, | ||
Redirect, | ||
Route, | ||
useLocation, | ||
useParams, | ||
useRouteMatch, | ||
} from 'react-router-dom'; | ||
CompatRoute, | ||
CompatRouter, | ||
Navigate, | ||
} from 'react-router-dom-v5-compat'; | ||
import * as fp from 'lodash/fp'; | ||
|
||
import { cond } from '@terra-ui-packages/core-utils'; | ||
|
@@ -26,11 +23,6 @@ export interface Guard { | |
renderBlocked?: () => React.ReactElement; | ||
} | ||
|
||
export const usePath = () => { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. unused |
||
const { path } = useRouteMatch(); | ||
return path; | ||
}; | ||
|
||
export const parseQueryParams = (search: string) => { | ||
return new URLSearchParams(search); | ||
}; | ||
|
@@ -110,7 +102,7 @@ const getUserConfirmation = (message, callback) => { | |
export const AppRouter = ({ children }): React.ReactElement => { | ||
return ( | ||
<BrowserRouter getUserConfirmation={getUserConfirmation}> | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. don't need this anymore, because it was only used to wrap |
||
{children} | ||
<CompatRouter>{children}</CompatRouter> | ||
</BrowserRouter> | ||
); | ||
}; | ||
|
@@ -145,17 +137,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 commentThe reason will be displayed to describe this comment to others. Learn more. changing Route to CompatRoute is part of the migration path. |
||
<CompatRoute {...{ path, exact }}> | ||
{cond<React.ReactNode>( | ||
[redirectPath, () => <Redirect to={redirectPath} />], | ||
[redirectPath, () => <Navigate to={redirectPath} />], | ||
[renderBlocked, () => renderBlocked()], | ||
() => children | ||
)} | ||
</Route> | ||
</CompatRoute> | ||
); | ||
}; | ||
|
||
export const Navigate = ({ to }): React.ReactElement => { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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 |
||
const location = useLocation(); | ||
return <Redirect to={{ pathname: to, state: { from: location } }} />; | ||
}; |
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