TypeScript Type Traps

[msorens]

🔩 Description: What code changed, and why?

On the periphery of some UI work I noticed some type safety anomalies where things that should have been compiler errors (or so I thought) were not flagged as such. Upon refreshing myself on how static typing, type inference, and type assertions actually work in TypeScript I made some surprising discoveries. This PR fixes an assortment of such issues.

They are grouped by commit into several violation categories, so follow the commits!

Here is one startling example:

After entering an invalid value (CUSTOMxxx) for a type, the error is not flagged in VSCode until one removes the type assertion:

⛓️ Related Resources

Alas, these types of potential pitfalls are not going to be noticed by the computer no matter how much linting you do. So I wrote up some slides ("TypeScript Type Traps") to document and explain these for the team's reference moving forward.

---

Please read TypeScript Type Traps

---

👍 Definition of Done

A more type-safe UI codebase.

👟 How to Build and Test the Change

Rebuild automate-ui

[srenatus]

Thanks! I appreciate that we try to use typescript properly 😃

[srenatus]

Worth changing the user interface to something like

interface User {
  id?: string;
  name?: string;
  membership_id?: string;
}

...so we could not set them to empty strings here?

[msorens]

Here is my reasoning: in any strongly-typed language, if you have a User object that contains a property name, you know with certainty that the property user.name exists. But with the original code (user = <User>{};), or with your suggested change, we no longer have that certainty. That may or may not have any practical impact; referencing the non-existent user.name would return undefined instead of an empty string. For a non-string field, it becomes more problematic. Say we had a type field that could be 'a' or 'b' only, but making it optional wouldn't undefined also need to be checked? That seems akin to the Worst Mistake in Computer Science (e.g. https://bit.ly/1JHIqbY), no? TypeScript lets us be tolerant, though, as you have suggested... but should we be? Not sure. 🤷‍♂

[srenatus]

in any strongly-typed language, if you have a User object that contains a property name, you know with certainty that the property user.name exists.

...but if we set user.name to '', that certainty isn't worth much: we don't know if it's a valid, proper username -- we only know it's not undefined. That's not the kind of certainty I appreciate 😄

That said, there's surely better ways than what I have proposed. Akin to a Maybe or Optional type, we might be able to say User | undefined. I've never done a deep dive into how to do this in TS The Right Way; I'm vaguely aware that there's a project called ts-fp that provides Option.

[bcmdarroch]

Why does this.user need to be initialized here?

[msorens]

@bcmdarroch: I was trying to answer that very question above; sorry if not clear. In a nutshell:

1. If you have something that is a User object, you might assign null to it, in which case it certainly does not have an id, name, etc.
2. But if it is non-null, I do not believe one should have to wonder (read: check or worse, blow-up because you did not check) if it has a property named id. It is a User object: it should always have that property. Now, whether that property itself is initialized--to @srenatus 's point--is worth considering, too, but I'm just taking the first, small step here.

[srenatus]

but I'm just taking the first, small step here.

Let's not spread this, but keep trying to improve it in each step, then 😃

[bcmdarroch]

👍 👍