POC of local storage manager

[tidy-dev]

Description

Some discussion sparked from #17370 around the desire to make it so that existing users keep the current behavior of showing the commit length warning, but having it so that future installations have this disabled by default - especially when we circle back to making the feature accessible meaning that it will be more obtrusive.

We discussed implementing something like:

desktop/app/src/lib/stores/app-store.ts

Lines 588 to 595 in a6b3d89

	// We're considering flipping the default value and have new users
	// start off with repository indicators disabled. As such we'll start
	// persisting the current default to localstorage right away so we
	// can change the default in the future without affecting current
	// users.
	if (getBoolean(repositoryIndicatorsEnabledKey) === undefined) {
	setBoolean(repositoryIndicatorsEnabledKey, true)
	}

with a default constant set to false. Then, when the accessibility refactor goes in, we simply delete those lines. This is probably the most straight forward solution.

But Markus also suggested that this may be a good time to implement versioning of our local storage schema -> a system that would allow us to update the defaults for only new users.

Something like:

if (getNumber('config-version', 0) < 1) {
  if (getBoolean(repositoryIndicatorsEnabledKey) === undefined) {
    setBoolean(repositoryIndicatorsEnabledKey, true)
  }
  setNumber('config-version', 1)
}

Then.. I went down a rabbit hole. 😛 I went to attempt to implement this versioning type of thing so I could see if it was simple enough to ask on top of the open PR or not and when writing documentation around realized I was referring desparate parts of the app-store file.... and then, I was like.. all these constants are a bit unwieldy, not consistent, and even this PR showed that locating all the different pieces of local storage/setting management wasn't straight forward. Seemed like it would be nice if we had this a little more structured and then the versioning would have a nice home. So... I dug into a localStorageManager thing that forces typing around each of the keys and their default values. It would automatically set default values for keys by just adding them to the key and default value types.

However, I only really updated the first key in alphabetical order of askToMoveToApplicationsFolderSetting to use this as it will be a little tedious to go through and replace all the existing use cases.

I really like the structure of this.. but before I go down this rabbit hole any further. Wanted to get your all thoughts.

Release notes

Notes: no-notes

[tidy-dev]

This is bad.. but didn't know how to properly do the typing stuff on this. unless I did a getBoolean, getNumber, getObject`... but, didn't want to explore more because I was just trying to do quick/dirty POC right now.

[sergiou87]

I think this cast should be safe but do we need generics in this function? Unless I'm missing something, I would define a type for the types of values that can be stored:

type LocalStorageValue = string | number | boolean | object | null

and then use that both in the map of default values:

const LocalStorageDefaults: Record<LocalStorageKey, LocalStorageValue> = { ... }

And in this function:

  public get(key: LocalStorageKey): LocalStorageValue {
    const value: LocalStorageValue | undefined = this.storage.get(key)

    if (value === undefined) {
      fatalError(`Unknown key: ${key}`)
    }

    if (typeof value !== typeof LocalStorageDefaults[key]) {
      fatalError(`Incorrect Type: ${key}`)
    }

    return value
  }

And in set (unless for some reason we can't set a value to null):

public set(key: LocalStorageKey, value: LocalStorageValue) { ... }

Does this make sense? 🤔

[sergiou87]

Ok, reading https://github.com/desktop/desktop/pull/17386/files#r1324939041 now I understand why you ask and why my approach wouldn't work either 😂

[sergiou87]

AFAIK you cannot narrow the type down more than you already do, because we would need something like:

  public get<T extends typeof LocalStorageDefaults[key]>(
    key: LocalStorageKey
  ): T {

Which is impossible because the typing part cannot use anything from the runtime part.

[tidy-dev]

This is highlighting my other comment that the casting of the type is bad. :D Because the image-diff-type is a object.. but this doesn't complain even tho askToMoveToApplicationsFolderSetting is a boolean.

[sergiou87]

In this case image-diff-type is a number, right? Hence why it works at runtime, because it's able to convert it to a boolean.

However, get is inferring T from the required return type, which is a boolean thanks to askToMoveToApplicationsFolderSetting, instead of inferring it from the type of the value for the given image-diff-type key, which I think can't be done 😞

[sergiou87]

This idea made the trick:

1. First remove the Record… typing from the defaults object:

const LocalStorageDefaults = { ... } as const

2. Make get generic parameter to infer the key type (which should be the string itself) and then use that to parameterize the return type:

  public get<
    V extends LocalStorageKey,
    T extends typeof LocalStorageDefaults[V]
  >(key: V): T {

The only issue I see with this is that for nullable values, you need to be explicit about the type in the defaults:

  'version-and-users-of-last-thank-you': null as object | null,

This is what this line looks like now:

Finally, in order to make sure LocalStorageDefaults is exhaustive and has all keys, we could do something like…

const TypeSafeLocalStorageDefaults: Record<
  LocalStorageKey,
  string | object | number | boolean | null
> = LocalStorageDefaults

Which should do the check for us at build time. A bit hacky, but sounds like the best option to me 🤔

[tidy-dev]

Highlights our inconsistency in the past of key naming 😄

[sergiou87]

I like the idea overall! Typing is hard, though, for this kind of generic things 😂 But we might be able to get a nice type-safe approach, and I'm sure @niik knows some magic to make it even better.

[niik]

I like the idea overall! Typing is hard, though, for this kind of generic things 😂 But we might be able to get a nice type-safe approach, and I'm sure @niik knows some magic to make it even better.

I'm sure we could figure out something nice and type safe, perhaps something like this.

I'm more interesting in taking a second look at the introduction of a class here which (at first glance to be sure) seems unnecessary and not idiomatic. LocalStorage is already a synchronous readily available map and I doubt the parsing overhead is enough to justify the added complexity of maintaining an in-memory map and passing a singleton around to the function that need to use config values.

I'm (not surprisingly) very much in favor of more strongly typing our config values and having a well defined mapping between keys and their intended types seems like a solid idea but I think we can achieve that without having to introduce a class or memory backed store.

[tidy-dev]

LocalStorage is already a synchronous readily available map and I doubt the parsing overhead is enough to justify the added complexity of maintaining an in-memory map and passing a singleton around to the function that need to use config values.

The reason I store it in an in memory map is we do so in the app-store except is not stored in a map it is just stored as variables on the app-store. I believe this is to prevent us from continually reading from the local storage every time we want to get the current app state?

This class is doing both this part - storing it in local storage and keeping in memory: