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

[risk=low][RW-13601] Upgrade react-router to 6 #8780

Draft
wants to merge 4 commits into
base: main
Choose a base branch
from
Draft
Show file tree
Hide file tree
Changes from 2 commits
Commits
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
3 changes: 2 additions & 1 deletion ui/package.json
Original file line number Diff line number Diff line change
Expand Up @@ -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",
Copy link
Collaborator Author

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

"color": "^3.1.1",
"highcharts": "^9.3.3",
"highcharts-react-official": "^3.2.1",
Expand All @@ -70,6 +70,7 @@
"react-router": "^5.3.4",
"react-router-dom": "^5.3.4",
"react-router-hash-link": "^2.4.3",
"react-router-dom-v5-compat": "^6.26.2",
"react-select": "^5",
"react-spring": "8.0.27",
"react-switch": "^6",
Expand Down
2 changes: 1 addition & 1 deletion ui/src/app/components/admin/audit-page-component.tsx
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';
Copy link
Collaborator Author

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

import {
Button,
StyledExternalLink,
Expand Down
138 changes: 70 additions & 68 deletions ui/src/app/components/app-router.spec.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -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';
Copy link
Collaborator Author

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

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';
Expand All @@ -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>;
Expand All @@ -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) => {
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

initialIndex was not used

const TestAppRouter = () => (
Copy link
Collaborator Author

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)

<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();
});
});
31 changes: 9 additions & 22 deletions ui/src/app/components/app-router.tsx
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';
Expand All @@ -26,11 +23,6 @@ export interface Guard {
renderBlocked?: () => React.ReactElement;
}

export const usePath = () => {
Copy link
Collaborator Author

Choose a reason for hiding this comment

The 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);
};
Expand Down Expand Up @@ -110,7 +102,7 @@ const getUserConfirmation = (message, callback) => {
export const AppRouter = ({ children }): React.ReactElement => {
return (
<BrowserRouter getUserConfirmation={getUserConfirmation}>
Copy link
Collaborator Author

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()

{children}
<CompatRouter>{children}</CompatRouter>
</BrowserRouter>
);
};
Expand Down Expand Up @@ -145,17 +137,12 @@ export const AppRoute = ({
fp.find(({ allowed }) => !allowed(), guards) || {};

return (
<Route exact={exact} path={path}>
Copy link
Collaborator Author

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

<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 => {
Copy link
Collaborator Author

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

const location = useLocation();
return <Redirect to={{ pathname: to, state: { from: location } }} />;
};
5 changes: 1 addition & 4 deletions ui/src/app/pages/data/cohort/variant-search.spec.tsx
Original file line number Diff line number Diff line change
@@ -1,7 +1,6 @@
import '@testing-library/jest-dom';

import * as React from 'react';
import { BrowserRouter } from 'react-router-dom';

import { CohortBuilderApi } from 'generated/fetch';

Expand All @@ -25,9 +24,7 @@ describe('VariantSearch', () => {
});
const component = () => {
return renderWithRouter(
<BrowserRouter>
<VariantSearch select={() => {}} selectedIds={[]} />
</BrowserRouter>
<VariantSearch select={() => {}} selectedIds={[]} />
);
};

Expand Down
7 changes: 4 additions & 3 deletions ui/src/app/routing/signed-in-app-routing.tsx
Original file line number Diff line number Diff line change
@@ -1,5 +1,6 @@
import * as React from 'react';
import { Redirect, Switch, useLocation } from 'react-router-dom';
import { Switch, useLocation } from 'react-router-dom';
import { Navigate } from 'react-router-dom-v5-compat';
import * as fp from 'lodash/fp';

import {
Expand Down Expand Up @@ -204,7 +205,7 @@ export const SignedInRoutes = () => {
<AppRoute exact path='/admin/user'>
{' '}
{/* included for backwards compatibility */}
<Redirect to={'/admin/users'} />
<Navigate to={'/admin/users'} />
</AppRoute>
<AppRoute
exact
Expand Down Expand Up @@ -382,7 +383,7 @@ export const SignedInRoutes = () => {
/>
</AppRoute>
<AppRoute exact path='*'>
<Redirect to={'/not-found'} />
<Navigate to={'/not-found'} />
</AppRoute>
</Switch>
);
Expand Down
5 changes: 3 additions & 2 deletions ui/src/app/routing/workspace-app-routing.tsx
Original file line number Diff line number Diff line change
@@ -1,5 +1,6 @@
import * as React from 'react';
import { Redirect, Switch, useParams, useRouteMatch } from 'react-router-dom';
import { Switch, useParams, useRouteMatch } from 'react-router-dom';
import { Navigate } from 'react-router-dom-v5-compat';
import * as fp from 'lodash/fp';

import { AppRoute, withRouteData } from 'app/components/app-router';
Expand Down Expand Up @@ -489,7 +490,7 @@ export const WorkspaceRoutes = () => {
/>
</AppRoute>
<AppRoute exact={false} path={`${path}`}>
<Redirect to={'/not-found'} />
<Navigate to={'/not-found'} />
</AppRoute>
</Switch>
);
Expand Down
4 changes: 2 additions & 2 deletions ui/src/app/utils/access-utils.tsx
Original file line number Diff line number Diff line change
@@ -1,5 +1,5 @@
import * as React from 'react';
import { Redirect } from 'react-router-dom';
import { Navigate } from 'react-router-dom-v5-compat';
import * as fp from 'lodash/fp';

import {
Expand Down Expand Up @@ -131,7 +131,7 @@ export const redirectToRas = (openInNewTab: boolean = true): void => {
buildRasRedirectUrl() +
'&response_type=code&scope=openid+profile+email+federated_identities';

openInNewTab ? window.open(url, '_blank') : <Redirect to={url} />;
openInNewTab ? window.open(url, '_blank') : <Navigate to={url} />;
};

export const DATA_ACCESS_REQUIREMENTS_PATH = '/data-access-requirements';
Expand Down
Loading