Skip to content

Commit

Permalink
Actual react-router-dom upgrade (#6504)
Browse files Browse the repository at this point in the history 
### Description of the change

After the prequal work, this PR does the actual upgrade of
react-router-dom, including updating all the use of `Route`, as well as
removing connected-react-router and the remaining container components.
Mostly following from their docs for the upgrade at:
https://reactrouter.com/en/main/upgrading/v5#upgrade-to-react-router-v6
with some custom changes required by our specific test infrastructure.

### Benefits

Finally upgrade react-router and upgrade to hook-based components
(something that has been long-running tech-debt slowing us down).

### Possible drawbacks

There could be some fall-out of small UX issues that weren't caught in
CI. I know of one that I'll fix straight away (two requests sent off for
available packages when viewing the catalog - one for all apps in all
namespaces).

### Applicable issues

- fixes #6187

---------

Signed-off-by: Michael Nelson <minelson@vmware.com>
  • Loading branch information
@absoludity
absoludity committed Aug 3, 2023
1 parent 99332c4 commit 7b4a984
Show file tree
Hide file tree
Showing 62 changed files with 1,634 additions and 1,531 deletions.
1 change: 0 additions & 1 deletion cmd/kubeapps-apis/plugins/helm/packages/v1alpha1/server.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -626,7 +626,6 @@ func (s *Server) GetInstalledPackageDetail(ctx context.Context, request *connect
Namespace: release.Namespace,
// For OCI charts, the name is *not* just the chart name from the release,
// but includes the repo and project: `test-oci/kubeapps%2Fsimplechart`
// UPTOHERE: Investigate what happened in kubeops when this last worked perhaps?
ChartName: release.Chart.Metadata.Name,
Version: release.Chart.Metadata.Version,
AppVersion: release.Chart.Metadata.AppVersion,
Expand Down
8 changes: 4 additions & 4 deletions dashboard/package.json
View file
Original file line number Diff line number Diff line change
Expand Up @@ -37,13 +37,10 @@
"@paciolan/remote-component": "^2.13.0",
"@tanstack/match-sorter-utils": "^8.7.6",
"@tanstack/react-table": "^8.9.3",
"@testing-library/jest-dom": "^5.16.5",
"ajv": "^8.12.0",
"axios": "^1.3.5",
"connected-react-router": "^6.9.3",
"fast-json-patch": "^3.1.1",
"google-protobuf": "^3.21.2",
"history": "^4.10.1",
"jsonwebtoken": "^9.0.1",
"lodash": "^4.17.21",
"lodash-es": "^4.17.21",
Expand All @@ -61,7 +58,7 @@
"react-minimal-pie-chart": "^8.4.0",
"react-monaco-editor": "^0.51.0",
"react-redux": "^7.2.9",
"react-router-dom": "^5.3.0",
"react-router-dom": "^6.14.1",
"react-router-hash-link": "^2.4.3",
"react-tooltip": "^5.19.0",
"react-transition-group": "^4.4.5",
Expand All @@ -83,7 +80,9 @@
"@craco/craco": "^7.1.0",
"@formatjs/cli": "^6.1.3",
"@reduxjs/toolkit": "^1.9.5",
"@testing-library/jest-dom": "^5.16.5",
"@testing-library/react": "^12.1.5",
"@testing-library/user-event": "^14.4.3",
"@types/enzyme": "^3.10.13",
"@types/jest": "^29.5.3",
"@types/jsonwebtoken": "^9.0.2",
Expand Down Expand Up @@ -119,6 +118,7 @@
"react-scripts": "^5.0.1",
"react-test-renderer": "^17.0.2",
"redux-mock-store": "^1.5.4",
"resize-observer-polyfill": "^1.5.1",
"sass": "^1.64.2",
"shx": "^0.3.4",
"stylelint": "^15.10.2",
Expand Down
5 changes: 0 additions & 5 deletions dashboard/src/actions/index.ts
View file
Original file line number Diff line number Diff line change
@@ -1,7 +1,6 @@
// Copyright 2018-2022 the Kubeapps contributors.
// SPDX-License-Identifier: Apache-2.0

import { push } from "connected-react-router";
import * as installedpackages from "./installedpackages";
import * as auth from "./auth";
import * as availablepackages from "./availablepackages";
Expand All @@ -20,8 +19,4 @@ export default {
namespace,
operators,
repos,
shared: {
pushSearchFilter: (f: string) => push(`?q=${f}`),
pushAllNSFilter: (y: boolean) => push(`?allns=${y ? "yes" : "no"}`),
},
};
6 changes: 6 additions & 0 deletions dashboard/src/components/AppList/AppList.test.tsx
View file
Original file line number Diff line number Diff line change
Expand Up @@ -91,6 +91,7 @@ context("when changing props", () => {
<MemoryRouter initialEntries={["/foo?q=foo"]}>
<AppList />
</MemoryRouter>,
false,
);
expect(wrapper.find(SearchFilter).prop("value")).toEqual("foo");
});
Expand All @@ -104,6 +105,7 @@ context("when changing props", () => {
<MemoryRouter initialEntries={["/foo?allns=yes"]}>
<AppList />
</MemoryRouter>,
false,
);
expect(wrapper.find("input[type='checkbox']")).toBeChecked();
});
Expand Down Expand Up @@ -158,6 +160,7 @@ context("when changing props", () => {
<MemoryRouter initialEntries={["/foo?allns=yes"]}>
<AppList />
</MemoryRouter>,
false,
);
expect(setAllNS).not.toHaveBeenCalledWith(false);
});
Expand Down Expand Up @@ -300,6 +303,7 @@ context("when apps available", () => {
<MemoryRouter initialEntries={["/foo?q=bar"]}>
<AppList />
</MemoryRouter>,
false,
);
expect(wrapper.find(AppListItem).key()).toBe("fooNs-foobar/bar");
});
Expand Down Expand Up @@ -366,6 +370,7 @@ context("when apps available", () => {
<MemoryRouter initialEntries={["/foo?q=bar"]}>
<AppList />
</MemoryRouter>,
false,
);
expect(wrapper.find(AppListItem).first().key()).toBe("fooNs-foobar/bar");
expect(wrapper.find(AppListItem).last().key()).toBe("barNs-foobar/bar");
Expand Down Expand Up @@ -418,6 +423,7 @@ context("when custom resources available", () => {
<MemoryRouter initialEntries={["/foo?q=nop"]}>
<AppList />
</MemoryRouter>,
false,
);
const itemList = wrapper.find(CustomResourceListItem);
expect(itemList).not.toExist();
Expand Down
5 changes: 3 additions & 2 deletions dashboard/src/components/AppList/AppList.tsx
View file
Original file line number Diff line number Diff line change
Expand Up @@ -7,7 +7,7 @@ import { CdsToggle, CdsToggleGroup } from "@cds/react/toggle";
import actions from "actions";
import ErrorAlert from "components/ErrorAlert";
import LoadingWrapper from "components/LoadingWrapper/LoadingWrapper";
import { push } from "connected-react-router";
import { usePush } from "hooks/push";
import qs from "qs";
import { useEffect, useState } from "react";
import { useDispatch, useSelector } from "react-redux";
Expand Down Expand Up @@ -44,6 +44,7 @@ function AppList() {
submitFilters(!allNS);
setAllNS(!allNS);
};
const push = usePush();

const submitFilters = (allns: boolean) => {
const filters = [];
Expand All @@ -55,7 +56,7 @@ function AppList() {
if (searchFilter) {
filters.push(`q=${searchFilter}`);
}
dispatch(push(`?${filters.join("&")}`));
push(`?${filters.join("&")}`);
};
const submitSearchFilter = () => submitFilters(allNS);

Expand Down
66 changes: 36 additions & 30 deletions dashboard/src/components/AppUpgrade/AppUpgrade.test.tsx
View file
Original file line number Diff line number Diff line change
Expand Up @@ -21,7 +21,7 @@ import {
import { Plugin } from "gen/kubeappsapis/core/plugins/v1alpha1/plugins_pb";
import * as ReactRedux from "react-redux";
import * as ReactRouter from "react-router";
import { MemoryRouter, Route } from "react-router-dom";
import { MemoryRouter, Route, Routes } from "react-router-dom";
import { IPackageRepositoryState } from "reducers/repos";
import { defaultStore, getStore, mountWrapper } from "shared/specs/mountWrapper";
import {
Expand Down Expand Up @@ -106,14 +106,12 @@ const repo1Detail = {
} as PackageRepositoryDetail;

let spyOnUseDispatch: jest.SpyInstance;
let spyOnUseHistory: jest.SpyInstance;
let spyOnUseNavigate: jest.SpyInstance;

beforeEach(() => {
const mockDispatch = jest.fn();
spyOnUseDispatch = jest.spyOn(ReactRedux, "useDispatch").mockReturnValue(mockDispatch);
spyOnUseHistory = jest
.spyOn(ReactRouter, "useHistory")
.mockReturnValue({ push: jest.fn() } as any);
spyOnUseNavigate = jest.spyOn(ReactRouter, "useNavigate").mockReturnValue(jest.fn());
// mock the window.matchMedia for selecting the theme
Object.defineProperty(window, "matchMedia", {
writable: true,
Expand Down Expand Up @@ -154,7 +152,7 @@ beforeEach(() => {
afterEach(() => {
jest.restoreAllMocks();
spyOnUseDispatch.mockRestore();
spyOnUseHistory.mockRestore();
spyOnUseNavigate.mockRestore();
});

const routePathParam = `/c/${defaultProps.cluster}/ns/${defaultProps.namespace}/apps/${defaultProps.plugin.name}/${defaultProps.plugin.version}/${defaultProps.releaseName}/upgrade`;
Expand All @@ -169,10 +167,11 @@ it("renders the repo selection form if not introduced", () => {
const wrapper = mountWrapper(
getStore({ ...defaultStore, ...state } as Partial<IStoreState>),
<MemoryRouter initialEntries={[routePathParam]}>
<Route path={routePath}>
<AppUpgrade />,
</Route>
<Routes>
<Route path={routePath} element={<AppUpgrade />} />
</Routes>
</MemoryRouter>,
false,
);
expect(wrapper.find(LoadingWrapper).prop("loaded")).toBe(false);
});
Expand All @@ -194,10 +193,11 @@ it("renders the repo selection form if not introduced when the app is loaded", (
...state,
} as Partial<IStoreState>),
<MemoryRouter initialEntries={[routePathParam]}>
<Route path={routePath}>
<AppUpgrade />,
</Route>
<Routes>
<Route path={routePath} element={<AppUpgrade />} />
</Routes>
</MemoryRouter>,
false,
);
expect(wrapper.find(SelectRepoForm)).toExist();
expect(wrapper.find(Alert)).not.toExist();
Expand All @@ -217,10 +217,11 @@ describe("when an error exists", () => {
...state,
} as Partial<IStoreState>),
<MemoryRouter initialEntries={[routePathParam]}>
<Route path={routePath}>
<AppUpgrade />,
</Route>
<Routes>
<Route path={routePath} element={<AppUpgrade />} />
</Routes>
</MemoryRouter>,
false,
);

expect(wrapper.find(Alert)).toExist();
Expand All @@ -247,10 +248,11 @@ describe("when an error exists", () => {
...state,
} as Partial<IStoreState>),
<MemoryRouter initialEntries={[routePathParam]}>
<Route path={routePath}>
<AppUpgrade />,
</Route>
<Routes>
<Route path={routePath} element={<AppUpgrade />} />
</Routes>
</MemoryRouter>,
false,
);
expect(wrapper.find(SelectRepoForm).find(Alert)).toExist();
expect(wrapper.find(UpgradeForm)).not.toExist();
Expand All @@ -275,10 +277,11 @@ describe("when an error exists", () => {
...state,
} as Partial<IStoreState>),
<MemoryRouter initialEntries={[routePathParam]}>
<Route path={routePath}>
<AppUpgrade />,
</Route>
<Routes>
<Route path={routePath} element={<AppUpgrade />} />
</Routes>
</MemoryRouter>,
false,
);
expect(wrapper.find(UpgradeForm)).toExist();
expect(wrapper.find(UpgradeForm).find(Alert)).toIncludeText(upgradeError.message);
Expand Down Expand Up @@ -310,10 +313,11 @@ it("renders the upgrade form when the repo is available, clears state and fetche
...state,
} as Partial<IStoreState>),
<MemoryRouter initialEntries={[routePathParam]}>
<Route path={routePath}>
<AppUpgrade />,
</Route>
<Routes>
<Route path={routePath} element={<AppUpgrade />} />
</Routes>
</MemoryRouter>,
false,
);
expect(wrapper.find(UpgradeForm)).toExist();
expect(wrapper.find(Alert)).not.toExist();
Expand Down Expand Up @@ -346,10 +350,11 @@ it("renders the upgrade form with the version property", () => {
...state,
} as Partial<IStoreState>),
<MemoryRouter initialEntries={[routePathParam + "/0.0.1"]}>
<Route path={routePath + "/:version"}>
<AppUpgrade />,
</Route>
<Routes>
<Route path={routePath + "/:version"} element={<AppUpgrade />} />
</Routes>
</MemoryRouter>,
false,
);
expect(wrapper.find(UpgradeForm)).toExist();
expect(wrapper.find(UpgradeForm)).toHaveProp("version", "0.0.1");
Expand All @@ -374,10 +379,11 @@ it("skips the repo selection form if the app contains upgrade info", () => {
...state,
} as Partial<IStoreState>),
<MemoryRouter initialEntries={[routePathParam]}>
<Route path={routePath}>
<AppUpgrade />,
</Route>
<Routes>
<Route path={routePath} element={<AppUpgrade />} />
</Routes>
</MemoryRouter>,
false,
);
expect(wrapper.find(UpgradeForm)).toExist();
expect(wrapper.find(Alert)).not.toExist();
Expand Down
15 changes: 3 additions & 12 deletions dashboard/src/components/AppUpgrade/AppUpgrade.tsx
View file
Original file line number Diff line number Diff line change
Expand Up @@ -15,19 +15,10 @@ import LoadingWrapper from "../LoadingWrapper/LoadingWrapper";
import SelectRepoForm from "../SelectRepoForm/SelectRepoForm";
import UpgradeForm from "../UpgradeForm/UpgradeForm";

interface IRouteParams {
cluster: string;
namespace: string;
releaseName: string;
pluginName: string;
pluginVersion: string;
version?: string;
}

function AppUpgrade() {
const dispatch: ThunkDispatch<IStoreState, null, Action> = useDispatch();
const { cluster, namespace, releaseName, pluginName, pluginVersion, version } =
ReactRouter.useParams() as IRouteParams;
ReactRouter.useParams();

const {
apps: {
Expand Down Expand Up @@ -81,8 +72,8 @@ function AppUpgrade() {
}
return (
<SelectRepoForm
cluster={cluster}
namespace={namespace}
cluster={cluster || ""}
namespace={namespace || ""}
app={installedAppInstalledPackageDetail}
/>
);
Expand Down
12 changes: 4 additions & 8 deletions dashboard/src/components/AppView/AppControls/DeleteButton/DeleteButton.tsx
View file
Original file line number Diff line number Diff line change
Expand Up @@ -4,7 +4,7 @@
import { CdsIcon } from "@cds/react/icon";
import actions from "actions";
import ConfirmDialog from "components/ConfirmDialog/ConfirmDialog";
import { push } from "connected-react-router";
import { usePush } from "hooks/push";
import {
InstalledPackageReference,
InstalledPackageStatus,
Expand Down Expand Up @@ -33,6 +33,7 @@ export default function DeleteButton({
const [deleting, setDeleting] = useState(false);
const dispatch: ThunkDispatch<IStoreState, null, Action> = useDispatch();
const error = useSelector((state: IStoreState) => state.apps.error);
const push = usePush();

const openModal = () => setModal(true);
const closeModal = () => setModal(false);
Expand All @@ -43,13 +44,8 @@ export default function DeleteButton({
);
setDeleting(false);
if (deleted) {
dispatch(
push(
app.apps.list(
installedPackageRef.context?.cluster,
installedPackageRef.context?.namespace,
),
),
push(
app.apps.list(installedPackageRef.context?.cluster, installedPackageRef.context?.namespace),
);
}
};
Expand Down
Loading

0 comments on commit 7b4a984

Please sign in to comment.