From 84470e4c608579362187018192222b905d120c40 Mon Sep 17 00:00:00 2001 From: Victor Lin <13424970+victorlin@users.noreply.github.com> Date: Tue, 12 Nov 2024 17:05:35 -0800 Subject: [PATCH 01/10] Handle internal errors Add a custom error class for errors that aren't user error and can be considered a bug in the code. For now I've scoped this to ListResources, but a similar approach can be adopted for other components too. --- .../ListResources/IndividualResource.tsx | 3 +- .../src/components/ListResources/Modal.tsx | 3 +- .../ListResources/ResourceGroup.tsx | 3 +- .../src/components/ListResources/errors.tsx | 54 +++++++++++++++++++ .../src/components/ListResources/index.tsx | 9 ++-- 5 files changed, 66 insertions(+), 6 deletions(-) create mode 100644 static-site/src/components/ListResources/errors.tsx diff --git a/static-site/src/components/ListResources/IndividualResource.tsx b/static-site/src/components/ListResources/IndividualResource.tsx index 2a1136dd3..41d745eb9 100644 --- a/static-site/src/components/ListResources/IndividualResource.tsx +++ b/static-site/src/components/ListResources/IndividualResource.tsx @@ -5,6 +5,7 @@ import { MdHistory } from "react-icons/md"; import { SetModalResourceContext } from './Modal'; import { ResourceDisplayName, Resource } from './types'; import { IconType } from 'react-icons'; +import { InternalError } from './errors'; export const LINK_COLOR = '#5097BA' export const LINK_HOVER_COLOR = '#31586c' @@ -129,7 +130,7 @@ export const IndividualResource = ({ isMobile: boolean }) => { const setModalResource = useContext(SetModalResourceContext); - if (!setModalResource) throw new Error("Context not provided!") + if (!setModalResource) throw new InternalError("Context not provided!") const ref = useRef(null); const [topOfColumn, setTopOfColumn] = useState(false); diff --git a/static-site/src/components/ListResources/Modal.tsx b/static-site/src/components/ListResources/Modal.tsx index 585b84515..0975f6246 100644 --- a/static-site/src/components/ListResources/Modal.tsx +++ b/static-site/src/components/ListResources/Modal.tsx @@ -5,6 +5,7 @@ import * as d3 from "d3"; import { MdClose } from "react-icons/md"; import { dodge } from "./dodge"; import { Resource } from './types'; +import { InternalError } from './errors'; export const SetModalResourceContext = createContext> | null>(null); @@ -134,7 +135,7 @@ const Title = styled.div` function _snapshotSummary(dates: string[]) { const d = [...dates].sort() - if (d.length < 1) throw new Error("Missing dates.") + if (d.length < 1) throw new InternalError("Missing dates.") const d1 = new Date(d.at( 0)!).getTime(); const d2 = new Date(d.at(-1)!).getTime(); diff --git a/static-site/src/components/ListResources/ResourceGroup.tsx b/static-site/src/components/ListResources/ResourceGroup.tsx index f44438e43..2deeab7a3 100644 --- a/static-site/src/components/ListResources/ResourceGroup.tsx +++ b/static-site/src/components/ListResources/ResourceGroup.tsx @@ -6,6 +6,7 @@ import { IndividualResource, getMaxResourceWidth, TooltipWrapper, IconContainer, ResourceLinkWrapper, ResourceLink, LINK_COLOR, LINK_HOVER_COLOR } from "./IndividualResource" import { SetModalResourceContext } from "./Modal"; import { Group, QuickLink, Resource } from './types'; +import { InternalError } from './errors'; const ResourceGroupHeader = ({ group, @@ -25,7 +26,7 @@ const ResourceGroupHeader = ({ quickLinks: QuickLink[] }) => { const setModalResource = useContext(SetModalResourceContext); - if (!setModalResource) throw new Error("Context not provided!") + if (!setModalResource) throw new InternalError("Context not provided!") /* Filter the known quick links to those which appear in resources of this group */ const resourcesByName = Object.fromEntries(group.resources.map((r) => [r.name, r])); diff --git a/static-site/src/components/ListResources/errors.tsx b/static-site/src/components/ListResources/errors.tsx new file mode 100644 index 000000000..c4bd9eeff --- /dev/null +++ b/static-site/src/components/ListResources/errors.tsx @@ -0,0 +1,54 @@ +import React, { ErrorInfo, ReactNode } from "react"; +import { ErrorContainer } from "../../pages/404"; + +export class InternalError extends Error { + constructor(message: string) { + super(message); + } +} + +interface Props { + children: ReactNode; +} + +interface State { + hasError: boolean; + errorMessage: string; +} + +export class ErrorBoundary extends React.Component { + constructor(props: Props) { + super(props); + this.state = { + hasError: false, + errorMessage:"", + }; + } + + static getDerivedStateFromError(error: Error): State { + return { + hasError: true, + errorMessage: error instanceof InternalError ? error.message : "Unknown error (thrown value was not an InternalError)", + }; + } + + override componentDidCatch(error: Error, info: ErrorInfo) { + console.error(error); + console.error(info); + } + + override render() { + if (this.state.hasError) { + return ( + + {"Something isn't working!"} +
+ {`Error: ${this.state.errorMessage}`} +
+ {"Please "}get in touch{" if this keeps happening"} +
+ ) + } + return this.props.children; + } +} \ No newline at end of file diff --git a/static-site/src/components/ListResources/index.tsx b/static-site/src/components/ListResources/index.tsx index da3e0f86a..08aa7bf52 100644 --- a/static-site/src/components/ListResources/index.tsx +++ b/static-site/src/components/ListResources/index.tsx @@ -14,6 +14,7 @@ import {ResourceModal, SetModalResourceContext} from "./Modal"; import { ExpandableTiles } from "../ExpandableTiles"; import { FilterTile, FilterOption, Group, QuickLink, Resource, ResourceListingInfo, SortMethod } from './types'; import { HugeSpacer } from "../../layouts/generalComponents"; +import { ErrorBoundary } from './errors'; const LIST_ANCHOR = "list"; @@ -165,9 +166,11 @@ function ListResourcesResponsive(props: ListResourcesResponsiveProps) { }; }, []); return ( -
- -
+ +
+ +
+
) } From c175886dc0c3fb5eb946545568ca1569cb2e1b4d Mon Sep 17 00:00:00 2001 From: Victor Lin <13424970+victorlin@users.noreply.github.com> Date: Thu, 7 Nov 2024 16:14:01 -0800 Subject: [PATCH 02/10] Condition on versioned value Instead of indirectly inferring this from presence of a lastUpdated value for the first group. --- static-site/src/components/ListResources/index.tsx | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/static-site/src/components/ListResources/index.tsx b/static-site/src/components/ListResources/index.tsx index 08aa7bf52..84e77e834 100644 --- a/static-site/src/components/ListResources/index.tsx +++ b/static-site/src/components/ListResources/index.tsx @@ -94,7 +94,7 @@ function ListResources({ - { groups?.[0]?.lastUpdated && ( + { versioned && ( ) || ( From 9fab60dac4966d26ca23892ae71fb0517ae5e118 Mon Sep 17 00:00:00 2001 From: Victor Lin <13424970+victorlin@users.noreply.github.com> Date: Wed, 13 Nov 2024 15:06:27 -0800 Subject: [PATCH 03/10] Update Group prop types This better reflects the actual usage. --- static-site/src/components/ListResources/types.ts | 4 ++-- static-site/src/components/ListResources/useDataFetch.ts | 2 +- 2 files changed, 3 insertions(+), 3 deletions(-) diff --git a/static-site/src/components/ListResources/types.ts b/static-site/src/components/ListResources/types.ts index aae85efd2..c1f614b1f 100644 --- a/static-site/src/components/ListResources/types.ts +++ b/static-site/src/components/ListResources/types.ts @@ -10,8 +10,8 @@ export type SortMethod = "lastUpdated" | "alphabetical"; export interface Group { groupName: string nResources: number - nVersions?: number - lastUpdated: string // date + nVersions: number | undefined + lastUpdated: string | undefined resources: Resource[] groupUrl?: string groupDisplayName?: string diff --git a/static-site/src/components/ListResources/useDataFetch.ts b/static-site/src/components/ListResources/useDataFetch.ts index 270f7c81e..939aaf45f 100644 --- a/static-site/src/components/ListResources/useDataFetch.ts +++ b/static-site/src/components/ListResources/useDataFetch.ts @@ -106,7 +106,7 @@ function groupsFrom( groupName: groupName, nResources: resources.length, nVersions: resources.reduce((total, r) => r.nVersions ? total+r.nVersions : total, 0) || undefined, - lastUpdated: resources.map((r) => r.lastUpdated).sort().at(-1)!, + lastUpdated: resources.map((r) => r.lastUpdated).sort().at(-1), resources, } /* add optional properties */ From c1096d4c6fddf80a59fcfef2189769f88bfb9b61 Mon Sep 17 00:00:00 2001 From: Victor Lin <13424970+victorlin@users.noreply.github.com> Date: Wed, 13 Nov 2024 15:07:33 -0800 Subject: [PATCH 04/10] Add type for versioned group This reduces the amount of optional properties and need for scattered type narrowing downstream. Some restructuring of the code was necessary for proper type narrowing. This includes changing the switch/case to if/else which allows redefining variables with the same name in each block. --- .../src/components/ListResources/types.ts | 17 +++++ .../ListResources/useSortAndFilter.ts | 71 ++++++++++--------- 2 files changed, 55 insertions(+), 33 deletions(-) diff --git a/static-site/src/components/ListResources/types.ts b/static-site/src/components/ListResources/types.ts index c1f614b1f..9d2fb7430 100644 --- a/static-site/src/components/ListResources/types.ts +++ b/static-site/src/components/ListResources/types.ts @@ -17,6 +17,23 @@ export interface Group { groupDisplayName?: string } +export interface VersionedGroup extends Group { + nVersions: number + lastUpdated: string +} + +export function convertVersionedGroup(group: Group): VersionedGroup { + if (group.nVersions !== undefined && + group.lastUpdated !== undefined) { + return { + ...group, + nVersions: group.nVersions, + lastUpdated: group.lastUpdated, + } + } + throw new InternalError("Group is not versioned."); +} + export interface Resource { name: string displayName?: ResourceDisplayName diff --git a/static-site/src/components/ListResources/useSortAndFilter.ts b/static-site/src/components/ListResources/useSortAndFilter.ts index 81d47df35..53343c6f4 100644 --- a/static-site/src/components/ListResources/useSortAndFilter.ts +++ b/static-site/src/components/ListResources/useSortAndFilter.ts @@ -1,5 +1,5 @@ import { useMemo } from 'react'; -import { FilterOption, Group, Resource, SortMethod } from './types'; +import { convertVersionedGroup, FilterOption, Group, Resource, SortMethod, VersionedGroup } from './types'; export const useSortAndFilter = ( @@ -13,28 +13,6 @@ export const useSortAndFilter = ( /* Following console log is really useful for development */ // console.log(`useSortAndFilter() sortMethod "${sortMethod}" ` + (selectedFilterOptions.length ? `filtering to ${selectedFilterOptions.map((el) => el.value).join(", ")}` : '(no filtering)')) - let _sortGroups: (groupA: Group, groupB: Group) => 1 | -1 | 0, - _sortResources: (a: Resource, b: Resource) => 1 | -1 | 0; - switch (sortMethod) { - case "lastUpdated": - _sortGroups = (groupA: Group, groupB: Group) => _newestFirstSort(groupA.lastUpdated, groupB.lastUpdated); - _sortResources = (a: Resource, b: Resource) => { - if (!a.lastUpdated || !b.lastUpdated || a.lastUpdated === b.lastUpdated) { - // resources updated on the same day or without a last updated date - // sort alphabetically - return _lexicographicSort(a.name, b.name) - } - else { - return _newestFirstSort(a.lastUpdated, b.lastUpdated); - } - } - break; - case "alphabetical": - _sortGroups = (groupA: Group, groupB: Group) => _lexicographicSort(groupA.groupName.toLowerCase(), groupB.groupName.toLowerCase()), - _sortResources = (a: Resource, b: Resource) => _lexicographicSort(a.name, b.name) - break; - } - const searchValues = selectedFilterOptions.map((o) => o.value); function _filterResources(resource: Resource) { if (searchValues.length===0) return true; @@ -43,17 +21,44 @@ export const useSortAndFilter = ( .every((x) => x); } - const resourceGroups = originalData - .map((group) => ({ - ...group, - resources: group.resources - .filter(_filterResources) - .sort(_sortResources) - })) - .filter((group) => !!group.resources.length) - .sort(_sortGroups); + function sortAndFilter( + groups: T[], + sortGroups: (a: T, b: T) => number, + sortResources: (a: Resource, b: Resource) => number, + ): T[] { + return groups + .map((group) => ({ + ...group, + resources: group.resources + .filter(_filterResources) + .sort(sortResources) + })) + .filter((group) => !!group.resources.length) + .sort(sortGroups); + } - setState(resourceGroups); + if (sortMethod === "lastUpdated") { + const groups = originalData.map((group: Group) => convertVersionedGroup(group)) + const _sortGroups = (groupA: VersionedGroup, groupB: VersionedGroup) => _newestFirstSort(groupA.lastUpdated, groupB.lastUpdated) + const _sortResources = (a: Resource, b: Resource) => { + if (!a.lastUpdated || !b.lastUpdated || a.lastUpdated === b.lastUpdated) { + // resources updated on the same day or without a last updated date + // sort alphabetically + return _lexicographicSort(a.name, b.name) + } + else { + return _newestFirstSort(a.lastUpdated, b.lastUpdated); + } + } + const resourceGroups = sortAndFilter(groups, _sortGroups, _sortResources) + setState(resourceGroups) + } else if (sortMethod === "alphabetical") { + const groups = originalData; + const _sortGroups = (groupA: Group, groupB: Group) => _lexicographicSort(groupA.groupName.toLowerCase(), groupB.groupName.toLowerCase()) + const _sortResources = (a: Resource, b: Resource) => _lexicographicSort(a.name, b.name) + const resourceGroups = sortAndFilter(groups, _sortGroups, _sortResources) + setState(resourceGroups) + } }, [sortMethod, selectedFilterOptions, originalData, setState]) } From 950c70f644a3e56ba8e166f7c801be2d1db4555f Mon Sep 17 00:00:00 2001 From: Victor Lin <13424970+victorlin@users.noreply.github.com> Date: Tue, 12 Nov 2024 17:08:55 -0800 Subject: [PATCH 05/10] Add type for versioned resource This reduces the amount of optional properties and need for scattered type narrowing downstream. --- .../src/components/ListResources/Modal.tsx | 9 +++---- .../src/components/ListResources/index.tsx | 6 ++--- .../src/components/ListResources/types.ts | 27 +++++++++++++++++++ 3 files changed, 33 insertions(+), 9 deletions(-) diff --git a/static-site/src/components/ListResources/Modal.tsx b/static-site/src/components/ListResources/Modal.tsx index 0975f6246..97f91b0e1 100644 --- a/static-site/src/components/ListResources/Modal.tsx +++ b/static-site/src/components/ListResources/Modal.tsx @@ -4,7 +4,7 @@ import styled from 'styled-components'; import * as d3 from "d3"; import { MdClose } from "react-icons/md"; import { dodge } from "./dodge"; -import { Resource } from './types'; +import { Resource, VersionedResource } from './types'; import { InternalError } from './errors'; export const SetModalResourceContext = createContext> | null>(null); @@ -17,7 +17,7 @@ export const ResourceModal = ({ resource, dismissModal, }: { - resource?: Resource + resource: VersionedResource dismissModal: () => void }) => { const [ref, setRef] = useState(null); @@ -43,9 +43,6 @@ export const ResourceModal = ({ _draw(ref, resource) }, [ref, resource]) - // modal is only applicable for versioned resources - if (!resource || !resource.dates || !resource.updateCadence) return null; - const summary = _snapshotSummary(resource.dates); return (
@@ -147,7 +144,7 @@ function _snapshotSummary(dates: string[]) { return {duration, first: d[0], last:d.at(-1)}; } -function _draw(ref, resource: Resource) { +function _draw(ref, resource: VersionedResource) { // do nothing if resource has no dates if (!resource.dates) return diff --git a/static-site/src/components/ListResources/index.tsx b/static-site/src/components/ListResources/index.tsx index 84e77e834..4bc9a714f 100644 --- a/static-site/src/components/ListResources/index.tsx +++ b/static-site/src/components/ListResources/index.tsx @@ -12,7 +12,7 @@ import { ErrorContainer } from "../../pages/404"; import { TooltipWrapper } from "./IndividualResource"; import {ResourceModal, SetModalResourceContext} from "./Modal"; import { ExpandableTiles } from "../ExpandableTiles"; -import { FilterTile, FilterOption, Group, QuickLink, Resource, ResourceListingInfo, SortMethod } from './types'; +import { FilterTile, FilterOption, Group, QuickLink, Resource, ResourceListingInfo, SortMethod, convertVersionedResource } from './types'; import { HugeSpacer } from "../../layouts/generalComponents"; import { ErrorBoundary } from './errors'; @@ -117,8 +117,8 @@ function ListResources({ - { versioned && ( - setModalResource(undefined)}/> + { versioned && modalResource && ( + setModalResource(undefined)}/> )} diff --git a/static-site/src/components/ListResources/types.ts b/static-site/src/components/ListResources/types.ts index 9d2fb7430..564e63210 100644 --- a/static-site/src/components/ListResources/types.ts +++ b/static-site/src/components/ListResources/types.ts @@ -1,3 +1,4 @@ +import { InternalError } from "./errors"; import { Tile } from "../ExpandableTiles/types" export interface FilterOption { @@ -48,6 +49,32 @@ export interface Resource { updateCadence?: UpdateCadence } +export interface VersionedResource extends Resource { + lastUpdated: string // date + firstUpdated: string // date + dates: string[] + nVersions: number + updateCadence: UpdateCadence +} + +export function convertVersionedResource(resource: Resource): VersionedResource { + if (resource.lastUpdated !== undefined && + resource.firstUpdated !== undefined && + resource.dates !== undefined && + resource.nVersions !== undefined && + resource.updateCadence !== undefined) { + return { + ...resource, + lastUpdated: resource.lastUpdated, + firstUpdated: resource.firstUpdated, + dates: resource.dates, + nVersions: resource.nVersions, + updateCadence: resource.updateCadence + } + } + throw new InternalError("Resource is not versioned."); +} + export interface ResourceDisplayName { hovered: string default: string From 52c2919f7dcd3a69a25efb53a4f809f82ec8b865 Mon Sep 17 00:00:00 2001 From: Victor Lin <13424970+victorlin@users.noreply.github.com> Date: Wed, 13 Nov 2024 15:34:20 -0800 Subject: [PATCH 06/10] Replace displayName null check with conditional type With the current code, it is not possible for displayName to be unset in getMaxResourceWidth. Make this clear with type narrowing. --- .../ListResources/IndividualResource.tsx | 6 ++---- .../components/ListResources/ResourceGroup.tsx | 16 ++++++++++------ .../src/components/ListResources/types.ts | 4 ++++ 3 files changed, 16 insertions(+), 10 deletions(-) diff --git a/static-site/src/components/ListResources/IndividualResource.tsx b/static-site/src/components/ListResources/IndividualResource.tsx index 41d745eb9..177d3272a 100644 --- a/static-site/src/components/ListResources/IndividualResource.tsx +++ b/static-site/src/components/ListResources/IndividualResource.tsx @@ -3,7 +3,7 @@ import React, {useState, useRef, useEffect, useContext} from 'react'; import styled from 'styled-components'; import { MdHistory } from "react-icons/md"; import { SetModalResourceContext } from './Modal'; -import { ResourceDisplayName, Resource } from './types'; +import { ResourceDisplayName, Resource, DisplayNamedResource } from './types'; import { IconType } from 'react-icons'; import { InternalError } from './errors'; @@ -20,10 +20,8 @@ export const LINK_HOVER_COLOR = '#31586c' const [resourceFontSize, namePxPerChar, summaryPxPerChar] = [16, 10, 9]; const iconWidth = 20; // not including text const gapSize = 10; -export const getMaxResourceWidth = (displayResources: Resource[]) => { +export const getMaxResourceWidth = (displayResources: DisplayNamedResource[]) => { return displayResources.reduce((w, r) => { - if (!r.displayName) return w - /* add the pixels for the display name */ let _w = r.displayName.default.length * namePxPerChar; if (r.nVersions && r.updateCadence) { diff --git a/static-site/src/components/ListResources/ResourceGroup.tsx b/static-site/src/components/ListResources/ResourceGroup.tsx index 2deeab7a3..079e86536 100644 --- a/static-site/src/components/ListResources/ResourceGroup.tsx +++ b/static-site/src/components/ListResources/ResourceGroup.tsx @@ -5,7 +5,7 @@ import { MdHistory, MdFormatListBulleted, MdChevronRight } from "react-icons/md" import { IndividualResource, getMaxResourceWidth, TooltipWrapper, IconContainer, ResourceLinkWrapper, ResourceLink, LINK_COLOR, LINK_HOVER_COLOR } from "./IndividualResource" import { SetModalResourceContext } from "./Modal"; -import { Group, QuickLink, Resource } from './types'; +import { DisplayNamedResource, Group, QuickLink, Resource } from './types'; import { InternalError } from './errors'; const ResourceGroupHeader = ({ @@ -138,8 +138,8 @@ export const ResourceGroup = ({ const {collapseThreshold, resourcesToShowWhenCollapsed} = collapseThresolds(numGroups); const collapsible = group.resources.length > collapseThreshold; const [isCollapsed, setCollapsed] = useState(collapsible); // if it is collapsible, start collapsed - const displayResources = isCollapsed ? group.resources.slice(0, resourcesToShowWhenCollapsed) : group.resources; - _setDisplayName(displayResources) + const resources = isCollapsed ? group.resources.slice(0, resourcesToShowWhenCollapsed) : group.resources; + const displayResources = _setDisplayName(resources) /* isMobile: boolean determines whether we expose snapshots, as we hide them on small screens */ const isMobile = elWidth < 500; @@ -258,9 +258,9 @@ function NextstrainLogo() { * "seasonal-flu | h1n1pdm" * " | h3n2" */ -function _setDisplayName(resources: Resource[]) { +function _setDisplayName(resources: Resource[]): DisplayNamedResource[] { const sep = "│"; // ASCII 179 - resources.forEach((r, i) => { + return resources.map((r, i) => { let name; if (i===0) { name = r.nameParts.join(sep); @@ -271,7 +271,11 @@ function _setDisplayName(resources: Resource[]) { } name = r.nameParts.map((word, j) => j < matchIdx ? ' '.repeat(word.length) : word).join(sep); } - r.displayName = {hovered: r.nameParts.join(sep), default: name} + + return { + ...r, + displayName: {hovered: r.nameParts.join(sep), default: name} + } }) } diff --git a/static-site/src/components/ListResources/types.ts b/static-site/src/components/ListResources/types.ts index 564e63210..57c8e5c64 100644 --- a/static-site/src/components/ListResources/types.ts +++ b/static-site/src/components/ListResources/types.ts @@ -49,6 +49,10 @@ export interface Resource { updateCadence?: UpdateCadence } +export interface DisplayNamedResource extends Resource { + displayName: ResourceDisplayName +} + export interface VersionedResource extends Resource { lastUpdated: string // date firstUpdated: string // date From cd0212742b9c6284f8c9705e1c79078d843e9fe3 Mon Sep 17 00:00:00 2001 From: Victor Lin <13424970+victorlin@users.noreply.github.com> Date: Thu, 7 Nov 2024 15:55:01 -0800 Subject: [PATCH 07/10] Use narrowing instead of non-null assertion Same behavior but using better practices. --- static-site/src/components/ListResources/Modal.tsx | 13 +++++++------ 1 file changed, 7 insertions(+), 6 deletions(-) diff --git a/static-site/src/components/ListResources/Modal.tsx b/static-site/src/components/ListResources/Modal.tsx index 97f91b0e1..86e598d30 100644 --- a/static-site/src/components/ListResources/Modal.tsx +++ b/static-site/src/components/ListResources/Modal.tsx @@ -132,16 +132,17 @@ const Title = styled.div` function _snapshotSummary(dates: string[]) { const d = [...dates].sort() - if (d.length < 1) throw new InternalError("Missing dates.") - - const d1 = new Date(d.at( 0)!).getTime(); - const d2 = new Date(d.at(-1)!).getTime(); - const days = (d2 - d1)/1000/60/60/24; + const d1 = d[0]; + const d2 = d.at(-1); + if (d1 === undefined || d2 === undefined) { + throw new InternalError("Missing dates."); + } + const days = (new Date(d2).getTime() - new Date(d1).getTime())/1000/60/60/24; let duration = ''; if (days < 100) duration=`${days} days`; else if (days < 365*2) duration=`${Math.round(days/(365/12))} months`; else duration=`${Math.round(days/365)} years`; - return {duration, first: d[0], last:d.at(-1)}; + return {duration, first: d1, last: d2}; } function _draw(ref, resource: VersionedResource) { From 7aaeb484c4850db7760308df52ccb42b1bb99482 Mon Sep 17 00:00:00 2001 From: Victor Lin <13424970+victorlin@users.noreply.github.com> Date: Tue, 12 Nov 2024 15:28:10 -0800 Subject: [PATCH 08/10] Properly check that flatData[0] is defined The previous check was loosely coupled, relying on a non-null assertion. This should be more clear to both the compiler and humans. --- static-site/src/components/ListResources/Modal.tsx | 10 +++++----- 1 file changed, 5 insertions(+), 5 deletions(-) diff --git a/static-site/src/components/ListResources/Modal.tsx b/static-site/src/components/ListResources/Modal.tsx index 86e598d30..e94bca1fb 100644 --- a/static-site/src/components/ListResources/Modal.tsx +++ b/static-site/src/components/ListResources/Modal.tsx @@ -146,15 +146,16 @@ function _snapshotSummary(dates: string[]) { } function _draw(ref, resource: VersionedResource) { - // do nothing if resource has no dates - if (!resource.dates) return - /* Note that _page_ resizes by themselves will not result in this function rerunning, which isn't great, but for a modal I think it's perfectly acceptable */ const sortedDateStrings = [...resource.dates].sort(); const flatData = sortedDateStrings.map((version) => ({version, 'date': new Date(version)})); + if (flatData[0] === undefined) { + throw new InternalError("Resource does not have any dates."); + } + const width = ref.clientWidth; const graphIndent = 20; const heights = { @@ -178,8 +179,7 @@ function _draw(ref, resource: VersionedResource) { /* Create the x-scale and draw the x-axis */ const x = d3.scaleTime() - // presence of dates on resource has already been checked so this assertion is safe - .domain([flatData[0]!.date, new Date()]) // the domain extends to the present day + .domain([flatData[0].date, new Date()]) // the domain extends to the present day .range([graphIndent, width-graphIndent]) svg.append('g') .attr("transform", `translate(0, ${heights.height - heights.marginBelowAxis})`) From 14e0e825b2c01247e610666087328e6fbd4c785c Mon Sep 17 00:00:00 2001 From: Victor Lin <13424970+victorlin@users.noreply.github.com> Date: Wed, 13 Nov 2024 15:56:08 -0800 Subject: [PATCH 09/10] _setDisplayName: Replace optional chaining with proper error handing With the current code, it is not possible for resources[i-1] to be undefined. Make this clear with type narrowing. --- static-site/src/components/ListResources/ResourceGroup.tsx | 6 +++++- 1 file changed, 5 insertions(+), 1 deletion(-) diff --git a/static-site/src/components/ListResources/ResourceGroup.tsx b/static-site/src/components/ListResources/ResourceGroup.tsx index 079e86536..5a9345aba 100644 --- a/static-site/src/components/ListResources/ResourceGroup.tsx +++ b/static-site/src/components/ListResources/ResourceGroup.tsx @@ -265,7 +265,11 @@ function _setDisplayName(resources: Resource[]): DisplayNamedResource[] { if (i===0) { name = r.nameParts.join(sep); } else { - let matchIdx = r.nameParts.map((word, j) => word === resources[i-1]?.nameParts[j]).findIndex((v) => !v); + const previousResource = resources[i-1]; + if (previousResource === undefined) { + throw new InternalError("Previous resource is undefined. Check that this is not run on i===0."); + } + let matchIdx = r.nameParts.map((word, j) => word === previousResource.nameParts[j]).findIndex((v) => !v); if (matchIdx===-1) { // -1 means every word is in the preceding name, but we should display the last word anyway matchIdx = r.nameParts.length-2; } From f8feb3eddb745208e6e02b1659617f7a066367f1 Mon Sep 17 00:00:00 2001 From: Victor Lin <13424970+victorlin@users.noreply.github.com> Date: Wed, 13 Nov 2024 16:08:22 -0800 Subject: [PATCH 10/10] Throw an error if IndividualResource cannot be rendered With the current code, this condition will always fail. Prevent this from silently passing and instead raise an error. --- .../src/components/ListResources/IndividualResource.tsx | 9 +++++---- 1 file changed, 5 insertions(+), 4 deletions(-) diff --git a/static-site/src/components/ListResources/IndividualResource.tsx b/static-site/src/components/ListResources/IndividualResource.tsx index 177d3272a..2e1ad18f6 100644 --- a/static-site/src/components/ListResources/IndividualResource.tsx +++ b/static-site/src/components/ListResources/IndividualResource.tsx @@ -133,10 +133,11 @@ export const IndividualResource = ({ const ref = useRef(null); const [topOfColumn, setTopOfColumn] = useState(false); useEffect(() => { - // don't do anything if the ref is undefined or the parent is not a div (IndividualResourceContainer) - if (!ref.current - || !ref.current.parentNode - || ref.current.parentNode.nodeName != 'DIV') return; + if (ref.current === null || + ref.current.parentNode === null || + ref.current.parentNode.nodeName != 'DIV') { + throw new InternalError("ref must be defined and the parent must be a div (IndividualResourceContainer)."); + } /* 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 */