types.ts#L81-L126

This contributes to the abundance of strictNullChecks violations.

From @jameshadfield in https://github.com/nextstrain/auspice/pull/1864#pullrequestreview-2419502455:

For the upcoming work exploring reducing the optional parameters on types let's explore a few ideas at a small scale to better see which approach will be best for us!

Possible solutions

Note: the solution may vary per type/interface

Use mapped types
- I started on this in d08ac2b8fab84f26d0305ad26f93cd054a33ade9...12aff241c7fed96675ee691b445ffa6cd72552ea but dropped it to keep the PR simple.
Have a type A with lots of optional properties that we can use as we build up the data structure then turn this into a type B once it arrives in redux and we know all/most properties have been defined. Type predicates may be useful here. (ref)
?

genehack commented 2 weeks ago

Also worth thinking about ways of handling this on a class level, e.g. this-based type guards.

victorlin commented 2 weeks ago

Re: https://github.com/nextstrain/auspice/issues/1888#issuecomment-2462953779

As we discussed yesterday, I think the best approach here is to have 2 types per "thing" — one where the properties are largely optional and a second where they're not (insert naming bikeshed convo here...) — and then use type guards to "up-convert" the optional form into the fully reified one.

I would like to try this out on something like ReduxNode first since it has properties such as ReduxNode.children?: ReduxNode. So if there is a ReduxNodeA.children?: ReduxNodeA and ReduxNodeB.children: ReduxNodeB, it's not clear to me how the "up-conversion" will work.

jameshadfield commented 2 weeks ago

I would like to try this out on something like ReduxNode first since it has properties such as ReduxNode.children?: ReduxNode. So if there is a ReduxNodeA.children?: ReduxNodeA and ReduxNodeB.children: ReduxNodeB, it's not clear to me how the "up-conversion" will work.

I was also interested and gave it a shot this morning in-between meetings:

interface LoadingTypeI {
  foo?: number[]; // example of optional property
  children: LoadingTypeI[];
}

interface LoadedTypeI {
  // Doesn't need to be written out like this, you can use some conditional recursive
  // TS magic to turn LoadingTypeI into this
  foo: number[];
  children: LoadedTypeI[];
}

// (Not shown: we've used a whole bunch of functions to incrementally create the data & now consider it loaded)
const data: LoadingTypeI = {
  foo: [1,2,3],
  children: [
    {foo: [4,5,6], children: []},
    {foo: [7,8,9], children: []},
  ]
}

function isLoadedTypeI(x: LoadingTypeI | LoadedTypeI): x is LoadedTypeI {
  // type predicate <https://www.typescriptlang.org/docs/handbook/2/narrowing.html#using-type-predicates>
  // note that this is not free!
  const properties_to_enforce = ['foo']; // Can use some TS magic to get this from the type itself
  let complete=true;
  const stack=[x]
  while (stack.length) {
    const node = stack.shift()
    if (node===undefined) break; // TS limitation? Is this to do with sparse arrays? I though TS didn't know about them
    for (const child of node.children) {stack.push(child)};
    if (properties_to_enforce.some((p) => node[p]===undefined)) {
      complete=false;
      break;
    }
  }
  return complete;
}

function toRedux(x: LoadingTypeI): LoadedTypeI {
  if (isLoadedTypeI(x)) return x;
  throw new Error(); // make sure this is handled by an error boundary in React
}

function dispatch(arg: any):void {console.log("DISPATCH:", arg)}

dispatch({type: "LOAD", data: toRedux(data)})

toRedux(data).foo.length; // no problem - foo is guaranteed to exist

jameshadfield commented 2 weeks ago

I played around with this after resurrecting an old prototype of adding types to reduxState.metadata, essentially defining a type for the "complete" state intended to be used by rendering components etc:

interface MetadataState {
  loaded: true; // discriminant property
  title?: string;
  mainTreeNumTips: number;
  identicalGenomeMapAcrossBothTrees: boolean;

and a "builder" type used when parsing the JSON and also as the default redux state:

type IncompleteMetadataState = Partial<Omit<MetadataState, "loaded">> & {loaded: false};

At the end of dataset loading we run something like

function convertIncompleteMetadataStateToMetadataState(meta: IncompleteMetadataState): MetadataState

which will throw if the metadata is incomplete such that Auspice can't visualise the data. Within components we can then use the discriminant property to narrow the type, essentially asserting everything is as is should be:

const mapState = (state: RootState) => {
  if (!state.metadata.loaded) throw new Error("Something's gone seriously wrong")
  return {metadata: state.metadata}
}

It all felt a little bit hard. I'm sure it can be simplified. But the overall result is the ability to simplify rendering components as we don't need to constantly check a property is defined or not.

nextstrain / auspice

TypeScript: Reduce the amount of optional properties #1889

1854 is set to add types/interfaces with an abundance of optional properties. Example: https://github.com/nextstrain/auspice/blob/6ca81c61632e22ae5b81e8e052d4bfc570e49d96/src/components/tree/phyloTree/types.ts#L81-L126

Possible solutions