Skip to content

Commit c05c014

Browse files
marcaaronmarcaaron
authored
Merge pull request #588 from fabioh8010/bugfix/48725-v2
Fix problems related to Onyx.clear and useOnyx
2 parents 36ff87b + c71c1db commit c05c014

8 files changed

Lines changed: 224 additions & 36 deletions

File tree

‎lib/Onyx.ts‎

Lines changed: 5 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -2,7 +2,7 @@
22
import _ from 'underscore';
33
import lodashPick from 'lodash/pick';
44
import * as Logger from './Logger';
5-
import cache from './OnyxCache';
5+
import cache, {TASK} from './OnyxCache';
66
import * as PerformanceUtils from './PerformanceUtils';
77
import Storage from './storage';
88
import utils from './utils';
@@ -449,7 +449,7 @@ function clear(keysToPreserve: OnyxKey[] = []): Promise<void> {
449449
const defaultKeyStates = OnyxUtils.getDefaultKeyStates();
450450
const initialKeys = Object.keys(defaultKeyStates);
451451

452-
return OnyxUtils.getAllKeys()
452+
const promise = OnyxUtils.getAllKeys()
453453
.then((cachedKeys) => {
454454
cache.clearNullishStorageKeys();
455455

@@ -529,13 +529,16 @@ function clear(keysToPreserve: OnyxKey[] = []): Promise<void> {
529529
// Remove only the items that we want cleared from storage, and reset others to default
530530
keysToBeClearedFromStorage.forEach((key) => cache.drop(key));
531531
return Storage.removeItems(keysToBeClearedFromStorage)
532+
.then(() => connectionManager.refreshSessionID())
532533
.then(() => Storage.multiSet(defaultKeyValuePairs))
533534
.then(() => {
534535
DevTools.clearState(keysToPreserve);
535536
return Promise.all(updatePromises);
536537
});
537538
})
538539
.then(() => undefined);
540+
541+
return cache.captureTask(TASK.CLEAR, promise) as Promise<void>;
539542
}
540543

541544
function updateSnapshots(data: OnyxUpdate[]) {

‎lib/OnyxCache.ts‎

Lines changed: 15 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -1,8 +1,18 @@
11
import {deepEqual} from 'fast-equals';
22
import bindAll from 'lodash/bindAll';
3+
import type {ValueOf} from 'type-fest';
34
import utils from './utils';
45
import type {OnyxKey, OnyxValue} from './types';
56

7+
// Task constants
8+
const TASK = {
9+
GET: 'get',
10+
GET_ALL_KEYS: 'getAllKeys',
11+
CLEAR: 'clear',
12+
} as const;
13+
14+
type CacheTask = ValueOf<typeof TASK> | `${ValueOf<typeof TASK>}:${string}`;
15+
616
/**
717
* In memory cache providing data by reference
818
* Encapsulates Onyx cache related functionality
@@ -172,7 +182,7 @@ class OnyxCache {
172182
* Check whether the given task is already running
173183
* @param taskName - unique name given for the task
174184
*/
175-
hasPendingTask(taskName: string): boolean {
185+
hasPendingTask(taskName: CacheTask): boolean {
176186
return this.pendingPromises.get(taskName) !== undefined;
177187
}
178188

@@ -182,7 +192,7 @@ class OnyxCache {
182192
* provided from this function
183193
* @param taskName - unique name given for the task
184194
*/
185-
getTaskPromise(taskName: string): Promise<OnyxValue<OnyxKey> | OnyxKey[]> | undefined {
195+
getTaskPromise(taskName: CacheTask): Promise<OnyxValue<OnyxKey> | OnyxKey[]> | undefined {
186196
return this.pendingPromises.get(taskName);
187197
}
188198

@@ -191,7 +201,7 @@ class OnyxCache {
191201
* hook up to the promise if it's still pending
192202
* @param taskName - unique name for the task
193203
*/
194-
captureTask(taskName: string, promise: Promise<OnyxValue<OnyxKey>>): Promise<OnyxValue<OnyxKey>> {
204+
captureTask(taskName: CacheTask, promise: Promise<OnyxValue<OnyxKey>>): Promise<OnyxValue<OnyxKey>> {
195205
const returnPromise = promise.finally(() => {
196206
this.pendingPromises.delete(taskName);
197207
});
@@ -242,3 +252,5 @@ class OnyxCache {
242252
const instance = new OnyxCache();
243253

244254
export default instance;
255+
export {TASK};
256+
export type {CacheTask};

‎lib/OnyxConnectionManager.ts‎

Lines changed: 28 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -73,12 +73,28 @@ class OnyxConnectionManager {
7373
*/
7474
private lastCallbackID: number;
7575

76+
/**
77+
* Stores the last generated session ID for the connection manager. The current session ID
78+
* is appended to the connection IDs and it's used to create new different connections for the same key
79+
* when `refreshSessionID()` is called.
80+
*
81+
* When calling `Onyx.clear()` after a logout operation some connections might remain active as they
82+
* aren't tied to the React's lifecycle e.g. `Onyx.connect()` usage, causing infinite loading state issues to new `useOnyx()` subscribers
83+
* that are connecting to the same key as we didn't populate the cache again because we are still reusing such connections.
84+
*
85+
* To elimitate this problem, the session ID must be refreshed during the `Onyx.clear()` call (by using `refreshSessionID()`)
86+
* in order to create fresh connections when new subscribers connect to the same keys again, allowing them
87+
* to use the cache system correctly and avoid the mentioned issues in `useOnyx()`.
88+
*/
89+
private sessionID: string;
90+
7691
constructor() {
7792
this.connectionsMap = new Map();
7893
this.lastCallbackID = 0;
94+
this.sessionID = Str.guid();
7995

8096
// Binds all public methods to prevent problems with `this`.
81-
bindAll(this, 'generateConnectionID', 'fireCallbacks', 'connect', 'disconnect', 'disconnectAll', 'addToEvictionBlockList', 'removeFromEvictionBlockList');
97+
bindAll(this, 'generateConnectionID', 'fireCallbacks', 'connect', 'disconnect', 'disconnectAll', 'refreshSessionID', 'addToEvictionBlockList', 'removeFromEvictionBlockList');
8298
}
8399

84100
/**
@@ -89,7 +105,10 @@ class OnyxConnectionManager {
89105
*/
90106
private generateConnectionID<TKey extends OnyxKey>(connectOptions: ConnectOptions<TKey>): string {
91107
const {key, initWithStoredValues, reuseConnection, waitForCollectionCallback} = connectOptions;
92-
let suffix = '';
108+
109+
// The current session ID is appended to the connection ID so we can have different connections
110+
// after an `Onyx.clear()` operation.
111+
let suffix = `,sessionID=${this.sessionID}`;
93112

94113
// We will generate a unique ID in any of the following situations:
95114
// - `reuseConnection` is `false`. That means the subscriber explicitly wants the connection to not be reused.
@@ -229,6 +248,13 @@ class OnyxConnectionManager {
229248
this.connectionsMap.clear();
230249
}
231250

251+
/**
252+
* Refreshes the connection manager's session ID.
253+
*/
254+
refreshSessionID(): void {
255+
this.sessionID = Str.guid();
256+
}
257+
232258
/**
233259
* Adds the connection to the eviction block list. Connections added to this list can never be evicted.
234260
* */

‎lib/OnyxUtils.ts‎

Lines changed: 6 additions & 8 deletions
Original file line numberDiff line numberDiff line change
@@ -6,7 +6,7 @@ import type {ValueOf} from 'type-fest';
66
import DevTools from './DevTools';
77
import * as Logger from './Logger';
88
import type Onyx from './Onyx';
9-
import cache from './OnyxCache';
9+
import cache, {TASK} from './OnyxCache';
1010
import * as PerformanceUtils from './PerformanceUtils';
1111
import * as Str from './Str';
1212
import unstable_batchedUpdates from './batch';
@@ -244,7 +244,7 @@ function get<TKey extends OnyxKey, TValue extends OnyxValue<TKey>>(key: TKey): P
244244
return Promise.resolve(cache.get(key) as TValue);
245245
}
246246

247-
const taskName = `get:${key}`;
247+
const taskName = `${TASK.GET}:${key}` as const;
248248

249249
// When a value retrieving task for this key is still running hook to it
250250
if (cache.hasPendingTask(taskName)) {
@@ -296,7 +296,7 @@ function multiGet<TKey extends OnyxKey>(keys: CollectionKeyBase[]): Promise<Map<
296296
return;
297297
}
298298

299-
const pendingKey = `get:${key}`;
299+
const pendingKey = `${TASK.GET}:${key}` as const;
300300
if (cache.hasPendingTask(pendingKey)) {
301301
pendingTasks.push(cache.getTaskPromise(pendingKey) as Promise<OnyxValue<TKey>>);
302302
pendingKeys.push(key);
@@ -378,11 +378,9 @@ function getAllKeys(): Promise<Set<OnyxKey>> {
378378
return Promise.resolve(cachedKeys);
379379
}
380380

381-
const taskName = 'getAllKeys';
382-
383381
// When a value retrieving task for all keys is still running hook to it
384-
if (cache.hasPendingTask(taskName)) {
385-
return cache.getTaskPromise(taskName) as Promise<Set<OnyxKey>>;
382+
if (cache.hasPendingTask(TASK.GET_ALL_KEYS)) {
383+
return cache.getTaskPromise(TASK.GET_ALL_KEYS) as Promise<Set<OnyxKey>>;
386384
}
387385

388386
// Otherwise retrieve the keys from storage and capture a promise to aid concurrent usages
@@ -393,7 +391,7 @@ function getAllKeys(): Promise<Set<OnyxKey>> {
393391
return cache.getAllKeys();
394392
});
395393

396-
return cache.captureTask(taskName, promise) as Promise<Set<OnyxKey>>;
394+
return cache.captureTask(TASK.GET_ALL_KEYS, promise) as Promise<Set<OnyxKey>>;
397395
}
398396

399397
/**

‎lib/useOnyx.ts‎

Lines changed: 17 additions & 7 deletions
Original file line numberDiff line numberDiff line change
@@ -1,6 +1,6 @@
11
import {deepEqual, shallowEqual} from 'fast-equals';
22
import {useCallback, useEffect, useRef, useSyncExternalStore} from 'react';
3-
import OnyxCache from './OnyxCache';
3+
import OnyxCache, {TASK} from './OnyxCache';
44
import type {Connection} from './OnyxConnectionManager';
55
import connectionManager from './OnyxConnectionManager';
66
import OnyxUtils from './OnyxUtils';
@@ -23,6 +23,12 @@ type BaseUseOnyxOptions = {
2323
* If set to `true`, data will be retrieved from cache during the first render even if there is a pending merge for the key.
2424
*/
2525
allowStaleData?: boolean;
26+
27+
/**
28+
* If set to `false`, the connection won't be reused between other subscribers that are listening to the same Onyx key
29+
* with the same connect configurations.
30+
*/
31+
reuseConnection?: boolean;
2632
};
2733

2834
type UseOnyxInitialValueOption<TInitialValue> = {
@@ -230,11 +236,14 @@ function useOnyx<TKey extends OnyxKey, TReturnValue = OnyxValue<TKey>>(key: TKey
230236
areValuesEqual = shallowEqual(previousValueRef.current ?? undefined, newValueRef.current);
231237
}
232238

233-
// If the previously cached value is different from the new value, we update both cached value
234-
// and the result to be returned by the hook.
235-
// If the cache was set for the first time, we also update the cached value and the result.
236-
const isCacheSetFirstTime = previousValueRef.current === null && hasCacheForKey;
237-
if (isCacheSetFirstTime || !areValuesEqual) {
239+
// We updated the cached value and the result in the following conditions:
240+
// We will update the cached value and the result in any of the following situations:
241+
// - The previously cached value is different from the new value.
242+
// - The previously cached value is `null` (not set from cache yet) and we have cache for this key
243+
// OR we have a pending `Onyx.clear()` task (if `Onyx.clear()` is running cache might not be available anymore
244+
// so we update the cached value/result right away in order to prevent infinite loading state issues).
245+
const shouldUpdateResult = !areValuesEqual || (previousValueRef.current === null && (hasCacheForKey || OnyxCache.hasPendingTask(TASK.CLEAR)));
246+
if (shouldUpdateResult) {
238247
previousValueRef.current = newValueRef.current;
239248

240249
// If the new value is `null` we default it to `undefined` to ensure the consumer gets a consistent result from the hook.
@@ -261,6 +270,7 @@ function useOnyx<TKey extends OnyxKey, TReturnValue = OnyxValue<TKey>>(key: TKey
261270
},
262271
initWithStoredValues: options?.initWithStoredValues,
263272
waitForCollectionCallback: OnyxUtils.isCollectionKey(key) as true,
273+
reuseConnection: options?.reuseConnection,
264274
});
265275

266276
checkEvictableKey();
@@ -274,7 +284,7 @@ function useOnyx<TKey extends OnyxKey, TReturnValue = OnyxValue<TKey>>(key: TKey
274284
isFirstConnectionRef.current = false;
275285
};
276286
},
277-
[key, options?.initWithStoredValues, checkEvictableKey],
287+
[key, options?.initWithStoredValues, options?.reuseConnection, checkEvictableKey],
278288
);
279289

280290
useEffect(() => {

‎tests/unit/OnyxConnectionManagerTest.ts‎

Lines changed: 86 additions & 7 deletions
Original file line numberDiff line numberDiff line change
@@ -10,12 +10,14 @@ import type {WithOnyxInstance} from '../../lib/withOnyx/types';
1010
import type GenericCollection from '../utils/GenericCollection';
1111
import waitForPromisesToResolve from '../utils/waitForPromisesToResolve';
1212

13-
// We need access to `connectionsMap` and `generateConnectionID` during the tests but the properties are private,
13+
// We need access to some internal properties of `connectionManager` during the tests but they are private,
1414
// so this workaround allows us to have access to them.
1515
// eslint-disable-next-line dot-notation
1616
const connectionsMap = connectionManager['connectionsMap'];
1717
// eslint-disable-next-line dot-notation
1818
const generateConnectionID = connectionManager['generateConnectionID'];
19+
// eslint-disable-next-line dot-notation
20+
const getSessionID = () => connectionManager['sessionID'];
1921

2022
function generateEmptyWithOnyxInstance() {
2123
return new (class {
@@ -51,31 +53,39 @@ describe('OnyxConnectionManager', () => {
5153
describe('generateConnectionID', () => {
5254
it('should generate a stable connection ID', async () => {
5355
const connectionID = generateConnectionID({key: ONYXKEYS.TEST_KEY});
54-
expect(connectionID).toEqual(`onyxKey=${ONYXKEYS.TEST_KEY},initWithStoredValues=true,waitForCollectionCallback=false`);
56+
expect(connectionID).toEqual(`onyxKey=${ONYXKEYS.TEST_KEY},initWithStoredValues=true,waitForCollectionCallback=false,sessionID=${getSessionID()}`);
5557
});
5658

5759
it("should generate a stable connection ID regardless of the order which the option's properties were passed", async () => {
5860
const connectionID = generateConnectionID({key: ONYXKEYS.TEST_KEY, waitForCollectionCallback: true, initWithStoredValues: true});
59-
expect(connectionID).toEqual(`onyxKey=${ONYXKEYS.TEST_KEY},initWithStoredValues=true,waitForCollectionCallback=true`);
61+
expect(connectionID).toEqual(`onyxKey=${ONYXKEYS.TEST_KEY},initWithStoredValues=true,waitForCollectionCallback=true,sessionID=${getSessionID()}`);
6062
});
6163

6264
it('should generate unique connection IDs if certain options are passed', async () => {
6365
const connectionID1 = generateConnectionID({key: ONYXKEYS.TEST_KEY, reuseConnection: false});
6466
const connectionID2 = generateConnectionID({key: ONYXKEYS.TEST_KEY, reuseConnection: false});
65-
expect(connectionID1.startsWith(`onyxKey=${ONYXKEYS.TEST_KEY},initWithStoredValues=true,waitForCollectionCallback=false,uniqueID=`)).toBeTruthy();
66-
expect(connectionID2.startsWith(`onyxKey=${ONYXKEYS.TEST_KEY},initWithStoredValues=true,waitForCollectionCallback=false,uniqueID=`)).toBeTruthy();
67+
expect(connectionID1.startsWith(`onyxKey=${ONYXKEYS.TEST_KEY},initWithStoredValues=true,waitForCollectionCallback=false,sessionID=${getSessionID()},uniqueID=`)).toBeTruthy();
68+
expect(connectionID2.startsWith(`onyxKey=${ONYXKEYS.TEST_KEY},initWithStoredValues=true,waitForCollectionCallback=false,sessionID=${getSessionID()},uniqueID=`)).toBeTruthy();
6769
expect(connectionID1).not.toEqual(connectionID2);
6870

6971
const connectionID3 = generateConnectionID({key: ONYXKEYS.TEST_KEY, initWithStoredValues: false});
70-
expect(connectionID3.startsWith(`onyxKey=${ONYXKEYS.TEST_KEY},initWithStoredValues=false,waitForCollectionCallback=false,uniqueID=`)).toBeTruthy();
72+
expect(connectionID3.startsWith(`onyxKey=${ONYXKEYS.TEST_KEY},initWithStoredValues=false,waitForCollectionCallback=false,sessionID=${getSessionID()},uniqueID=`)).toBeTruthy();
7173

7274
const connectionID4 = generateConnectionID({
7375
key: ONYXKEYS.TEST_KEY,
7476
displayName: 'Component1',
7577
statePropertyName: 'prop1',
7678
withOnyxInstance: generateEmptyWithOnyxInstance(),
7779
} as WithOnyxConnectOptions<OnyxKey>);
78-
expect(connectionID4.startsWith(`onyxKey=${ONYXKEYS.TEST_KEY},initWithStoredValues=true,waitForCollectionCallback=false,uniqueID=`)).toBeTruthy();
80+
expect(connectionID4.startsWith(`onyxKey=${ONYXKEYS.TEST_KEY},initWithStoredValues=true,waitForCollectionCallback=false,sessionID=${getSessionID()},uniqueID=`)).toBeTruthy();
81+
});
82+
83+
it('should generate an unique connection ID if the session ID is changed', async () => {
84+
const connectionID1 = generateConnectionID({key: ONYXKEYS.TEST_KEY});
85+
connectionManager.refreshSessionID();
86+
const connectionID2 = generateConnectionID({key: ONYXKEYS.TEST_KEY});
87+
88+
expect(connectionID1).not.toEqual(connectionID2);
7989
});
8090
});
8191

@@ -320,6 +330,56 @@ describe('OnyxConnectionManager', () => {
320330
}).not.toThrow();
321331
});
322332

333+
it('should create a separate connection for the same key after a Onyx.clear() call', async () => {
334+
await StorageMock.setItem(ONYXKEYS.TEST_KEY, 'test');
335+
336+
const callback1 = jest.fn();
337+
connectionManager.connect({key: ONYXKEYS.TEST_KEY, callback: callback1});
338+
expect(connectionsMap.size).toEqual(1);
339+
340+
await act(async () => waitForPromisesToResolve());
341+
342+
expect(callback1).toHaveBeenCalledTimes(1);
343+
expect(callback1).toHaveBeenCalledWith('test', ONYXKEYS.TEST_KEY);
344+
callback1.mockReset();
345+
346+
await act(async () => Onyx.clear());
347+
348+
expect(callback1).toHaveBeenCalledTimes(1);
349+
expect(callback1).toHaveBeenCalledWith(undefined, ONYXKEYS.TEST_KEY);
350+
callback1.mockReset();
351+
352+
const callback2 = jest.fn();
353+
connectionManager.connect({key: ONYXKEYS.TEST_KEY, callback: callback2});
354+
355+
const callback3 = jest.fn();
356+
connectionManager.connect({key: ONYXKEYS.TEST_KEY, callback: callback3});
357+
358+
// We expect to have two connections for ONYXKEYS.TEST_KEY, one for the first subscription before Onyx.clear(),
359+
// and the other for the two subscriptions with the same key after Onyx.clear().
360+
expect(connectionsMap.size).toEqual(2);
361+
362+
await act(async () => waitForPromisesToResolve());
363+
364+
expect(callback2).toHaveBeenCalledTimes(1);
365+
expect(callback2).toHaveBeenCalledWith(undefined, undefined);
366+
expect(callback3).toHaveBeenCalledTimes(1);
367+
expect(callback3).toHaveBeenCalledWith(undefined, undefined);
368+
callback1.mockReset();
369+
callback2.mockReset();
370+
callback3.mockReset();
371+
372+
Onyx.merge(ONYXKEYS.TEST_KEY, 'test2');
373+
await act(async () => waitForPromisesToResolve());
374+
375+
expect(callback1).toHaveBeenCalledTimes(1);
376+
expect(callback1).toHaveBeenCalledWith('test2', ONYXKEYS.TEST_KEY);
377+
expect(callback2).toHaveBeenCalledTimes(1);
378+
expect(callback2).toHaveBeenCalledWith('test2', ONYXKEYS.TEST_KEY);
379+
expect(callback3).toHaveBeenCalledTimes(1);
380+
expect(callback3).toHaveBeenCalledWith('test2', ONYXKEYS.TEST_KEY);
381+
});
382+
323383
describe('withOnyx', () => {
324384
it('should connect to a key two times with withOnyx and create two connections instead of one', async () => {
325385
await StorageMock.setItem(ONYXKEYS.TEST_KEY, 'test');
@@ -406,6 +466,25 @@ describe('OnyxConnectionManager', () => {
406466
});
407467
});
408468

469+
describe('refreshSessionID', () => {
470+
it('should create a separate connection for the same key if the session ID changes', async () => {
471+
await StorageMock.setItem(ONYXKEYS.TEST_KEY, 'test');
472+
await StorageMock.setItem(ONYXKEYS.TEST_KEY_2, 'test2');
473+
474+
const connection1 = connectionManager.connect({key: ONYXKEYS.TEST_KEY, callback: jest.fn()});
475+
476+
expect(connectionsMap.size).toEqual(1);
477+
478+
connectionManager.refreshSessionID();
479+
480+
const connection2 = connectionManager.connect({key: ONYXKEYS.TEST_KEY, callback: jest.fn()});
481+
482+
expect(connectionsMap.size).toEqual(2);
483+
expect(connectionsMap.has(connection1.id)).toBeTruthy();
484+
expect(connectionsMap.has(connection2.id)).toBeTruthy();
485+
});
486+
});
487+
409488
describe('addToEvictionBlockList / removeFromEvictionBlockList', () => {
410489
it('should add and remove connections from the eviction block list correctly', async () => {
411490
const evictionBlocklist = OnyxUtils.getEvictionBlocklist();

0 commit comments

Comments
 (0)