Skip to content

Type settings entries via module declaration merging

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)

see here for info about declaration merging

Ideally, we would be able to just declare what setting keys we have, and what types they have, so that we don't need to worry about incorrect typings elsewhere in the code, i.e. something like:

interface SettingTypes {
  'key-1': string
  'key-2': number
}

class Settings {
  public get<SettingKey extends keyof SettingTypes>(id: SettingKey): SettingTypes[SettingKey] {
    // ...
  }
}

Due to the very modular nature of the Recovery Cat code base however, we can't just delcare this in the settings service, as they're declared elsewhere.

Typescript allows however for merging interface declarations across modules, via the declare module syntax:

settings.module.ts

/** Interface here is empty, and must be expanded upon by other modules */
interface SettingTypes {
}

another.module.ts

declare module '@settings/path' {
  interface SettingTypes {
    'another-module-key': string
  }
}

Altogether, this looks something like:

interface SettingTypes {

}

const get = <SettingKey extends keyof SettingTypes>(id: SettingKey): SettingTypes[SettingKey] => {
  let returnValue: SettingTypes[SettingKey] = getValueFromStore(id)
  return returnValue
}

// interface expanded in another module, via `declare module '@module/path'`
interface SettingTypes {
  foo: string
  bar: number
}

const foo: string = get('foo')  // Must be a string
const bar: number = get('bar')  // Must be a number

What's going on

interface SettingTypes {

}

The interface is initially left empty, as the settings service doesn't have the responsibility of saying what settings there, they're delcared elsewhere

const get = <SettingKey extends keyof SettingTypes>(id: SettingKey): SettingTypes[SettingKey] => {
  let returnValue: SettingTypes[SettingKey] = getValueFromStore(id)
  return returnValue
}

Although we don't yet know what's going to be in the interface, we know that when used, the return type should be whatever the value declared in the interface is

declare module '@settings/settings.module' {
  interface SettingTypes {
    foo: string
    bar: number
  }
}

const foo: string = get('foo')  // Must be a string
const bar: number = get('bar')  // Must be a number

Here, in another module, we declare what keys and types we want, and when using the settings service, the types are then directly inferred, and would give a type error if contradicting, e.g.

const foo: string = get('f0o') // Gives a type error, as the setting key was misspelled, or doesn't exist
const bar: ADifferentType = get('bar') // Gives a type error, as the settings key was declared as number

Pros

  • Setting types are automatically inferred, no need to use get<string> etc.
  • Setting types are strongly typed, we can't say that a key is of type string, and eleswhere say it's a type of number
  • Responsibility for declaring setting types remains modular, and outside of the setting service
  • Get editor type hints when fetching a setting value, no need to look up in the code what the name of the settings key is, or where we might import it from
  • We'll get type errors if mistyping a settings key, i.e. settingsService.get('active-language') would give a type error, as it's declared as 'activeLanguage', not 'active-language'
  • We'll get type errors if requesting a key that has yet to be declared, forcing type maintenance

Cons

  • Some type maintenance required (it's not too much, but more than it was before)
  • No runtime checking (though we could add runtime checking some other way if we really want that, imo it's not needed)
Edited by James Jenkinson

Merge request reports

Loading