Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add types to ListResources #874

Merged
merged 6 commits into from
Jun 5, 2024
Merged

Add types to ListResources #874

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
6 commits
Select commit Hold shift + click to select a range
5186c63
Remove .jsx extension from imports
victorlin May 24, 2024
34a3140
Simplify intermediate functions
victorlin Jun 4, 2024
432577b
Add types to ListResources
victorlin Jun 3, 2024
82b0521
Rename variables to reflect types
victorlin May 21, 2024
54be4d6
Fix error-prone code exposed by TypeScript conversion
victorlin Jun 4, 2024
7c750e2
Add individual exceptions for unused vars
victorlin May 24, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 2 additions & 2 deletions static-site/src/components/Cards/styles.jsx → static-site/src/components/Cards/styles.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -15,7 +15,7 @@ export const CardInner = styled.div`
cursor: pointer;
`;

export const CardOuter = styled.div`
export const CardOuter = styled.div<{$squashed: boolean}>`
background-color: #FFFFFF;
padding: 0;
overflow: hidden;
Expand All @@ -38,7 +38,7 @@ export const CardOuter = styled.div`
}
`;

export const CardTitle = styled.div`
export const CardTitle = styled.div<{$squashed: boolean}>`
font-family: ${(props) => props.theme.generalFont};
font-weight: 500;
font-size: ${(props) => props.$squashed ? "22px" : "26px"};
Expand Down
93 changes: 64 additions & 29 deletions ...ents/ListResources/IndividualResource.jsx → ...ents/ListResources/IndividualResource.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2,7 +2,9 @@
import React, {useState, useRef, useEffect, useContext} from 'react';
import styled from 'styled-components';
import { MdHistory } from "react-icons/md";
import { SetModalContext } from './Modal';
import { SetModalResourceContext } from './Modal';
import { ResourceDisplayName, Resource } from './types';
import { IconType } from 'react-icons';

export const LINK_COLOR = '#5097BA'
export const LINK_HOVER_COLOR = '#31586c'
Expand All @@ -17,27 +19,36 @@ 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) => {
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
}

export const ResourceLink = styled.a`
export const ResourceLink = styled.a<{$hovered?: boolean}>`
font-size: ${resourceFontSize}px;
font-family: monospace;
white-space: pre; /* don't collapse back-to-back spaces */
color: ${(props) => props.$hovered ? LINK_HOVER_COLOR : LINK_COLOR} !important;
text-decoration: none !important;
`;

function Name({displayName, $hovered, href, topOfColumn}) {
interface NameProps {
displayName: ResourceDisplayName
$hovered?: boolean
href: string
topOfColumn: boolean
}

function Name({displayName, $hovered = false, href, topOfColumn}: NameProps) {
return (
<ResourceLink href={href} target="_blank" rel="noreferrer" $hovered={$hovered}>
{'• '}{($hovered||topOfColumn) ? displayName.hovered : displayName.default}
Expand Down Expand Up @@ -70,7 +81,15 @@ export function TooltipWrapper({description, children}) {
)
}

export function IconContainer({Icon, text, handleClick=undefined, color=undefined, hoverColor=undefined}) {
interface IconContainerProps {
Icon: IconType
text: string
handleClick?: () => void
color?: string
hoverColor?: string
}

export function IconContainer({Icon, text, handleClick, color, hoverColor}: IconContainerProps) {
const [hovered, setHovered] = useState(false);
const defaultColor = '#aaa';
const defaultHoverColor = "rgb(79, 75, 80)";
Expand All @@ -89,45 +108,61 @@ export function IconContainer({Icon, text, handleClick=undefined, color=undefine
}


/**
*
* @param {*} param0
* @returns
*/
export const IndividualResource = ({data, isMobile}) => {
const setModal = useContext(SetModalContext);
const ref = useRef(null);
interface IndividualResourceProps {
resource: Resource
isMobile: boolean
}

export const IndividualResource = ({resource, isMobile}: IndividualResourceProps) => {
const setModalResource = useContext(SetModalResourceContext);
if (!setModalResource) throw new Error("Context not provided!")

const ref = useRef<HTMLDivElement>(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;

/* 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.offsetTop) {
if (ref.current.offsetTop===(ref.current.parentNode as HTMLDivElement).offsetTop) {
setTopOfColumn(true);
}
}, []);

// don't show anything if display name is unavailable
if (!resource.displayName) return null

// add history if mobile and resource is versioned
let history: React.JSX.Element | null = null
if (!isMobile && resource.versioned && resource.updateCadence && resource.nVersions) {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[not a request for changes] in lieu of using types to differentiate versioned vs unversioned resources, I wonder if a helper function would be clarifying here, e.g. (resource: Resource) => boolean, or is there more nuance around "versioned" vs "unversioned" than I remember?

Copy link
Member Author

@victorlin victorlin Jun 5, 2024 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The previous code used presence of resource.versioned to determine if resource.updateCadence and resource.nVersions are available, but that's operating under the assumption that those are only set if resource.versioned = true which is a loose coupling that the compiler is unaware of:

src/components/ListResources/IndividualResource.tsx:138:40 - error TS18048: 'resource.updateCadence' is possibly 'undefined'.

138           <TooltipWrapper description={resource.updateCadence.description +
                                           ~~~~~~~~~~~~~~~~~~~~~~

The proper way to handle the case when those attributes are unavailable is to check their presence directly.

I would go one step further and remove the versioned attribute since it's only use is here as an alias. Something like this: #894

A helper function would only provide the same loose coupling as the current versioned attribute.

history = (
<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.updateCadence.summary} (n=${resource.nVersions})`}
handleClick={() => setModalResource(resource)}
/>
</TooltipWrapper>
)
}

