Skip to content

Commit

Permalink
Audit type assertions
Browse files Browse the repository at this point in the history 
Type assertions can lead to runtime errors and should be avoided. Two
alternatives:

1. Instead of asserting that certain resource properties are available
   through type assertions, check that they are actually available with
   null checks that will return null instead of an unexpected error.
2. Instead of asserting that array elements accessed via index are
   available through type assertions, use a non-null assertion which
   effectively does the same thing but without the need to cast the
   original type.

This leaves just one type assertion (parentNode as HTMLElement) with
added commentary on why it's necessary.
  • Loading branch information
@victorlin
victorlin committed Jun 3, 2024
1 parent 213995c commit 7b24562
Show file tree
Hide file tree
Showing 7 changed files with 40 additions and 41 deletions.
23 changes: 15 additions & 8 deletions static-site/src/components/ListResources/IndividualResource.tsx
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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, ResourceWithDisplayName, VersionedResource } from './types';
import { ResourceDisplayName, Resource } from './types';
import { IconType } from 'react-icons';

export const LINK_COLOR = '#5097BA'
Expand All @@ -19,13 +19,15 @@ 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: ResourceWithDisplayName[]) => {
export const getMaxResourceWidth = (displayResources: Resource[]) => {
return displayResources.reduce((w, r) => {
if (!r.displayName || !r.updateCadence) return w

/* add the pixels for the display name */
let _w = r.displayName.default.length * namePxPerChar;
if (r.nVersions) {
_w += gapSize + iconWidth;
_w += ((r?.updateCadence?.summary?.length || 0) + 5 + String(r.nVersions).length)*summaryPxPerChar;
_w += ((r.updateCadence.summary.length || 0) + 5 + String(r.nVersions).length)*summaryPxPerChar;
}
return _w>w ? _w : w;
}, 200); // 200 (pixels) is the minimum
Expand Down Expand Up @@ -107,7 +109,7 @@ export function IconContainer({Icon, text, handleClick, color, hoverColor}: Icon


interface IndividualResourceProps {
resource: ResourceWithDisplayName
resource: Resource
isMobile: boolean
}

Expand All @@ -119,30 +121,35 @@ export const IndividualResource = ({resource, isMobile}: IndividualResourceProps
if (!ref.current) return;
/* 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 */

/* offsetTop is not available on ParentNode but we know that the parent node
is an HTMLElement */
if (ref.current.offsetTop===(ref.current.parentNode as HTMLElement).offsetTop) {
setTopOfColumn(true);
}
}, []);

if (!setModalResource || !resource.displayName || !resource.updateCadence) return null

return (
<Container ref={ref}>

<FlexRow>

<TooltipWrapper description={`Last known update on ${resource.lastUpdated}`}>
<ResourceLinkWrapper onShiftClick={() => setModalResource && setModalResource(resource as VersionedResource)}>
<ResourceLinkWrapper onShiftClick={() => setModalResource(resource)}>
<Name displayName={resource.displayName} href={resource.url} topOfColumn={topOfColumn}/>
</ResourceLinkWrapper>
</TooltipWrapper>

{resource.versioned && !isMobile && (
<TooltipWrapper description={(resource as VersionedResource).updateCadence.description +
<TooltipWrapper description={resource.updateCadence.description +
`<br/>Last known update on ${resource.lastUpdated}` +
`<br/>${resource.nVersions} snapshots of this dataset available (click to see them)`}>
<IconContainer
Icon={MdHistory}
text={`${(resource as VersionedResource).updateCadence.summary} (n=${resource.nVersions})`}
handleClick={() => setModalResource && setModalResource(resource as VersionedResource)}
text={`${resource.updateCadence.summary} (n=${resource.nVersions})`}
handleClick={() => setModalResource(resource)}
/>
</TooltipWrapper>
)}
Expand Down
16 changes: 9 additions & 7 deletions static-site/src/components/ListResources/Modal.tsx
View file
Original file line number Diff line number Diff line change
Expand Up @@ -4,16 +4,16 @@ import styled from 'styled-components';
import * as d3 from "d3";
import { MdClose } from "react-icons/md";
import { dodge } from "./dodge";
import { VersionedResource } from './types';
import { Resource } from './types';

export const SetModalResourceContext = createContext<React.Dispatch<React.SetStateAction<VersionedResource | undefined>> | null>(null);
export const SetModalResourceContext = createContext<React.Dispatch<React.SetStateAction<Resource | undefined>> | null>(null);

export const RAINBOW20 = ["#511EA8", "#4432BD", "#3F4BCA", "#4065CF", "#447ECC", "#4C91BF", "#56A0AE", "#63AC9A", "#71B486", "#81BA72", "#94BD62", "#A7BE54", "#BABC4A", "#CBB742", "#D9AE3E", "#E29E39", "#E68935", "#E56E30", "#E14F2A", "#DC2F24"];
const lightGrey = 'rgba(0,0,0,0.1)';


interface ResourceModalProps {
resource?: VersionedResource
resource?: Resource
dismissModal: () => void
}

Expand Down Expand Up @@ -41,7 +41,7 @@ export const ResourceModal = ({resource, dismissModal}: ResourceModalProps) => {
_draw(ref, resource)
}, [ref, resource])

if (!resource) return null;
if (!resource || !resource.dates || !resource.updateCadence) return null;

const summary = _snapshotSummary(resource.dates);
return (
Expand Down Expand Up @@ -132,8 +132,8 @@ const Title = styled.div`

function _snapshotSummary(dates: string[]) {
const d = [...dates].sort()
const d1 = new Date(d.at( 0) as string).getTime();
const d2 = new Date(d.at(-1) as string).getTime();
const d1 = new Date(d.at( 0)!).getTime();
const d2 = new Date(d.at(-1)!).getTime();

This comment has been minimized.

Sign in to view

Copy link
@ivan-aksamentov

ivan-aksamentov Jun 3, 2024 •
edited
Loading

Member
If `dates` always contains 2 elements, then tuple type will be better here: 
function _snapshotSummary(dates: [string, string]) {
  const [d1, d2] = dates.map((date) => new Date(date).getTime())

Most importantly it avoids deceiving the compiler and prevents runtime errors from happening.

Totally misread this code.

I'd use a wrapper something like first() and last(), or firstAndLast() which throw on incorrect input.

This comment has been minimized.

Sign in to view

Copy link
@victorlin

victorlin Jun 4, 2024

Author Member

Updated to check for presence of at least one entry:

nextstrain.org/static-site/src/components/ListResources/Modal.tsx

Lines 136 to 139 in aae874c

if (d.length < 1) throw new Error("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;
let duration = '';
if (days < 100) duration=`${days} days`;
Expand All @@ -142,7 +142,9 @@ function _snapshotSummary(dates: string[]) {
return {duration, first: d[0], last:d.at(-1)};
}

function _draw(ref, resource: VersionedResource) {
function _draw(ref, resource: Resource) {
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 */
Expand Down
11 changes: 6 additions & 5 deletions static-site/src/components/ListResources/ResourceGroup.tsx
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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, ResourceWithDisplayName, VersionedResource } from './types';
import { Group, QuickLink, Resource } from './types';

interface ResourceGroupHeaderProps {
group: Group
Expand All @@ -23,6 +23,8 @@ const ResourceGroupHeader = ({group, isMobile, setCollapsed, collapsible, isColl
const resourcesByName = Object.fromEntries(group.resources.map((r) => [r.name, r]));
const quickLinksToDisplay = (quickLinks || []).filter((ql) => !!resourcesByName[ql.name] || ql.groupName===group.groupName)

if (!setModalResource) return null

return (
<HeaderContainer>

Expand Down Expand Up @@ -77,7 +79,7 @@ const ResourceGroupHeader = ({group, isMobile, setCollapsed, collapsible, isColl
)}
{quickLinksToDisplay.map((ql) => (
<div style={{paddingLeft: '5px'}} key={ql.name}>
<ResourceLinkWrapper onShiftClick={() => {setModalResource && setModalResource(resourcesByName[ql.name] as VersionedResource)}}>
<ResourceLinkWrapper onShiftClick={() => {setModalResource(resourcesByName[ql.name])}}>
<ResourceLink href={`/${ql.name}`} target="_blank" rel="noreferrer">
{ql.display}
</ResourceLink>
Expand Down Expand Up @@ -127,13 +129,12 @@ export const ResourceGroup = ({group, elWidth, numGroups, sortMethod, quickLinks
const [isCollapsed, setCollapsed] = useState(collapsible); // if it is collapsible, start collapsed
const displayResources = isCollapsed ? group.resources.slice(0, resourcesToShowWhenCollapsed) : group.resources;

// after this, displayResources can/should be safely casted to ResourceWithDisplayName[]
_setDisplayName(displayResources)

/* isMobile: boolean determines whether we expose snapshots, as we hide them on small screens */
const isMobile = elWidth < 500;

const maxResourceWidth = getMaxResourceWidth(displayResources as ResourceWithDisplayName[]);
const maxResourceWidth = getMaxResourceWidth(displayResources);

return (
<ResourceGroupContainer>
Expand All @@ -145,7 +146,7 @@ export const ResourceGroup = ({group, elWidth, numGroups, sortMethod, quickLinks

<IndividualResourceContainer $maxResourceWidth={maxResourceWidth}>
{/* what to do when there's only one tile in a group? */}
{(displayResources as ResourceWithDisplayName[]).map((d) => (
{(displayResources).map((d) => (
// We use key changes to re-render the component & thus recompute the DOM position
<IndividualResource resource={d} key={`${d.name}_${isCollapsed}_${elWidth}_${sortMethod}`} isMobile={isMobile}/>
))}
Expand Down
2 changes: 1 addition & 1 deletion static-site/src/components/ListResources/Showcase.tsx
View file
Original file line number Diff line number Diff line change
Expand Up @@ -102,7 +102,7 @@ const CardOuter = styled.div`

const themeColors = [...theme.titleColors];
const getColor = () => {
themeColors.push(themeColors.shift() as string); // this will always be string; type assertion is necessary to appease tsc
themeColors.push(themeColors.shift()!); // this will always be string; non-null assertion is necessary to appease tsc

This comment has been minimized.

Sign in to view

Copy link
@ivan-aksamentov

ivan-aksamentov Jun 3, 2024

Member

Perhaps not strictly relevant to the topic of the PR, but this looks like something that need to be fixed on js level. I'd add a prominent TODO or FIXME comment here. Current comment feels like the compiler is some sort of a weird unwanted dude here.

Is this code the same as

return [...theme.titleColors.slice(1), theme.titleColors[0]]

?

AFAIK .shift() mutates themeColors, which is a global variable. Or a copy? A shallow copy? And then the push of a shift. Who remembers what it does and what weird js rules apply here? You need a PhD in JS magic to understand this code.

It's hard to add types to code like this.

Bangs are really hard to find later on (you'd mostly find boolean "not" operator instead). So, if cannot fixed now, a loud comment would be great here.

This comment has been minimized.

Sign in to view

Copy link
@victorlin

victorlin Jun 4, 2024

Author Member

This would be the equivalent closest to your suggestion. It still needs non-null assertions:

let themeColors = [...theme.titleColors];
const getColor = () => {
  themeColors = [...themeColors.slice(1)!, themeColors[0]!]
  return themeColors.at(-1);
}

I've updated it with comments on what the code is doing:

nextstrain.org/static-site/src/components/ListResources/Showcase.tsx

Lines 103 to 109 in aae874c

const themeColors = [...theme.titleColors];
const getColor = () => {
// rotate colors by moving the first color (which is always defined) to the end
themeColors.push(themeColors.shift()!);
// return the last color
return themeColors.at(-1);
}

return themeColors.at(-1);
}

Expand Down
4 changes: 2 additions & 2 deletions static-site/src/components/ListResources/index.tsx
View file
Original file line number Diff line number Diff line change
Expand Up @@ -13,7 +13,7 @@ import { ErrorContainer } from "../../pages/404";
import { TooltipWrapper } from "./IndividualResource";
import {ResourceModal, SetModalResourceContext} from "./Modal";
import { Showcase, useShowcaseCards} from "./Showcase";
import { Card, FilterOption, Group, GroupDisplayNames, QuickLink, VersionedResource } from './types';
import { Card, FilterOption, Group, GroupDisplayNames, QuickLink, Resource } from './types';

interface ListResourcesProps extends ListResourcesResponsiveProps {
elWidth: number
Expand Down Expand Up @@ -45,7 +45,7 @@ function ListResources({
const [resourceGroups, setResourceGroups] = useState<Group[]>([]);
useSortAndFilter(sortMethod, selectedFilterOptions, groups, setResourceGroups)
const availableFilterOptions = useFilterOptions(resourceGroups);
const [modalResource, setModalResource ] = useState<VersionedResource>();
const [modalResource, setModalResource ] = useState<Resource>();

if (dataFetchError) {
return (
Expand Down
11 changes: 0 additions & 11 deletions static-site/src/components/ListResources/types.ts
View file
Original file line number Diff line number Diff line change
Expand Up @@ -28,17 +28,6 @@ export interface Resource {
updateCadence?: UpdateCadence
}

export interface ResourceWithDisplayName extends Resource {
displayName: ResourceDisplayName
}

export interface VersionedResource extends Resource {
firstUpdated: string // date
dates: string[]
nVersions: number
updateCadence: UpdateCadence
}

export interface ResourceDisplayName {
hovered: string
default: string
Expand Down
14 changes: 7 additions & 7 deletions static-site/src/components/ListResources/useDataFetch.ts
View file
Original file line number Diff line number Diff line change
Expand Up @@ -71,7 +71,7 @@ function partitionByPathogen(pathVersions: PathVersions, pathPrefix: string, ver
const nameParts = name.split('/');
const sortedDates = [...datum[1]].sort();

let groupName = nameParts[0] as string;
let groupName = nameParts[0]!;

if (!store[groupName]) store[groupName] = []
const resourceDetails: Resource = {
Expand All @@ -80,18 +80,18 @@ function partitionByPathogen(pathVersions: PathVersions, pathPrefix: string, ver
nameParts,
sortingName: _sortableName(nameParts),
url: `/${pathPrefix}${name}`,
lastUpdated: sortedDates.at(-1) as string,
lastUpdated: sortedDates.at(-1)!,
versioned
};
if (versioned) {
resourceDetails.firstUpdated = sortedDates[0] as string;
resourceDetails.lastUpdated = sortedDates.at(-1) as string;
resourceDetails.firstUpdated = sortedDates[0]!;
resourceDetails.lastUpdated = sortedDates.at(-1)!;
resourceDetails.dates = sortedDates;
resourceDetails.nVersions = sortedDates.length;
resourceDetails.updateCadence = updateCadence(sortedDates.map((date)=> new Date(date)));
}

(store[groupName] as Resource[]).push(resourceDetails)
(store[groupName])!.push(resourceDetails)

return store;
}
Expand All @@ -105,12 +105,12 @@ function partitionByPathogen(pathVersions: PathVersions, pathPrefix: string, ver
function groupsFrom(partitions: Partitions, pathPrefix: string, defaultGroupLinks: boolean, groupDisplayNames: GroupDisplayNames) {

return Object.keys(partitions).map(groupName => {
const resources = partitions[groupName] as Resource[];
const resources = partitions[groupName]!;
const groupInfo: Group = {
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) as string,
lastUpdated: resources.map((r) => r.lastUpdated).sort().at(-1)!,
resources,
}
/* add optional properties */
Expand Down

0 comments on commit 7b24562

Please sign in to comment.