From 7b0bb1f0a3b7bf220456d9273d7cb30ffbe1de15 Mon Sep 17 00:00:00 2001 From: Victor Lin <13424970+victorlin@users.noreply.github.com> Date: Thu, 14 Nov 2024 17:09:54 -0800 Subject: [PATCH 01/14] typescript-eslint: Replace recommended with strict ruleset I originally wanted `no-non-null-assertion`, then realized that it was part of the strict ruleset. All violations of this ruleset seem reasonable to address, which I will do in subsequent commits. --- static-site/.eslintrc.yaml | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/static-site/.eslintrc.yaml b/static-site/.eslintrc.yaml index 88cd73d72..967527ca6 100644 --- a/static-site/.eslintrc.yaml +++ b/static-site/.eslintrc.yaml @@ -10,7 +10,7 @@ extends: - eslint:recommended - plugin:react/recommended - plugin:react-hooks/recommended - - plugin:@typescript-eslint/recommended + - plugin:@typescript-eslint/strict # We use the recommended Next.js eslint configuration `next/core-web-vitals` as per # # As of April 2024, the extension simply adds the `@next/next/core-web-vitals` plugin: From 0af3c3a8c1f00861587c31687a3485723fb30763 Mon Sep 17 00:00:00 2001 From: Victor Lin <13424970+victorlin@users.noreply.github.com> Date: Tue, 19 Nov 2024 15:06:40 -0800 Subject: [PATCH 02/14] Move error boundary code to components level This makes it appropriate for other components outside of ListResources to re-use this functionality. --- .../components/{ListResources/errors.tsx => ErrorBoundary.tsx} | 2 +- static-site/src/components/ListResources/IndividualResource.tsx | 2 +- static-site/src/components/ListResources/Modal.tsx | 2 +- static-site/src/components/ListResources/ResourceGroup.tsx | 2 +- static-site/src/components/ListResources/index.tsx | 2 +- static-site/src/components/ListResources/types.ts | 2 +- 6 files changed, 6 insertions(+), 6 deletions(-) rename static-site/src/components/{ListResources/errors.tsx => ErrorBoundary.tsx} (96%) diff --git a/static-site/src/components/ListResources/errors.tsx b/static-site/src/components/ErrorBoundary.tsx similarity index 96% rename from static-site/src/components/ListResources/errors.tsx rename to static-site/src/components/ErrorBoundary.tsx index c4bd9eeff..7db7c692f 100644 --- a/static-site/src/components/ListResources/errors.tsx +++ b/static-site/src/components/ErrorBoundary.tsx @@ -1,5 +1,5 @@ import React, { ErrorInfo, ReactNode } from "react"; -import { ErrorContainer } from "../../pages/404"; +import { ErrorContainer } from "../pages/404"; export class InternalError extends Error { constructor(message: string) { diff --git a/static-site/src/components/ListResources/IndividualResource.tsx b/static-site/src/components/ListResources/IndividualResource.tsx index 2e1ad18f6..ad55d315b 100644 --- a/static-site/src/components/ListResources/IndividualResource.tsx +++ b/static-site/src/components/ListResources/IndividualResource.tsx @@ -5,7 +5,7 @@ import { MdHistory } from "react-icons/md"; import { SetModalResourceContext } from './Modal'; import { ResourceDisplayName, Resource, DisplayNamedResource } from './types'; import { IconType } from 'react-icons'; -import { InternalError } from './errors'; +import { InternalError } from '../ErrorBoundary'; export const LINK_COLOR = '#5097BA' export const LINK_HOVER_COLOR = '#31586c' diff --git a/static-site/src/components/ListResources/Modal.tsx b/static-site/src/components/ListResources/Modal.tsx index e94bca1fb..49db5664c 100644 --- a/static-site/src/components/ListResources/Modal.tsx +++ b/static-site/src/components/ListResources/Modal.tsx @@ -5,7 +5,7 @@ import * as d3 from "d3"; import { MdClose } from "react-icons/md"; import { dodge } from "./dodge"; import { Resource, VersionedResource } from './types'; -import { InternalError } from './errors'; +import { InternalError } from '../ErrorBoundary'; export const SetModalResourceContext = createContext> | null>(null); diff --git a/static-site/src/components/ListResources/ResourceGroup.tsx b/static-site/src/components/ListResources/ResourceGroup.tsx index 5a9345aba..01309086d 100644 --- a/static-site/src/components/ListResources/ResourceGroup.tsx +++ b/static-site/src/components/ListResources/ResourceGroup.tsx @@ -6,7 +6,7 @@ import { IndividualResource, getMaxResourceWidth, TooltipWrapper, IconContainer, ResourceLinkWrapper, ResourceLink, LINK_COLOR, LINK_HOVER_COLOR } from "./IndividualResource" import { SetModalResourceContext } from "./Modal"; import { DisplayNamedResource, Group, QuickLink, Resource } from './types'; -import { InternalError } from './errors'; +import { InternalError } from '../ErrorBoundary'; const ResourceGroupHeader = ({ group, diff --git a/static-site/src/components/ListResources/index.tsx b/static-site/src/components/ListResources/index.tsx index 853d2e7f7..e6ddf3212 100644 --- a/static-site/src/components/ListResources/index.tsx +++ b/static-site/src/components/ListResources/index.tsx @@ -14,7 +14,7 @@ import {ResourceModal, SetModalResourceContext} from "./Modal"; import { ExpandableTiles } from "../ExpandableTiles"; import { FilterTile, FilterOption, Group, QuickLink, Resource, ResourceListingInfo, SortMethod, convertVersionedResource } from './types'; import { HugeSpacer } from "../../layouts/generalComponents"; -import { ErrorBoundary } from './errors'; +import { ErrorBoundary } from '../ErrorBoundary'; const LIST_ANCHOR = "list"; diff --git a/static-site/src/components/ListResources/types.ts b/static-site/src/components/ListResources/types.ts index 57c8e5c64..02390fe97 100644 --- a/static-site/src/components/ListResources/types.ts +++ b/static-site/src/components/ListResources/types.ts @@ -1,4 +1,4 @@ -import { InternalError } from "./errors"; +import { InternalError } from "../ErrorBoundary"; import { Tile } from "../ExpandableTiles/types" export interface FilterOption { From a90255e2757449bdd0f99167932a422dd8b66be8 Mon Sep 17 00:00:00 2001 From: Victor Lin <13424970+victorlin@users.noreply.github.com> Date: Thu, 14 Nov 2024 17:16:32 -0800 Subject: [PATCH 03/14] Add error handling to partitionByPathogen With the current code, it is not possible for these values to be undefined. Make this clear with type narrowing. --- .../src/components/ListResources/useDataFetch.ts | 15 ++++++++++++--- 1 file changed, 12 insertions(+), 3 deletions(-) diff --git a/static-site/src/components/ListResources/useDataFetch.ts b/static-site/src/components/ListResources/useDataFetch.ts index 939aaf45f..24616b2b1 100644 --- a/static-site/src/components/ListResources/useDataFetch.ts +++ b/static-site/src/components/ListResources/useDataFetch.ts @@ -1,5 +1,6 @@ import { useState, useEffect } from 'react'; import { Group, Resource, ResourceListingInfo } from './types'; +import { InternalError } from '../ErrorBoundary'; /** @@ -65,8 +66,16 @@ function partitionByPathogen( const sortedDates = [...dates].sort(); const nameParts = name.split('/'); - // split() will always return at least 1 string - const groupName = nameParts[0]!; + + if (nameParts[0] === undefined) { + throw new InternalError(`Name is not properly formatted: '${name}'`); + } + + if (sortedDates[0] === undefined) { + throw new InternalError("Resource does not have any dates."); + } + + const groupName = nameParts[0]; const resourceDetails: Resource = { name, @@ -77,7 +86,7 @@ function partitionByPathogen( lastUpdated: sortedDates.at(-1), }; if (versioned) { - resourceDetails.firstUpdated = sortedDates[0]!; + resourceDetails.firstUpdated = sortedDates[0]; resourceDetails.dates = sortedDates; resourceDetails.nVersions = sortedDates.length; resourceDetails.updateCadence = updateCadence(sortedDates.map((date)=> new Date(date))); From d17a91c10e9bef54b422debda256c26d33f62620 Mon Sep 17 00:00:00 2001 From: Victor Lin <13424970+victorlin@users.noreply.github.com> Date: Thu, 14 Nov 2024 17:24:58 -0800 Subject: [PATCH 04/14] Remove non-null assertion on `store[groupName]` Instead of checking null-ness to initialize an empty array then using a non-null assertion which is loosely coupled to the preceding line, do both in one line. --- static-site/src/components/ListResources/useDataFetch.ts | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-) diff --git a/static-site/src/components/ListResources/useDataFetch.ts b/static-site/src/components/ListResources/useDataFetch.ts index 24616b2b1..c18db4399 100644 --- a/static-site/src/components/ListResources/useDataFetch.ts +++ b/static-site/src/components/ListResources/useDataFetch.ts @@ -92,8 +92,7 @@ function partitionByPathogen( resourceDetails.updateCadence = updateCadence(sortedDates.map((date)=> new Date(date))); } - if (!store[groupName]) store[groupName] = []; - store[groupName]!.push(resourceDetails) + (store[groupName] ??= []).push(resourceDetails) return store; }, {}); From 96f3018dc92c7ae8263b1cd26dca8c0203b6e28e Mon Sep 17 00:00:00 2001 From: Victor Lin <13424970+victorlin@users.noreply.github.com> Date: Thu, 14 Nov 2024 17:38:41 -0800 Subject: [PATCH 05/14] Remove non-null assertion for `colors[0]` With the current code, it is not possible for this to be undefined. Make this clear with type narrowing. --- static-site/src/components/Groups/Tiles/index.tsx | 14 +++++++++++++- 1 file changed, 13 insertions(+), 1 deletion(-) diff --git a/static-site/src/components/Groups/Tiles/index.tsx b/static-site/src/components/Groups/Tiles/index.tsx index 518c50be6..ee3fc13fe 100644 --- a/static-site/src/components/Groups/Tiles/index.tsx +++ b/static-site/src/components/Groups/Tiles/index.tsx @@ -7,9 +7,18 @@ import { UserContext } from "../../../layouts/userDataWrapper"; import { GroupTile } from "./types"; import { Group } from "../types"; import { ExpandableTiles } from "../../ExpandableTiles"; +import { ErrorBoundary, InternalError } from "../../ErrorBoundary"; export const GroupTiles = () => { + return ( + + + + ); +}; + +const GroupTilesUnhandled = () => { const { visibleGroups } = useContext(UserContext); return ( a.name.localeCompare(b.name)) .map((group) => { - const groupColor = colors[0]!; + if (colors[0] === undefined) { + throw new InternalError("Colors are missing."); + } + const groupColor = colors[0]; colors.push(colors.shift()!); const tile: GroupTile = { From 4f134d8d5030db3e1ce83cccab0417b04105eb5c Mon Sep 17 00:00:00 2001 From: Victor Lin <13424970+victorlin@users.noreply.github.com> Date: Thu, 14 Nov 2024 17:39:14 -0800 Subject: [PATCH 06/14] Keep non-null assertion on color rotation ESLint has no way of knowing that this is safe code. Add an exception for this line with a comment explaining what it does. --- static-site/src/components/Groups/Tiles/index.tsx | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) diff --git a/static-site/src/components/Groups/Tiles/index.tsx b/static-site/src/components/Groups/Tiles/index.tsx index ee3fc13fe..4071b64e8 100644 --- a/static-site/src/components/Groups/Tiles/index.tsx +++ b/static-site/src/components/Groups/Tiles/index.tsx @@ -38,7 +38,9 @@ function createGroupTiles(groups: Group[], colors = [...theme.titleColors]): Gro throw new InternalError("Colors are missing."); } const groupColor = colors[0]; - colors.push(colors.shift()!); + + // Rotate the colors + colors.push(colors.shift()!); // eslint-disable-line @typescript-eslint/no-non-null-assertion const tile: GroupTile = { img: "empty.png", From b5c2d8ff89581508bf3e072c69a2737b63a11ea3 Mon Sep 17 00:00:00 2001 From: Victor Lin <13424970+victorlin@users.noreply.github.com> Date: Thu, 14 Nov 2024 17:42:18 -0800 Subject: [PATCH 07/14] Remove unnecessary constructor The parent constructor is automatically used by default. --- static-site/src/components/ErrorBoundary.tsx | 6 +----- 1 file changed, 1 insertion(+), 5 deletions(-) diff --git a/static-site/src/components/ErrorBoundary.tsx b/static-site/src/components/ErrorBoundary.tsx index 7db7c692f..f1cadbb6e 100644 --- a/static-site/src/components/ErrorBoundary.tsx +++ b/static-site/src/components/ErrorBoundary.tsx @@ -1,11 +1,7 @@ import React, { ErrorInfo, ReactNode } from "react"; import { ErrorContainer } from "../pages/404"; -export class InternalError extends Error { - constructor(message: string) { - super(message); - } -} +export class InternalError extends Error {} interface Props { children: ReactNode; From d264a2ed4063c8abec7c8cdb0ce959cf60fb61ac Mon Sep 17 00:00:00 2001 From: Victor Lin <13424970+victorlin@users.noreply.github.com> Date: Thu, 14 Nov 2024 17:47:16 -0800 Subject: [PATCH 08/14] typescript-eslint: Prohibit type assertions This should not be used unless there is no other way for the TypeScript compiler to infer the type properly. In such cases, a comment with per-line exception should be used. I will address existing violations in subsequent commits. --- static-site/.eslintrc.yaml | 1 + 1 file changed, 1 insertion(+) diff --git a/static-site/.eslintrc.yaml b/static-site/.eslintrc.yaml index 967527ca6..fb78f6089 100644 --- a/static-site/.eslintrc.yaml +++ b/static-site/.eslintrc.yaml @@ -27,6 +27,7 @@ ignorePatterns: - public/ rules: + "@typescript-eslint/consistent-type-assertions": ["error", { assertionStyle: 'never' }] react/prop-types: off # Remove this override once all props have been typed using PropTypes or TypeScript. '@next/next/no-img-element': off # Remove this if we use next.js optimisations for '@next/next/no-html-link-for-pages': off From a80079dba74d4f35e69a50000a97d7c8c1ce0466 Mon Sep 17 00:00:00 2001 From: Victor Lin <13424970+victorlin@users.noreply.github.com> Date: Thu, 14 Nov 2024 17:57:02 -0800 Subject: [PATCH 09/14] Add reasoning for HTMLDivElement type assertion --- .../components/ListResources/IndividualResource.tsx | 10 +++++++++- 1 file changed, 9 insertions(+), 1 deletion(-) diff --git a/static-site/src/components/ListResources/IndividualResource.tsx b/static-site/src/components/ListResources/IndividualResource.tsx index ad55d315b..eaa73fd1d 100644 --- a/static-site/src/components/ListResources/IndividualResource.tsx +++ b/static-site/src/components/ListResources/IndividualResource.tsx @@ -139,9 +139,17 @@ export const IndividualResource = ({ throw new InternalError("ref must be defined and the parent must be a div (IndividualResourceContainer)."); } + // The type of ref.current.parentNode is ParentNode which does not have an + // offsetTop property. I don't think there is a way to appease the + // TypeScript compiler other than a type assertion. It is loosely coupled + // to the check above for parentNode.nodeName. + // Note: this doesn't strictly have to be a div, but that's what it is in + // current usage of the component at the time of writing. + const parentNode = ref.current.parentNode as HTMLDivElement // eslint-disable-line @typescript-eslint/consistent-type-assertions + /* The column CSS is great but doesn't allow us to know if an element is at the top of its column, so we resort to JS */ - if (ref.current.offsetTop===(ref.current.parentNode as HTMLDivElement).offsetTop) { + if (ref.current.offsetTop===parentNode.offsetTop) { setTopOfColumn(true); } }, []); From 95683617bdf44d9d81276f7d861cee59acff38f3 Mon Sep 17 00:00:00 2001 From: Victor Lin <13424970+victorlin@users.noreply.github.com> Date: Thu, 14 Nov 2024 18:10:23 -0800 Subject: [PATCH 10/14] Remove type assertion on sort method This properly handles any new changes to input values defined by the form. --- static-site/src/components/ListResources/index.tsx | 9 +++++++-- 1 file changed, 7 insertions(+), 2 deletions(-) diff --git a/static-site/src/components/ListResources/index.tsx b/static-site/src/components/ListResources/index.tsx index e6ddf3212..d69ba71ff 100644 --- a/static-site/src/components/ListResources/index.tsx +++ b/static-site/src/components/ListResources/index.tsx @@ -14,7 +14,7 @@ import {ResourceModal, SetModalResourceContext} from "./Modal"; import { ExpandableTiles } from "../ExpandableTiles"; import { FilterTile, FilterOption, Group, QuickLink, Resource, ResourceListingInfo, SortMethod, convertVersionedResource } from './types'; import { HugeSpacer } from "../../layouts/generalComponents"; -import { ErrorBoundary } from '../ErrorBoundary'; +import { ErrorBoundary, InternalError } from '../ErrorBoundary'; const LIST_ANCHOR = "list"; @@ -182,7 +182,12 @@ function SortOptions({sortMethod, changeSortMethod}: { changeSortMethod: React.Dispatch>, }) { function onChangeValue(event: FormEvent): void { - changeSortMethod(event.currentTarget.value as SortMethod); + const sortMethod = event.currentTarget.value; + if (sortMethod !== "alphabetical" && + sortMethod !== "lastUpdated") { + throw new InternalError(`Unhandled sort method: '${sortMethod}'`); + } + changeSortMethod(sortMethod); } return ( From 72a73762ed81fda722f4a8e7f2b9023e582836fb Mon Sep 17 00:00:00 2001 From: Victor Lin <13424970+victorlin@users.noreply.github.com> Date: Thu, 14 Nov 2024 18:26:15 -0800 Subject: [PATCH 11/14] Add type assertion exception for YAML file import I remember this was tricky and couldn't figure out a way to map featured-analyses.yaml to SplashTile[] without this type assertion. --- static-site/src/components/splash/index.tsx | 1 + 1 file changed, 1 insertion(+) diff --git a/static-site/src/components/splash/index.tsx b/static-site/src/components/splash/index.tsx index bc6ee8b05..ac1bdf92d 100644 --- a/static-site/src/components/splash/index.tsx +++ b/static-site/src/components/splash/index.tsx @@ -116,6 +116,7 @@ const Splash = () => { + {/* eslint-disable-next-line @typescript-eslint/consistent-type-assertions */} From 22456f264596c96351aa6b702875d18073afe6d6 Mon Sep 17 00:00:00 2001 From: Victor Lin <13424970+victorlin@users.noreply.github.com> Date: Thu, 14 Nov 2024 18:31:31 -0800 Subject: [PATCH 12/14] Remove unnecessary type assertion The type is already set on this variable. --- static-site/src/sections/group-members-page.tsx | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/static-site/src/sections/group-members-page.tsx b/static-site/src/sections/group-members-page.tsx index 00776938c..dd180b2ad 100644 --- a/static-site/src/sections/group-members-page.tsx +++ b/static-site/src/sections/group-members-page.tsx @@ -77,7 +77,7 @@ const GroupMembersPage = ({ groupName }: {groupName: string}) => { {roles && members - ? + ? : Fetching group members...} ) From e62d01c2b918108851e23ee1d5849501a4bec8e9 Mon Sep 17 00:00:00 2001 From: Victor Lin <13424970+victorlin@users.noreply.github.com> Date: Thu, 14 Nov 2024 18:39:32 -0800 Subject: [PATCH 13/14] group-members-page: Handle unknown errors I'm not sure if it's possible for a non-Error error to be thrown in these parts, but TypeScript thinks it is possible and it's easy to handle. --- static-site/src/sections/group-members-page.tsx | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/static-site/src/sections/group-members-page.tsx b/static-site/src/sections/group-members-page.tsx index dd180b2ad..3b8f6d48a 100644 --- a/static-site/src/sections/group-members-page.tsx +++ b/static-site/src/sections/group-members-page.tsx @@ -40,7 +40,7 @@ const GroupMembersPage = ({ groupName }: {groupName: string}) => { roles = await rolesResponse.json(); members = await membersResponse.json(); } catch (err) { - const errorMessage = (err as Error).message + const errorMessage = err instanceof Error ? err.message : String(err) if(!ignore) { setErrorMessage({ title: "An error occurred when trying to fetch group membership data", @@ -150,7 +150,7 @@ export async function canViewGroupMembers(groupName: string) { const allowedMethods = new Set(groupMemberOptions.headers.get("Allow")?.split(/\s*,\s*/)); return allowedMethods.has("GET"); } catch (err) { - const errorMessage = (err as Error).message + const errorMessage = err instanceof Error ? err.message : String(err) console.error("Cannot check user permissions to view group members", errorMessage); } return false From 4982fa8833ee524ed2c510b9b5768e51c2c94246 Mon Sep 17 00:00:00 2001 From: Victor Lin <13424970+victorlin@users.noreply.github.com> Date: Tue, 19 Nov 2024 17:08:59 -0800 Subject: [PATCH 14/14] Add PopulatedMetadata type to avoid non-null assertions This better reflects actual usage. --- static-site/app/blog/[id]/page.tsx | 20 ++++++++++++++++---- 1 file changed, 16 insertions(+), 4 deletions(-) diff --git a/static-site/app/blog/[id]/page.tsx b/static-site/app/blog/[id]/page.tsx index bb8e77262..343c5e408 100644 --- a/static-site/app/blog/[id]/page.tsx +++ b/static-site/app/blog/[id]/page.tsx @@ -27,6 +27,18 @@ export function generateStaticParams(): BlogPostParams[] { }); } +type PopulatedMetadata = Metadata & { + metadataBase: URL + openGraph: { + description: string + images: { url: string}[] + siteName: string + title: string + type: "website" + url: URL | string + } +} + // generate opengraph and other metadata tags export async function generateMetadata({ params, @@ -37,7 +49,7 @@ export async function generateMetadata({ // set up some defaults that are independent of the specific blog post const baseUrl = new URL(siteUrl); - const metadata: Metadata = { + const metadata: PopulatedMetadata = { metadataBase: baseUrl, openGraph: { description: siteTitleAlt, @@ -61,9 +73,9 @@ export async function generateMetadata({ metadata.title = blogPost.title; metadata.description = description; - metadata.openGraph!.description = description; - metadata.openGraph!.title = `${siteTitle}: ${blogPost.title}`; - metadata.openGraph!.url = `/blog/${blogPost.blogUrlName}`; + metadata.openGraph.description = description; + metadata.openGraph.title = `${siteTitle}: ${blogPost.title}`; + metadata.openGraph.url = `/blog/${blogPost.blogUrlName}`; } return metadata;