POC of local storage manager #17386
There are no files selected for viewing
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -1,4 +1,7 @@ | ||
| import { ImageDiffType } from '../models/diff' | ||
| import { parseEnumValue } from './enum' | ||
| import { fatalError } from './fatal-error' | ||
| import { ILastThankYou } from '../models/last-thank-you' | ||
|
|
||
| /** | ||
| * Returns the value for the provided key from local storage interpreted as a | ||
|
|
@@ -238,3 +241,212 @@ export function setObject(key: string, value: object) { | |
| const rawData = JSON.stringify(value) | ||
| localStorage.setItem(key, rawData) | ||
| } | ||
|
|
||
| const LocalStorageKeys = [ | ||
| 'askToMoveToApplicationsFolder', | ||
| 'commit-spellcheck-enabled', | ||
| 'commit-summary-width', | ||
| 'confirmCheckoutCommit', | ||
| 'confirmDiscardChanges', | ||
| 'confirmDiscardChangesPermanentlyKey', | ||
| 'confirmDiscardStash', | ||
| 'confirmForcePush', | ||
| 'confirmRepoRemoval', | ||
| 'confirmUndoCommit', | ||
| 'externalEditor', | ||
| 'hide-whitespace-in-changes-diff', | ||
| 'hide-whitespace-in-diff', | ||
| 'hide-whitespace-in-pull-request-diff', | ||
| 'image-diff-type', | ||
| 'last-selected-repository-id', | ||
| 'version-and-users-of-last-thank-you', | ||
| 'pull-request-files-width', | ||
| 'pull-request-suggested-next-action-key', | ||
| 'recently-selected-repositories', | ||
| 'enable-repository-indicators', | ||
| 'showCommitLengthWarning', | ||
| 'shell', | ||
| 'sidebar-width', | ||
| 'stashed-files-width', | ||
| 'uncommittedChangesStrategyKind', | ||
| ] as const | ||
|
|
||
| export type LocalStorageKey = typeof LocalStorageKeys[number] | ||
|
|
||
| const LocalStorageDefaults: Record< | ||
| LocalStorageKey, | ||
| boolean | number | object | null | ||
| > = { | ||
| askToMoveToApplicationsFolder: true, | ||
| 'commit-spellcheck-enabled': true, | ||
| 'commit-summary-width': 250, | ||
| confirmCheckoutCommit: true, | ||
| confirmDiscardChanges: true, | ||
| confirmDiscardChangesPermanentlyKey: true, | ||
| confirmDiscardStash: true, | ||
| 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, | ||
| 'version-and-users-of-last-thank-you': null, | ||
| 'pull-request-files-width': 250, | ||
| 'pull-request-suggested-next-action-key': 0, | ||
| 'recently-selected-repositories': 0, | ||
| 'enable-repository-indicators': 0, | ||
| showCommitLengthWarning: false, | ||
| shell: 0, | ||
| 'sidebar-width': 250, | ||
| 'stashed-files-width': 250, | ||
| uncommittedChangesStrategyKind: 0, | ||
| } | ||
|
|
||
| export class LocalStorageManager { | ||
| private readonly storage = new Map< | ||
| string, | ||
| boolean | string | object | number | null | ||
| >() | ||
|
|
||
| public constructor() { | ||
| this.intialize() | ||
| } | ||
|
|
||
| public get<T extends typeof LocalStorageDefaults[LocalStorageKey]>( | ||
| key: LocalStorageKey | ||
| ): T { | ||
| const value = this.storage.get(key) | ||
|
|
||
| if (value === undefined) { | ||
| fatalError(`Unknown key: ${key}`) | ||
| } | ||
|
|
||
| if (typeof value !== typeof LocalStorageDefaults[key]) { | ||
| fatalError(`Incorrect Type: ${key}`) | ||
| } | ||
|
|
||
| // Not sure if this casting is safe.. | ||
| return value as T | ||
|
Comment on lines
+331
to
+332
There was a problem hiding this comment. This is bad.. but didn't know how to properly do the typing stuff on this. unless I did a There was a problem hiding this comment. 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 | nulland 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 public set(key: LocalStorageKey, value: LocalStorageValue) { ... }Does this make sense? 🤔 There was a problem hiding this comment. 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. 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. |
||
| } | ||
|
|
||
| public set(key: LocalStorageKey, value: boolean | string | object | number) { | ||
| switch (typeof value) { | ||
| case 'boolean': | ||
| setBoolean(key, value) | ||
| break | ||
| case 'number': | ||
| setNumber(key, value) | ||
| break | ||
| case 'object': | ||
| setObject(key, value) | ||
| break | ||
| default: | ||
| fatalError(`Unexpected default value type: ${typeof value}`) | ||
| } | ||
|
|
||
| this.storage.set(key, value) | ||
| } | ||
|
|
||
| private intialize() { | ||
| this.applySchema() | ||
|
|
||
| LocalStorageKeys.forEach(key => { | ||
| this.initializeValue(key) | ||
| }) | ||
| } | ||
|
|
||
| /** | ||
| * We are implementing a versioning of our localStorage config so that we can | ||
| * migrate new users to new app defaults while keeping existing users on their | ||
| * current configuration. Starting the versioning at 1. | ||
| * | ||
| * Another way to say this, is this provides a way to override the default | ||
| * values. If a config is not set in this versioning, the user will receive the | ||
| * default values defined in `LocalStorageDefaults` | ||
| */ | ||
| private applySchema() { | ||
| const latestSchemaVersion = 1 | ||
|
|
||
| // If config version doesn't exist (new installation), set it to the latest. | ||
| if (getNumber('config-version') === undefined) { | ||
| setNumber('config-version', latestSchemaVersion) | ||
| } | ||
|
|
||
| const userConfig = getNumber('config-version', latestSchemaVersion) | ||
|
|
||
| if (userConfig <= 1) { | ||
| this.overrideValue('enable-repository-indicators', true) | ||
| this.overrideValue('showCommitLengthWarning', true) | ||
| } | ||
| } | ||
|
|
||
| private overrideValue( | ||
| key: LocalStorageKey, | ||
| value: boolean | string | object | number | ||
| ) { | ||
| switch (typeof value) { | ||
| case 'boolean': | ||
| if (getBoolean(key) === undefined) { | ||
| setBoolean(key, value) | ||
| } | ||
| break | ||
| case 'number': | ||
| if (getNumber(key) === undefined) { | ||
| setNumber(key, value) | ||
| } | ||
| break | ||
| case 'object': | ||
| if (getObject(key) === undefined) { | ||
| setObject(key, value) | ||
| } | ||
| break | ||
| default: | ||
| fatalError(`Unexpected default value type: ${typeof value}`) | ||
| } | ||
| } | ||
|
|
||
| private initializeValue(key: LocalStorageKey) { | ||
| const defaultValue: boolean | number | object | null = | ||
| LocalStorageDefaults[key] | ||
|
|
||
| switch (typeof defaultValue) { | ||
| case 'boolean': | ||
| const booleanValue = getBoolean(key, defaultValue) | ||
| this.storage.set(key, booleanValue) | ||
| break | ||
| case 'number': | ||
| const numberValue = getNumber(key, defaultValue) | ||
| this.storage.set(key, numberValue) | ||
| break | ||
| case 'object': | ||
| this.intializeObjectValue(key, defaultValue) | ||
| break | ||
| default: | ||
| fatalError(`Unexpected default value type: ${typeof defaultValue}`) | ||
| } | ||
| } | ||
|
|
||
| private intializeObjectValue( | ||
| key: LocalStorageKey, | ||
| defaultValue: object | null | ||
| ) { | ||
| if (key === 'image-diff-type') { | ||
| const imageDiffTypeStoredValue = localStorage.getItem(key) | ||
| const value = | ||
| imageDiffTypeStoredValue === null | ||
| ? defaultValue | ||
| : parseInt(imageDiffTypeStoredValue) | ||
| this.storage.set(key, value) | ||
| return | ||
| } | ||
|
|
||
| if (key === 'version-and-users-of-last-thank-you') { | ||
| const storedValue = getObject<ILastThankYou>(key) ?? null | ||
| this.storage.set(key, storedValue) | ||
| return | ||
| } | ||
| } | ||
| } | ||
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -228,6 +228,7 @@ import { | |
| getObject, | ||
| setObject, | ||
| getFloatNumber, | ||
| LocalStorageManager, | ||
| } from '../local-storage' | ||
| import { ExternalEditorError, suggestedExternalEditor } from '../editors/shared' | ||
| import { ApiRepositoriesStore } from './api-repositories-store' | ||
|
|
@@ -349,15 +350,13 @@ const stashedFilesWidthConfigKey: string = 'stashed-files-width' | |
| const defaultPullRequestFileListWidth: number = 250 | ||
| const pullRequestFileListConfigKey: string = 'pull-request-files-width' | ||
|
|
||
| const confirmRepoRemovalDefault: boolean = true | ||
| const confirmDiscardChangesDefault: boolean = true | ||
| const confirmDiscardChangesPermanentlyDefault: boolean = true | ||
| const confirmDiscardStashDefault: boolean = true | ||
| const confirmCheckoutCommitDefault: boolean = true | ||
| const askForConfirmationOnForcePushDefault = true | ||
| const confirmUndoCommitDefault: boolean = true | ||
| const confirmRepoRemovalKey: string = 'confirmRepoRemoval' | ||
| const confirmDiscardChangesKey: string = 'confirmDiscardChanges' | ||
| const confirmDiscardStashKey: string = 'confirmDiscardStash' | ||
|
|
@@ -465,8 +464,6 @@ export class AppStore extends TypedBaseStore<IAppState> { | |
| private isUpdateAvailableBannerVisible: boolean = false | ||
| private isUpdateShowcaseVisible: boolean = false | ||
|
|
||
| private askForConfirmationOnRepositoryRemoval: boolean = | ||
| confirmRepoRemovalDefault | ||
| private confirmDiscardChanges: boolean = confirmDiscardChangesDefault | ||
|
|
@@ -546,7 +543,8 @@ export class AppStore extends TypedBaseStore<IAppState> { | |
| private readonly pullRequestCoordinator: PullRequestCoordinator, | ||
| private readonly repositoryStateCache: RepositoryStateCache, | ||
| private readonly apiRepositoriesStore: ApiRepositoriesStore, | ||
| private readonly notificationsStore: NotificationsStore, | ||
| private readonly localStorageManager: LocalStorageManager | ||
| ) { | ||
| super() | ||
|
|
||
|
|
@@ -968,7 +966,7 @@ export class AppStore extends TypedBaseStore<IAppState> { | |
| isUpdateShowcaseVisible: this.isUpdateShowcaseVisible, | ||
| currentBanner: this.currentBanner, | ||
| askToMoveToApplicationsFolderSetting: | ||
| this.localStorageManager.get('image-diff-type'), | ||
|
There was a problem hiding this comment. This is highlighting my other comment that the casting of the type is bad. :D Because the There was a problem hiding this comment. In this case However, There was a problem hiding this comment. This idea made the trick:
const LocalStorageDefaults = { ... } as const
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 const TypeSafeLocalStorageDefaults: Record<
LocalStorageKey,
string | object | number | boolean | null
> = LocalStorageDefaultsWhich should do the check for us at build time. A bit hacky, but sounds like the best option to me 🤔 |
||
| askForConfirmationOnRepositoryRemoval: | ||
| this.askForConfirmationOnRepositoryRemoval, | ||
| askForConfirmationOnDiscardChanges: this.confirmDiscardChanges, | ||
|
|
@@ -2082,11 +2080,6 @@ export class AppStore extends TypedBaseStore<IAppState> { | |
| // TODO: Initiliaze here for now... maybe move to dialog mounting | ||
| this.updatePullRequestResizableConstraints() | ||
|
|
||
| this.askForConfirmationOnRepositoryRemoval = getBoolean( | ||
| confirmRepoRemovalKey, | ||
| confirmRepoRemovalDefault | ||
|
|
@@ -5403,11 +5396,8 @@ export class AppStore extends TypedBaseStore<IAppState> { | |
| public _setAskToMoveToApplicationsFolderSetting( | ||
| value: boolean | ||
| ): Promise<void> { | ||
| this.localStorageManager.set('askToMoveToApplicationsFolder', value) | ||
| this.emitUpdate() | ||
| return Promise.resolve() | ||
| } | ||
|
|
||
|
|
||
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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 😄