return (
<Container ref={ref}>

<FlexRow>

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

{data.versioned && !isMobile && (
<TooltipWrapper description={data.updateCadence.description +
`<br/>Last known update on ${data.lastUpdated}` +
`<br/>${data.nVersions} snapshots of this dataset available (click to see them)`}>
<IconContainer
Icon={MdHistory}
text={`${data.updateCadence.summary} (n=${data.nVersions})`}
handleClick={() => setModal(data)}
/>
</TooltipWrapper>
)}
{history}

</FlexRow>

Expand Down
61 changes: 41 additions & 20 deletions ...te/src/components/ListResources/Modal.jsx → ...te/src/components/ListResources/Modal.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -4,13 +4,20 @@ import styled from 'styled-components';
import * as d3 from "d3";
import { MdClose } from "react-icons/md";
import { dodge } from "./dodge";
import { Resource } from './types';

export const SetModalContext = createContext(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)';

export const ResourceModal = ({data, dismissModal}) => {

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

export const ResourceModal = ({resource, dismissModal}: ResourceModalProps) => {
const [ref, setRef] = useState(null);
const handleRef = useCallback((node) => {setRef(node)}, [])

Expand All @@ -30,32 +37,33 @@ export const ResourceModal = ({data, dismissModal}) => {
}, []);

useEffect(() => {
if (!ref || !data) return;
_draw(ref, data)
}, [ref, data])
if (!ref || !resource) return;
_draw(ref, resource)
}, [ref, resource])

if (!data) return null;
// modal is only applicable for versioned resources
if (!resource || !resource.dates || !resource.updateCadence) return null;

