-
Notifications
You must be signed in to change notification settings - Fork 9.2k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
POC of local storage manager #17386
base: development
Are you sure you want to change the base?
POC of local storage manager #17386
Conversation
// Not sure if this casting is safe.. | ||
return value as T |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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? 🤔
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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 😂
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
@@ -968,7 +966,7 @@ export class AppStore extends TypedBaseStore<IAppState> { | |||
isUpdateShowcaseVisible: this.isUpdateShowcaseVisible, | |||
currentBanner: this.currentBanner, | |||
askToMoveToApplicationsFolderSetting: | |||
this.askToMoveToApplicationsFolderSetting, | |||
this.localStorageManager.get('image-diff-type'), |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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 😞
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This idea made the trick:
- First remove the
Record…
typing from the defaults object:
const LocalStorageDefaults = { ... } as const
- 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 🤔
confirmForcePush: true, | ||
confirmRepoRemoval: true, | ||
confirmUndoCommit: true, | ||
externalEditor: null, | ||
'hide-whitespace-in-changes-diff': false, | ||
'hide-whitespace-in-diff': false, | ||
'hide-whitespace-in-pull-request-diff': false, | ||
'image-diff-type': ImageDiffType.TwoUp, | ||
'last-selected-repository-id': 0, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Highlights our inconsistency in the past of key naming 😄
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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. |
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
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:
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