#363 Request for comments: Change type casting for settings service

Can be reviewed, but shouldn't be merged yet. NB this is why I upped the approval count.

This is meant as a proposal, that might solve the problem described in the ticket #363 (closed)

interface TypeAssertionMap {
  string: string
  boolean: boolean
}

type TypeAssertions = {
  [key in keyof TypeAssertionMap]: (value: unknown) => value is TypeAssertionMap[key]
}

const typeAssertions: TypeAssertions = {
  string: (value): value is string => typeof value === 'string',
  boolean: (value): value is boolean => typeof value === 'boolean'
}

const get = <Assertion extends keyof TypeAssertionMap>(id: string, type: Assertion): TypeAssertionMap[Assertion] => {
  let returnValue: unknown = getValueFromStore(id)
  const assertion = typeAssertions[type]

  if (!assertion(returnValue)) {
    throw new Error(`Expected type ${type}, got instead ${typeof returnValue}`)
  }
  return returnValue
}

const foo = get('foo', 'string') // has an inferred type of string
const bar = get('bar', 'boolean') // has an inferred type of boolean
const lemon = get('lemon', 'number') // doesn't exist! Gives a type error

What's going on

interface TypeAssertionMap {
  string: string
  boolean: boolean
}

This provides a record of the assertion methods we want to provide, and what types they should correspond to, this is how we know what the return type is when we call the method

type TypeAssertions = {
  [key in keyof TypeAssertionMap]: (value: unknown) => value is TypeAssertionMap[key]
}

const typeAssertions: TypeAssertions = {
  string: (value): value is string => typeof value === 'string',
  boolean: (value): value is boolean => typeof value === 'boolean'
}

This is a mapped type, that says every key with TypeAssertionMap needs to be present, and the value should be a type assertion function, of the value type in TypeAssertionMap

• If you don't add a key, or add anew key to TypeAssertionMap without adding an assertion function, you'll get a type error
• If the return type of the function in the assertion, isn't that in the type of TypeAssertionMap, you'll get a type error, i.e.

const typeAssertions: TypeAssertions = {
  boolean: (value): value is FooType => typeof value === 'boolean'
}

will give an error, because it needs to return value is boolean

const get = <Assertion extends keyof TypeAssertionMap>(id: string, type: Assertion): TypeAssertionMap[Assertion] => { ... }

The content of the function isn't important, what's important is the type signature. The second parameter must be a key of TypeAssertionMap (i.e. 'string' or 'number'). The return type is then the value type set in TypeAssertionMap, i.e. 'string' => string, 'number' => number etc.

const foo: string = get('foo', 'string') // Good, no error
const foo: boolean = get('foo', 'string') // Bad! Type error!

Pros

• We get to keep infered types of get, set, valueChanges
• We get additional runtime type checking, that has to match with the compile time checking
• We currently only have two types in use, string and boolean, so it's not complex to implement

Cons

• Need to provide typeguard functions for each type we want to cast to
• Still need to know ahead of time, what type a given ID will be, makes updating this difficult (though we had this problem before)
• Runtime typechecking has slight perf cost (but we're doing that in lots of places anyway)

With regards to the first problem, we could potentially keep both the old type casting, and the type assertions at the same type, via operator overloading, e.g.

class Settings {
  /** @deprecated Use get('id', 'type') instead, adding a new type assertion if necessary */
  public get<Type>(id: string): Type
  public get = <Assertion extends keyof TypeAssertionMap>(id: string, type: Assertion): TypeAssertionMap[Assertion]
  public get(id: string, type?: keyof TypeAssertionMap) {
    // ...
  }

  // Will be flagged as deprecated, but still will work
  const old: string = settings.get<string>('old-id')
  const new: string = settings.get('new-id', 'string')
}

This way, we can continue to ues the function in the old way, but gradually replace with the old way.

Not quite sure about the deprecated warning though