const summary = _snapshotSummary(data.dates);
const summary = _snapshotSummary(resource.dates);
return (
<div ref={scrollRef}>

<Background onClick={dismissModal}/>
<ModalContainer onClick={(e) => {e.stopPropagation()}}>
<CloseIcon onClick={dismissModal}/>
<Title>{data.name.replace(/\//g, "│")}</Title>
<Title>{resource.name.replace(/\//g, "│")}</Title>

<div style={{paddingBottom: '5px'}}>
<Bold>
{`${data.dates.length} snapshots spanning ${summary.duration}: ${summary.first} - ${summary.last}`}
{`${resource.dates.length} snapshots spanning ${summary.duration}: ${summary.first} - ${summary.last}`}
</Bold>
<a style={{fontSize: '1.8rem', paddingLeft: '10px'}}
href={`/${data.name}`} target="_blank" rel="noreferrer noopener">
href={`/${resource.name}`} target="_blank" rel="noreferrer noopener">
(click to view the latest available snapshot)
</a>
</div>
<div>
{data.updateCadence.description}
{resource.updateCadence.description}
</div>

<div ref={handleRef} /> {/* d3 controlled div */}
Expand Down Expand Up @@ -123,22 +131,28 @@ const Title = styled.div`
padding-bottom: 15px;
`

function _snapshotSummary(dates) {
function _snapshotSummary(dates: string[]) {
const d = [...dates].sort()
const [d1, d2] = [d[0], d.at(-1)].map((di) => new Date(di));
const days = (d2-d1)/1000/60/60/24;
if (d.length < 1) throw new Error("Missing dates.")

const d1 = new Date(d.at( 0)!).getTime();
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why is this bang needed? the preceding line ensures that there is at lease one element in d. Similarly for the subsequent line (d.at(-1)).

P.S. there's a bug in the UI for datasets with a single date where we end up reporting "0 days". But that's not for this PR!

Copy link
Member

@ivan-aksamentov ivan-aksamentov Jun 5, 2024 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why is this bang needed? the preceding line ensures that there is at lease one element in d.

Compiler is not smart enough to figure this out from this kind of the conditional. Connecting .length to .at() is tough for a type system.

What it can easily figure out is null checks. So a cleaner way would be to check and throw after non-fallible array access:

const t0 = d.at(0)?.getTime()
if(isNil(t0)) { // careful to not exclude the legit value `0`
    throw ...
}
// `t0` is guaranteed to be `number` in this branch

I would go further. Finding first and last value is a common enough "algorithm" that I would introduce a utility function:

export function firstLastOrThrow<T>(a: T[]): [T, T] {
    // TODO: use .at() or lodash head() and tail() along with null checks 
}

This would make the code very pretty and safe, with all dirty details hidden:

const [t0, t1] = firstLastOrThrow(dates) // guaranteed to return a pair of numbers or to  throw

Copy link
Member

@ivan-aksamentov ivan-aksamentov Jun 5, 2024 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Another thing is that this entire calculation is wrong. For example, not all years have 365 days. Date-time calculations is a very hard topic, and it is somewhat unreasonable to try and do on the spot. There are specialized high-quality libraries exist to do this correctly, e.g. luxon (continuation of moment.js). And then for human durations there are also small libs (e.g. this). Not even talking about that calendars and durations are different in different cultures.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Another thing is that this entire calculation is wrong.

(Let's break this out to a separate conversation if we want to continue discussing it -- I am aware how fraught date parsing is but I'll argue that the data this function returns is (purposefully) vague such that the overall function is correct.)

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Creating separate issues to continue these discussions: #892, #893

const d2 = new Date(d.at(-1)!).getTime();
const days = (d2 - d1)/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)};
}

function _draw(ref, data) {
function _draw(ref, resource: Resource) {
// 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 = [...data.dates].sort();
const sortedDateStrings = [...resource.dates].sort();
const flatData = sortedDateStrings.map((version) => ({version, 'date': new Date(version)}));

const width = ref.clientWidth;
Expand All @@ -164,7 +178,8 @@ function _draw(ref, data) {

/* Create the x-scale and draw the x-axis */
const x = d3.scaleTime()
.domain([flatData[0].date, new Date()]) // the domain extends to the present day
// presence of dates on resource has already been checked so this assertion is safe
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[not a request for changes] do you understand why this bang is needed? Is it a limitation of how types flow through d3, or is it how we're using it?

Copy link
Member

@ivan-aksamentov ivan-aksamentov Jun 5, 2024 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Bracket notation returns undefined if accessing out of bounds (IIRC), so the type of a is potentially undefined.

The problem with comments like this:

// presence of dates on resource has already been checked so this assertion is safe

is that 3 months from now a fresh intern comes and inserts new code between the check and the assertion (not knowing that this assertion exists - it's hard to find even when looking for it specifically) and then 💥. Though, to be fair, it did not happen with js previously (due to lack of interns?)

Again, a small wrapper would make it safe and clean:

export function at<T>(a: T[], index: number): T {}

If you can find a fallback value, then something like this might work:

flatData[0]?.date ?? new Date()

This cannot fail and requires no hacks.

Continuing with the wrapper (though not directly applicable here sadly):

export function at<T>(a: T[], index: number, fallback?: T): T {}

Another option, although more involved, is to write a custom array type which always returns value or throws and never returns undefined. There might be libraries implementing non-empty arrays.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

flatData is an array that could have a length of zero. In that case, flatData[0] is undefined and flatData[0].date would be an error. This is caught by the rules strictNullChecks + noUncheckedIndexedAccess. The initial TypeScript feature request for this is good context: microsoft/TypeScript#13778

The added comment describes the assumption of previous logic that's necessary for this bang to be valid, however it's fragile as @ivan-aksamentov mentions above.

Creating an issue to explore alternatives: #892

.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})`)
Expand Down Expand Up @@ -232,9 +247,11 @@ function _draw(ref, data) {
.attr("cy", (d) => heights.height - heights.marginBelowAxis - heights.marginAboveAxis - radius - padding - d.y)
.attr("r", radius)
.attr('fill', color)
// @ts-expect-error
.on("mouseover", function(e, d) {
/* lower opacity of non-hovered, increase radius of hovered circle */
beeswarm.join(
// @ts-expect-error
(enter) => {}, /* eslint-disable-line */
(update) => selectSnapshot(update, d)
)
Expand All @@ -254,15 +271,17 @@ function _draw(ref, data) {
})
.on("mouseleave", function() {
beeswarm.join(
// @ts-expect-error
(enter) => {}, /* eslint-disable-line */
(update) => resetBeeswarm(update)
)
/* hide the vertical line + text which appeared on mouseover */
selectedVersionGroup.selectAll("*")
.style("opacity", 0)
})
// @ts-expect-error
.on("click", function(e, d) {
window.open(`/${data.name}@${d.data.version}`,'_blank'); // TEST!
window.open(`/${resource.name}@${d.data.version}`,'_blank'); // TEST!
})

/**
Expand All @@ -281,6 +300,7 @@ function _draw(ref, data) {
.on("mousemove", function(e) {
const { datum, hoveredDateStr } = getVersion(e);
beeswarm.join(
// @ts-expect-error
(enter) => {}, /* eslint-disable-line */
(update) => selectSnapshot(update, datum)
)
Expand All @@ -301,6 +321,7 @@ function _draw(ref, data) {
})
.on("mouseleave", function() {
beeswarm.join(
// @ts-expect-error
(enter) => {}, /* eslint-disable-line */
(update) => resetBeeswarm(update)
)
Expand All @@ -309,7 +330,7 @@ function _draw(ref, data) {
})
.on("click", function(e) {
const { datum } = getVersion(e);
window.open(`/${data.name}@${datum.data.version}`,'_blank');
window.open(`/${resource.name}@${datum.data.version}`,'_blank');
})

function selectSnapshot(selection, selectedDatum) {
Expand Down Expand Up @@ -345,7 +366,7 @@ function _draw(ref, data) {

const dateWithYear = d3.utcFormat("%B %d, %Y");
const dateSameYear = d3.utcFormat("%B %d");
function prettyDate(mainDate, secondDate) {
function prettyDate(mainDate: string, secondDate?: string) {
const d1 = dateWithYear(new Date(mainDate));
if (!secondDate) return d1;
const d2 = (mainDate.slice(0,4)===secondDate.slice(0,4) ? dateSameYear : dateWithYear)(new Date(secondDate));
Expand Down
Loading