Skip to content
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

Merged
merged 4 commits into from
Feb 29, 2024
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
6 changes: 3 additions & 3 deletions jestSetup.js
Original file line number Diff line number Diff line change
@@ -1,7 +1,7 @@
jest.mock('./lib/storage');
jest.mock('./lib/storage/NativeStorage', () => require('./lib/storage/__mocks__'));
jest.mock('./lib/storage/WebStorage', () => require('./lib/storage/__mocks__'));
jest.mock('./lib/storage/providers/IDBKeyVal', () => require('./lib/storage/__mocks__'));
jest.mock('./lib/storage/platforms/index.native', () => require('./lib/storage/__mocks__'));
jest.mock('./lib/storage/platforms/index', () => require('./lib/storage/__mocks__'));
jest.mock('./lib/storage/providers/IDBKeyValProvider', () => require('./lib/storage/__mocks__'));

jest.mock('react-native-device-info', () => ({getFreeDiskStorage: () => {}}));
jest.mock('react-native-quick-sqlite', () => ({
Expand Down
18 changes: 10 additions & 8 deletions lib/Onyx.js
Original file line number Diff line number Diff line change
Expand Up @@ -1563,6 +1563,16 @@ function init({
shouldSyncMultipleInstances = Boolean(global.localStorage),
debugSetState = false,
} = {}) {
Storage.init();

if (shouldSyncMultipleInstances) {
Copy link
Collaborator

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.

Copy link
Contributor Author

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 👀

Storage.keepInstancesSync((key, value) => {
const prevValue = cache.getValue(key, false);
cache.set(key, value);
keyChanged(key, value, prevValue);
});
}

if (captureMetrics) {
// The code here is only bundled and applied when the captureMetrics is set
// eslint-disable-next-line no-use-before-define
Expand Down Expand Up @@ -1599,14 +1609,6 @@ function init({

// Initialize all of our keys with data provided then give green light to any pending connections
Promise.all([addAllSafeEvictionKeysToRecentlyAccessedList(), initializeWithDefaultKeyStates()]).then(deferredInitTask.resolve);

if (shouldSyncMultipleInstances && _.isFunction(Storage.keepInstancesSync)) {
Storage.keepInstancesSync((key, value) => {
const prevValue = cache.getValue(key, false);
cache.set(key, value);
keyChanged(key, value, prevValue);
});
}
}

const Onyx = {
Expand Down
16 changes: 16 additions & 0 deletions lib/storage/InstanceSync/index.ts
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,
Copy link
Collaborator

Choose a reason for hiding this comment

The 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 init: () => {}?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@tgolen if I add empty functions I'm getting eslint errors:

image

Do you think it would be better to specify our own function?

Copy link
Collaborator

Choose a reason for hiding this comment

The 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 NOOP for now. Thanks for trying!

setItem: NOOP,
removeItem: NOOP,
removeItems: NOOP,
mergeItem: NOOP,
clear: <T extends () => void>(callback: T) => Promise.resolve(callback()),
};

export default InstanceSync;
65 changes: 65 additions & 0 deletions lib/storage/InstanceSync/index.web.ts
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;
3 changes: 0 additions & 3 deletions lib/storage/NativeStorage.ts

This file was deleted.

73 changes: 0 additions & 73 deletions lib/storage/WebStorage.ts

This file was deleted.

4 changes: 4 additions & 0 deletions lib/storage/__mocks__/index.ts
Original file line number Diff line number Diff line change
Expand Up @@ -10,6 +10,7 @@ const set = jest.fn((key, value) => {
});

const idbKeyvalMock: StorageProvider = {
init: () => undefined,
setItem(key, value) {
return set(key, value);
},
Expand Down Expand Up @@ -60,10 +61,12 @@ const idbKeyvalMock: StorageProvider = {
},
// eslint-disable-next-line @typescript-eslint/no-empty-function
setMemoryOnlyKeys() {},
keepInstancesSync: () => undefined,
};

const idbKeyvalMockSpy = {
idbKeyvalSet: set,
init: jest.fn(idbKeyvalMock.init),
setItem: jest.fn(idbKeyvalMock.setItem),
getItem: jest.fn(idbKeyvalMock.getItem),
removeItem: jest.fn(idbKeyvalMock.removeItem),
Expand All @@ -80,6 +83,7 @@ const idbKeyvalMockSpy = {
}),
getDatabaseSize: jest.fn(idbKeyvalMock.getDatabaseSize),
setMemoryOnlyKeys: jest.fn(idbKeyvalMock.setMemoryOnlyKeys),
keepInstancesSync: jest.fn(idbKeyvalMock.keepInstancesSync),
};

export default idbKeyvalMockSpy;
3 changes: 0 additions & 3 deletions lib/storage/index.native.ts

This file was deleted.

138 changes: 136 additions & 2 deletions lib/storage/index.ts
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;
3 changes: 3 additions & 0 deletions lib/storage/platforms/index.native.ts
Original file line number Diff line number Diff line change
@@ -0,0 +1,3 @@
import NativeStorage from '../providers/SQLiteProvider';

export default NativeStorage;
3 changes: 3 additions & 0 deletions lib/storage/platforms/index.ts
Original file line number Diff line number Diff line change
@@ -0,0 +1,3 @@
import WebStorage from '../providers/IDBKeyValProvider';

export default WebStorage;
Loading
Loading