Add types to ListResources

victorlin commented 1 month ago

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 #877
871

Discussions

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

Checklist

[x] Preview looks good, no functional changes
[x] Checks pass

ivan-aksamentov commented 1 month ago

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 commented 1 month ago

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 commented 3 weeks ago

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 commented 3 weeks ago

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

victorlin commented 3 weeks ago

Sorry that c9788e758c14413fa792de429427f545c3bcde9f 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!

nextstrain / nextstrain.org