-
Notifications
You must be signed in to change notification settings - Fork 71
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
refactor: unify storage/providers (for further InMemory storage integration) [1/3] #475
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,16 @@ | ||
import NOOP from 'lodash/noop'; | ||
|
||
/** | ||
* This is used to keep multiple browser tabs in sync, therefore only needed on web | ||
* On native platforms, we omit this syncing logic by setting this to mock implementation. | ||
*/ | ||
const InstanceSync = { | ||
init: NOOP, | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Does this need to use lodash, or can it just be an empty arrow function like There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. @tgolen if I add empty functions I'm getting eslint errors: Do you think it would be better to specify our own function? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Hm, that's interesting. OK, it's probably fine to leave it as |
||
setItem: NOOP, | ||
removeItem: NOOP, | ||
removeItems: NOOP, | ||
mergeItem: NOOP, | ||
clear: <T extends () => void>(callback: T) => Promise.resolve(callback()), | ||
}; | ||
|
||
export default InstanceSync; |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,65 @@ | ||
/* eslint-disable no-invalid-this */ | ||
/** | ||
* The InstancesSync object provides data-changed events like the ones that exist | ||
* when using LocalStorage APIs in the browser. These events are great because multiple tabs can listen for when | ||
* data changes and then stay up-to-date with everything happening in Onyx. | ||
*/ | ||
import type {KeyList, Key, OnStorageKeyChanged, Value} from '../providers/types'; | ||
|
||
const SYNC_ONYX = 'SYNC_ONYX'; | ||
|
||
/** | ||
* Raise an event through `localStorage` to let other tabs know a value changed | ||
* @param {String} onyxKey | ||
*/ | ||
function raiseStorageSyncEvent(onyxKey: Key) { | ||
global.localStorage.setItem(SYNC_ONYX, onyxKey); | ||
global.localStorage.removeItem(SYNC_ONYX); | ||
} | ||
|
||
function raiseStorageSyncManyKeysEvent(onyxKeys: KeyList) { | ||
onyxKeys.forEach((onyxKey) => { | ||
raiseStorageSyncEvent(onyxKey); | ||
}); | ||
} | ||
|
||
const InstanceSync = { | ||
/** | ||
* @param {Function} onStorageKeyChanged Storage synchronization mechanism keeping all opened tabs in sync | ||
*/ | ||
init: (onStorageKeyChanged: OnStorageKeyChanged) => { | ||
// This listener will only be triggered by events coming from other tabs | ||
global.addEventListener('storage', (event) => { | ||
// Ignore events that don't originate from the SYNC_ONYX logic | ||
if (event.key !== SYNC_ONYX || !event.newValue) { | ||
return; | ||
} | ||
|
||
const onyxKey = event.newValue; | ||
// @ts-expect-error `this` will be substituted later in actual function call | ||
this.getItem(onyxKey).then((value: Value) => onStorageKeyChanged(onyxKey, value)); | ||
}); | ||
}, | ||
setItem: raiseStorageSyncEvent, | ||
removeItem: raiseStorageSyncEvent, | ||
removeItems: raiseStorageSyncManyKeysEvent, | ||
mergeItem: raiseStorageSyncEvent, | ||
clear: (clearImplementation: () => void) => { | ||
let allKeys: KeyList; | ||
|
||
// The keys must be retrieved before storage is cleared or else the list of keys would be empty | ||
// @ts-expect-error `this` will be substituted later in actual function call | ||
return this.getAllKeys() | ||
.then((keys: KeyList) => { | ||
allKeys = keys; | ||
}) | ||
.then(() => clearImplementation()) | ||
.then(() => { | ||
// Now that storage is cleared, the storage sync event can happen which is a more atomic action | ||
// for other browser tabs | ||
raiseStorageSyncManyKeysEvent(allKeys); | ||
}); | ||
}, | ||
}; | ||
|
||
export default InstanceSync; |
This file was deleted.
This file was deleted.
This file was deleted.
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -1,3 +1,137 @@ | ||
import WebStorage from './WebStorage'; | ||
import PlatformStorage from './platforms'; | ||
import InstanceSync from './InstanceSync'; | ||
import type StorageProvider from './providers/types'; | ||
|
||
export default WebStorage; | ||
const provider = PlatformStorage; | ||
let shouldKeepInstancesSync = false; | ||
|
||
type Storage = { | ||
getStorageProvider: () => StorageProvider; | ||
} & StorageProvider; | ||
|
||
const Storage: Storage = { | ||
/** | ||
* Returns the storage provider currently in use | ||
*/ | ||
getStorageProvider() { | ||
return provider; | ||
}, | ||
|
||
/** | ||
* Initializes all providers in the list of storage providers | ||
* and enables fallback providers if necessary | ||
*/ | ||
init() { | ||
provider.init(); | ||
}, | ||
|
||
/** | ||
* Get the value of a given key or return `null` if it's not available | ||
*/ | ||
getItem: (key) => provider.getItem(key), | ||
|
||
/** | ||
* Get multiple key-value pairs for the give array of keys in a batch | ||
*/ | ||
multiGet: (keys) => provider.multiGet(keys), | ||
|
||
/** | ||
* Sets the value for a given key. The only requirement is that the value should be serializable to JSON string | ||
*/ | ||
setItem: (key, value) => { | ||
const promise = provider.setItem(key, value); | ||
|
||
if (shouldKeepInstancesSync) { | ||
return promise.then(() => InstanceSync.setItem(key)); | ||
} | ||
|
||
return promise; | ||
}, | ||
|
||
/** | ||
* Stores multiple key-value pairs in a batch | ||
*/ | ||
multiSet: (pairs) => provider.multiSet(pairs), | ||
|
||
/** | ||
* Merging an existing value with a new one | ||
*/ | ||
mergeItem: (key, changes, modifiedData) => { | ||
const promise = provider.mergeItem(key, changes, modifiedData); | ||
|
||
if (shouldKeepInstancesSync) { | ||
return promise.then(() => InstanceSync.mergeItem(key)); | ||
} | ||
|
||
return promise; | ||
}, | ||
|
||
/** | ||
* Multiple merging of existing and new values in a batch | ||
* This function also removes all nested null values from an object. | ||
*/ | ||
multiMerge: (pairs) => provider.multiMerge(pairs), | ||
|
||
/** | ||
* Removes given key and its value | ||
*/ | ||
removeItem: (key) => { | ||
const promise = provider.removeItem(key); | ||
|
||
if (shouldKeepInstancesSync) { | ||
return promise.then(() => InstanceSync.removeItem(key)); | ||
} | ||
|
||
return promise; | ||
}, | ||
|
||
/** | ||
* Remove given keys and their values | ||
*/ | ||
removeItems: (keys) => { | ||
const promise = provider.removeItems(keys); | ||
|
||
if (shouldKeepInstancesSync) { | ||
return promise.then(() => InstanceSync.removeItems(keys)); | ||
} | ||
|
||
return promise; | ||
}, | ||
|
||
/** | ||
* Clears everything | ||
*/ | ||
clear: () => { | ||
if (shouldKeepInstancesSync) { | ||
return InstanceSync.clear(() => provider.clear()); | ||
} | ||
|
||
return provider.clear(); | ||
}, | ||
|
||
// This is a noop for now in order to keep clients from crashing see https://github.com/Expensify/Expensify/issues/312438 | ||
setMemoryOnlyKeys: () => provider.setMemoryOnlyKeys(), | ||
|
||
/** | ||
* Returns all available keys | ||
*/ | ||
getAllKeys: () => provider.getAllKeys(), | ||
|
||
/** | ||
* Gets the total bytes of the store | ||
*/ | ||
getDatabaseSize: () => provider.getDatabaseSize(), | ||
|
||
/** | ||
* @param onStorageKeyChanged - Storage synchronization mechanism keeping all opened tabs in sync (web only) | ||
*/ | ||
keepInstancesSync(onStorageKeyChanged) { | ||
// If InstanceSync is null, it means we're on a native platform and we don't need to keep instances in sync | ||
if (InstanceSync == null) return; | ||
|
||
shouldKeepInstancesSync = true; | ||
InstanceSync.init(onStorageKeyChanged); | ||
}, | ||
}; | ||
|
||
export default Storage; |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,3 @@ | ||
import NativeStorage from '../providers/SQLiteProvider'; | ||
|
||
export default NativeStorage; |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,3 @@ | ||
import WebStorage from '../providers/IDBKeyValProvider'; | ||
|
||
export default WebStorage; |
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.
Was there any specific reason to move this up here? I think it makes sense but I'm also surprised that it wasn't here to begin with.
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.
@tgolen it was already asked in #439 (comment)
And I agree, that it's better to keep initialization in a single place 👀