Skip to content

Commit b6146df

Browse files
brettkolodnybrettkolodny
authored
chore: remove circular dependencies (#17585)
I've been bit in the past by hard to deduce bugs caused by circular dependencies within TS projects. On a hunch that this could be contributing to some flaky tests I've used the tool [dpdm](https://github.com/acrazing/dpdm) to find and remove them. This PR does the following: - Move around exports/create new files to remove any non-type circular depencies - Add dpdm as a dev dependency and create the `check:circular-depency` pnpm script
1 parent 1258902 commit b6146df

Some content is hidden

Large Commits have some content hidden by default. Use the searchbox below for content that may be hidden.

42 files changed

+180
-75
lines changed

‎site/package.json

+3-1
Original file line numberDiff line numberDiff line change
@@ -13,8 +13,9 @@
1313
"dev": "vite",
1414
"format": "biome format --write .",
1515
"format:check": "biome format .",
16-
"lint": "pnpm run lint:check && pnpm run lint:types",
16+
"lint": "pnpm run lint:check && pnpm run lint:types && pnpm run lint:circular-deps",
1717
"lint:check": " biome lint --error-on-warnings .",
18+
"lint:circular-deps": "dpdm --no-tree --no-warning -T ./src/App.tsx",
1819
"lint:fix": " biome lint --error-on-warnings --write .",
1920
"lint:types": "tsc -p .",
2021
"playwright:install": "playwright install --with-deps chromium",
@@ -171,6 +172,7 @@
171172
"@vitejs/plugin-react": "4.3.4",
172173
"autoprefixer": "10.4.20",
173174
"chromatic": "11.25.2",
175+
"dpdm": "3.14.0",
174176
"express": "4.21.2",
175177
"jest": "29.7.0",
176178
"jest-canvas-mock": "2.5.2",

‎site/pnpm-lock.yaml

+97
Some generated files are not rendered by default. Learn more about customizing how changed files appear on GitHub.

‎site/src/components/Filter/UserFilter.tsx

+1-1
Original file line numberDiff line numberDiff line change
@@ -5,7 +5,7 @@ import {
55
type SelectFilterOption,
66
SelectFilterSearch,
77
} from "components/Filter/SelectFilter";
8-
import { useAuthenticated } from "contexts/auth/RequireAuth";
8+
import { useAuthenticated } from "hooks";
99
import type { FC } from "react";
1010
import { type UseFilterMenuOptions, useFilterMenu } from "./menu";
1111

‎site/src/contexts/ProxyContext.tsx

+1-1
Original file line numberDiff line numberDiff line change
@@ -1,7 +1,7 @@
11
import { API } from "api/api";
22
import { cachedQuery } from "api/queries/util";
33
import type { Region, WorkspaceProxy } from "api/typesGenerated";
4-
import { useAuthenticated } from "contexts/auth/RequireAuth";
4+
import { useAuthenticated } from "hooks";
55
import { useEmbeddedMetadata } from "hooks/useEmbeddedMetadata";
66
import {
77
type FC,

‎site/src/contexts/auth/RequireAuth.test.tsx

+1-1
Original file line numberDiff line numberDiff line change
@@ -1,4 +1,5 @@
11
import { renderHook, screen } from "@testing-library/react";
2+
import { useAuthenticated } from "hooks";
23
import { http, HttpResponse } from "msw";
34
import type { FC, PropsWithChildren } from "react";
45
import { QueryClientProvider } from "react-query";
@@ -9,7 +10,6 @@ import {
910
} from "testHelpers/renderHelpers";
1011
import { server } from "testHelpers/server";
1112
import { AuthContext, type AuthContextValue } from "./AuthProvider";
12-
import { useAuthenticated } from "./RequireAuth";
1313

1414
describe("RequireAuth", () => {
1515
it("redirects to /login if user is not authenticated", async () => {

‎site/src/contexts/auth/RequireAuth.tsx

+1-26
Original file line numberDiff line numberDiff line change
@@ -6,7 +6,7 @@ import { DashboardProvider as ProductionDashboardProvider } from "modules/dashbo
66
import { type FC, useEffect } from "react";
77
import { Navigate, Outlet, useLocation } from "react-router-dom";
88
import { embedRedirect } from "utils/redirect";
9-
import { type AuthContextValue, useAuthContext } from "./AuthProvider";
9+
import { useAuthContext } from "./AuthProvider";
1010

1111
type RequireAuthProps = Readonly<{
1212
ProxyProvider?: typeof ProductionProxyProvider;
@@ -81,28 +81,3 @@ export const RequireAuth: FC<RequireAuthProps> = ({
8181
</DashboardProvider>
8282
);
8383
};
84-
85-
type RequireKeys<T, R extends keyof T> = Omit<T, R> & {
86-
[K in keyof Pick<T, R>]-?: NonNullable<T[K]>;
87-
};
88-
89-
// We can do some TS magic here but I would rather to be explicit on what
90-
// values are not undefined when authenticated
91-
type AuthenticatedAuthContextValue = RequireKeys<
92-
AuthContextValue,
93-
"user" | "permissions"
94-
>;
95-
96-
export const useAuthenticated = (): AuthenticatedAuthContextValue => {
97-
const auth = useAuthContext();
98-
99-
if (!auth.user) {
100-
throw new Error("User is not authenticated.");
101-
}
102-
103-
if (!auth.permissions) {
104-
throw new Error("Permissions are not available.");
105-
}
106-
107-
return auth as AuthenticatedAuthContextValue;
108-
};

‎site/src/hooks/index.ts

+1
Original file line numberDiff line numberDiff line change
@@ -1,3 +1,4 @@
1+
export * from "./useAuthenticated";
12
export * from "./useClickable";
23
export * from "./useClickableTableRow";
34
export * from "./useClipboard";

‎site/src/hooks/useAuthenticated.tsx

+29
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,29 @@
1+
import {
2+
type AuthContextValue,
3+
useAuthContext,
4+
} from "contexts/auth/AuthProvider";
5+
6+
type RequireKeys<T, R extends keyof T> = Omit<T, R> & {
7+
[K in keyof Pick<T, R>]-?: NonNullable<T[K]>;
8+
};
9+
10+
// We can do some TS magic here but I would rather to be explicit on what
11+
// values are not undefined when authenticated
12+
type AuthenticatedAuthContextValue = RequireKeys<
13+
AuthContextValue,
14+
"user" | "permissions"
15+
>;
16+
17+
export const useAuthenticated = (): AuthenticatedAuthContextValue => {
18+
const auth = useAuthContext();
19+
20+
if (!auth.user) {
21+
throw new Error("User is not authenticated.");
22+
}
23+
24+
if (!auth.permissions) {
25+
throw new Error("Permissions are not available.");
26+
}
27+
28+
return auth as AuthenticatedAuthContextValue;
29+
};

‎site/src/modules/dashboard/DashboardLayout.tsx

+1-1
Original file line numberDiff line numberDiff line change
@@ -3,7 +3,7 @@ import Link from "@mui/material/Link";
33
import Snackbar from "@mui/material/Snackbar";
44
import { Button } from "components/Button/Button";
55
import { Loader } from "components/Loader/Loader";
6-
import { useAuthenticated } from "contexts/auth/RequireAuth";
6+
import { useAuthenticated } from "hooks";
77
import { AnnouncementBanners } from "modules/dashboard/AnnouncementBanners/AnnouncementBanners";
88
import { LicenseBanner } from "modules/dashboard/LicenseBanner/LicenseBanner";
99
import { type FC, type HTMLAttributes, Suspense } from "react";

‎site/src/modules/dashboard/DashboardProvider.tsx

+1-1
Original file line numberDiff line numberDiff line change
@@ -10,7 +10,7 @@ import type {
1010
} from "api/typesGenerated";
1111
import { ErrorAlert } from "components/Alert/ErrorAlert";
1212
import { Loader } from "components/Loader/Loader";
13-
import { useAuthenticated } from "contexts/auth/RequireAuth";
13+
import { useAuthenticated } from "hooks";
1414
import { useEmbeddedMetadata } from "hooks/useEmbeddedMetadata";
1515
import { canViewAnyOrganization } from "modules/permissions";
1616
import { type FC, type PropsWithChildren, createContext } from "react";

‎site/src/modules/dashboard/DeploymentBanner/DeploymentBanner.tsx

+1-1
Original file line numberDiff line numberDiff line change
@@ -1,6 +1,6 @@
11
import { health } from "api/queries/debug";
22
import { deploymentStats } from "api/queries/deployment";
3-
import { useAuthenticated } from "contexts/auth/RequireAuth";
3+
import { useAuthenticated } from "hooks";
44
import type { FC } from "react";
55
import { useQuery } from "react-query";
66
import { DeploymentBannerView } from "./DeploymentBannerView";

0 commit comments

Comments
 (0)