Safer typing for records

Description

Many of the bugs that we encounter in this application are due to the fact that Typescript is not safe-by-default when it comes to indexed access in arrays and objects/records. The type of a value that is accessed on a record (e.g. state.eventByCampaignId[campId]) will always be assumed to be the value type of the record (e.g. ZetkinEvent) even though in many cases a more accurate type would include undefined (e.g. ZetkinEvent | undefined) since we can't know that the key has previously been set.

For background on this "flaw" in Typescript, see (among many other soruces) this StackOverflow thread, this issue referenced in one of the responses, and the many issues linked from that issue.

Relevant Job Stories

N/A

Estimated size (S, M, L)

At this point, it's difficult to assess. I can see multiple solutions to this, and while it could be possible to start really small by introducing a custom type and using it in some places only, the other end of the spectrum entails enabling noUncheckedIndexedAccess which yields 700+ typescript errors in the codebase.

Prerequisites

None

Requirements

[ ] It should be harder to mistakingly write record indexing code that causes TypeError: Cannot read properties of undefined

Possible implementations

One option is to enable noUncheckedIndexedAccess in tsconfig.json. This is the most type-safe option, because it means we can never use [] access on an array or object without checking the value. Currently, however, this causes more than 700 compile errors, and lots of them are bound to be false positives. For example, with that option enabled, it's not possible to do a typical for (let i=0; i < array.length; i++) { array[i].doSomething(); } because array[i] could in theory be undefined.

Part of the problem here is that noUncheckedIndexedAccess treats arrays and objects/records the same, which I'm sure there is some good reason for, but at least in our case we nearly never have this problem with arrays, but have it quite often with records.

So another option is to introduce a new utility type that we can use instead of Record and start gradually replacing our use of Record with SafeRecord (whenever it makes sense). Here's what that could look like:

Before:

type SomeObj = { title: string };

const objById: Record<number, SomeObj> = {};
const obj = objById[123]; // Type inferred: SomeObj

// This would pass typescript but throw exception because obj is actually undefined
console.log(obj.title);

After:

type SafeRecord<K, V> = Record<K, V | undefined>; // Reusable utility type
type SomeObj = { title: string };

const objById: SafeRecord<number, SomeObj> = {};
const obj = objById[123]; // Type inferred: SomeObj | undefined

// This condition is required to pass typescript, and avoids the runtime exception
if (obj) {
  console.log(obj.title);
}

Open questions

Which route should we take?

Hello! I actually was thinking of how to tackle a similar problem at work recently, because we have an API which isn't really trustworthy. Either the key won't be present, or the value could be null or undefined. So I was thinking of using a utility type when I make a solution, very similar to above, but that it would be used mostly for intefaces rather than records which don't specify properties.

// From the API
type ZetkinJourney = SafeRecord<{ id: number, ...}>;

journey.id // Would be `number | null | undefined`

I think utility type is a good idea. But I wonder if it's "good enough" to define the SafeRecord on a parent someplace, and then the types descend down. So you only minimally have to actually write it out. I hope that makes sense. If it's something that will need to be written out all over the place, rather than for example on a parent Zetkin... type, then maybe it's too clunky, prone to being forgotten, and at that point you should turn on noUncheckedIndexedAccess. Hope that's useful and I also hope I understood well enough to give a helpful perspective!

zetkin / app.zetkin.org