Add types to ListResources

[victorlin]

Description

Taking a first stab at applying TypeScript to a set of components. Suggestions and discussion welcome for how I've used TypeScript in this PR, as it's likely to shape the way we use TypeScript in this project going forwards.

Related issues

• Based on Configure TypeScript for static-site #877
• Start using TypeScript #871

Discussions

• function style (ref)
• type vs. interface (ref)
• type/non-null assertions (ref)

Checklist

• Preview looks good, no functional changes
• Checks pass

[genehack]

++1 for getting started on this! Left some more or less nitpicky commentary; the any types are the thing I feel strongest about.

[ivan-aksamentov]

Not strictly Typescript related and very subjective, but as you are going through the code conversion, how do you guys feel about also reducing the punctuation noise?

Functions don't have to be anonymous closures assigned to named variables:

-export const useShowcaseCards = (cards: Card[], groups: Group[]) => {
+export function useShowcaseCards(cards: Card[], groups: Group[]) {

P.S. Watch out for this and other runtime quirks, especially in d3 code.

I believe there exist a transformer tool or a eslint rule with autofix to apply this en masse.

Also, interfaces don't have to be aliases to anonymous object types:

-type ShowcaseTileProps = {
+interface ShowcaseTileProps {

P.S. Watch out for differences as well, notably when extending.

This one is a bit longer to type, but there's 1 less token (=) to parse with your eyes and we all use autocomplete and snippets nowadays, right?

[victorlin]

Thanks @ivan-aksamentov and @genehack for the reviews – this kind of feedback and discussion is exactly what I'm looking for in this PR.

[ivan-aksamentov]

Looking at the code a little more closely I see that there are some cases of unsound js code (1, 2). It is difficult or impossible to provide sound typings for unsound code. The usual problems (and both of the examples I highlighted) are related to accessing potentially a null pointer.

Another example I saw is the ref.current.parentNode type casting to HTMLElement. This is an assumption ("we know") which can be broken, making this potentially a null pointer access undetectable by the compiler.

Hence, in terms of methodology, in the process of conversion, there is need for either:

• fixing the code
• adding a typing hack until later

One should decide which direction to chose on a case-by-case basis. If a hack direction is taken (cast, bang etc.), it is better to clearly document why it is needed with one of the loud comments, e.g. FIXME(ts) or HACK(types) and explain why types could not be correctly written here, without blaming the silly compiler, e.g.

// FIXME(ts): `dates` can be empty,
// but this code assumes at least 2 elements.
// We need explicit checks here (or a utility function).

not

// Silly compiler... Such a sweet summer child...
// Let's make it happy. Where is candies?
dates.at(999999999)!.explode

This way it's easy to grep and fix it later. By contrast, solitary bangs and casts are hard to find if there is no additional marking.

There is a trade-off:

• try to add typings with only minimal changes to the codebase (makeing it a refactoring)

• make typings sound (which requires fixes to the runtime behavior)

One of the things I like the most about TS is that when you add it, it highlights all the bad code immediately. And once TS is in place, it prevents from writing whole classes of new bad code - they will simply not compile. And if type hacks don't pass code reviews, then the codebase slowly becomes magically better.

[victorlin]

I've applied suggestions and cleaned up the commits – this is now ready for re-review.

[jameshadfield]

Adding types without changing too much code isn't easy, and I've no doubt the types will help development, so thanks for this work @victorlin!

No blocking changes, just questions. I'm surprised how many bangs were needed (I gave up making in-line comments!) and for my own learning I'd be interested to know if that's just the way it is or if it's an indication that the style of code used wasn't great and a better design is wanted. (I'm not saying a refactor / rewrite is worth our effort for this code at this point.)

[jameshadfield]

[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?

[victorlin]

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.

[jameshadfield]

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!

[ivan-aksamentov]

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

[ivan-aksamentov]

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.

[jameshadfield]

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.)

[victorlin]

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

[jameshadfield]

[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?

[ivan-aksamentov]

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.

[victorlin]

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

[jameshadfield]

[not a request for changes] similar to other comments, I'm surprised this is needed here - is this because tsc doesn't know if resources has been mutated underneath us, or because the if/else isn't informing the compiler that i>0?

[victorlin]

From 4.1 release notes which added noUncheckedIndexedAccess:

One consequence of using noUncheckedIndexedAccess is that indexing into an array is also more strictly checked, even in a bounds-checked loop.

[jameshadfield]

[not a request for changes] I guess the more ergonomic way is something like the following?

Object.entries(partitions).map(([groupName, resources]) => {

Is this assertion needed because the compiler isn't smart enough to know that group name is taken from the valid keys of partitions, or because mutability means that a key may have been removed from partitions before the map function gets that key?

[ivan-aksamentov]

It's simply because operator [] returns T | undefined. It does not care about any conditions. Here, you could try and annotate groupName: typeof keyof Partitions and then it might work. But not sure. Also it's not needed if iterating entries or just values instead.

This example demonstrates how modern, thoughtful code compiles with no additional effort, while more hacky code does not and requires even more hacks to "make compiler happy".

I would also explore a possibility of using partition() from lodash [docs] here, to avoid reinventing a well known algorithm. It might be nuanced though, and not immediately obvious. But could potentially be a one- or two-liner. I might be confusing this code with some other code

[victorlin]

@jameshadfield I got confused and didn't realize that [groupName, resources] is properly typed as long as the function is defined inline within Object.entries().map(). Changed 2aeb155 to 34a3140 which removes the need for the bang (reflected in force-push).

[victorlin]

Sorry that c9788e7 left you with so many (valid) questions @jameshadfield. Ideally that should've been broken into multiple commits with more detailed reasoning. At this point I'll just merge – happy to discuss other details with you later.

Also thanks @ivan-aksamentov for your advise here.

TypeScript incoming!