From 4be1da18c8b0e1d88c6317649183acbd1630d356 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?F=C3=A1bio=20Henriques?= Date: Mon, 23 Sep 2024 19:32:45 +0100 Subject: [PATCH 1/2] Disable connection reuse for collection keys with waitForCollectionCallback set to undefined/false --- lib/OnyxConnectionManager.ts | 19 +++++++++---- tests/unit/OnyxConnectionManagerTest.ts | 37 +++++++++++++++++++++++++ 2 files changed, 50 insertions(+), 6 deletions(-) diff --git a/lib/OnyxConnectionManager.ts b/lib/OnyxConnectionManager.ts index d73cb30fb..3e762ab3e 100644 --- a/lib/OnyxConnectionManager.ts +++ b/lib/OnyxConnectionManager.ts @@ -88,19 +88,26 @@ class OnyxConnectionManager { * according to their purpose and effect they produce in the Onyx connection. */ private generateConnectionID(connectOptions: ConnectOptions): string { + const {key, initWithStoredValues, reuseConnection, waitForCollectionCallback} = connectOptions; + let suffix = ''; // We will generate a unique ID in any of the following situations: - // - `connectOptions.reuseConnection` is `false`. That means the subscriber explicitly wants the connection to not be reused. - // - `connectOptions.initWithStoredValues` is `false`. This flag changes the subscription flow when set to `false`, so the connection can't be reused. + // - `reuseConnection` is `false`. That means the subscriber explicitly wants the connection to not be reused. + // - `initWithStoredValues` is `false`. This flag changes the subscription flow when set to `false`, so the connection can't be reused. + // - `key` is a collection key` AND `waitForCollectionCallback` is `undefined/false`. This combination needs a new connection at every subscribe + // in order to send all the collection entries, so the connection can't be reused. // - `withOnyxInstance` is defined inside `connectOptions`. That means the subscriber is a `withOnyx` HOC and therefore doesn't support connection reuse. - if (connectOptions.reuseConnection === false || connectOptions.initWithStoredValues === false || utils.hasWithOnyxInstance(connectOptions)) { + if ( + reuseConnection === false || + initWithStoredValues === false || + (!utils.hasWithOnyxInstance(connectOptions) && OnyxUtils.isCollectionKey(key) && (waitForCollectionCallback === undefined || waitForCollectionCallback === false)) || + utils.hasWithOnyxInstance(connectOptions) + ) { suffix += `,uniqueID=${Str.guid()}`; } - return `onyxKey=${connectOptions.key},initWithStoredValues=${connectOptions.initWithStoredValues ?? true},waitForCollectionCallback=${ - connectOptions.waitForCollectionCallback ?? false - }${suffix}`; + return `onyxKey=${connectOptions.key},initWithStoredValues=${initWithStoredValues ?? true},waitForCollectionCallback=${waitForCollectionCallback ?? false}${suffix}`; } /** diff --git a/tests/unit/OnyxConnectionManagerTest.ts b/tests/unit/OnyxConnectionManagerTest.ts index d6d4e26b4..ff7316ae0 100644 --- a/tests/unit/OnyxConnectionManagerTest.ts +++ b/tests/unit/OnyxConnectionManagerTest.ts @@ -271,6 +271,43 @@ describe('OnyxConnectionManager', () => { expect(connectionsMap.size).toEqual(0); }); + it("should create a separate connection to the same key when it's a collection one and waitForCollectionCallback is undefined/false", async () => { + const collection = { + [`${ONYXKEYS.COLLECTION.TEST_KEY}entry1`]: {id: 'entry1_id', name: 'entry1_name'}, + [`${ONYXKEYS.COLLECTION.TEST_KEY}entry2`]: {id: 'entry2_id', name: 'entry2_name'}, + [`${ONYXKEYS.COLLECTION.TEST_KEY}entry3`]: {id: 'entry3_id', name: 'entry3_name'}, + }; + + Onyx.mergeCollection(ONYXKEYS.COLLECTION.TEST_KEY, collection as GenericCollection); + + await act(async () => waitForPromisesToResolve()); + + const callback1 = jest.fn(); + const connection1 = connectionManager.connect({key: ONYXKEYS.COLLECTION.TEST_KEY, waitForCollectionCallback: undefined, callback: callback1}); + + await act(async () => waitForPromisesToResolve()); + + expect(callback1).toHaveBeenCalledTimes(3); + expect(callback1).toHaveBeenNthCalledWith(1, collection[`${ONYXKEYS.COLLECTION.TEST_KEY}entry1`], `${ONYXKEYS.COLLECTION.TEST_KEY}entry1`); + expect(callback1).toHaveBeenNthCalledWith(2, collection[`${ONYXKEYS.COLLECTION.TEST_KEY}entry2`], `${ONYXKEYS.COLLECTION.TEST_KEY}entry2`); + expect(callback1).toHaveBeenNthCalledWith(3, collection[`${ONYXKEYS.COLLECTION.TEST_KEY}entry3`], `${ONYXKEYS.COLLECTION.TEST_KEY}entry3`); + + const callback2 = jest.fn(); + const connection2 = connectionManager.connect({key: ONYXKEYS.COLLECTION.TEST_KEY, waitForCollectionCallback: false, callback: callback2}); + + expect(connection1.id).not.toEqual(connection2.id); + expect(connectionsMap.size).toEqual(2); + expect(connectionsMap.has(connection1.id)).toBeTruthy(); + expect(connectionsMap.has(connection2.id)).toBeTruthy(); + + await act(async () => waitForPromisesToResolve()); + + expect(callback2).toHaveBeenCalledTimes(3); + expect(callback2).toHaveBeenNthCalledWith(1, collection[`${ONYXKEYS.COLLECTION.TEST_KEY}entry1`], `${ONYXKEYS.COLLECTION.TEST_KEY}entry1`); + expect(callback2).toHaveBeenNthCalledWith(2, collection[`${ONYXKEYS.COLLECTION.TEST_KEY}entry2`], `${ONYXKEYS.COLLECTION.TEST_KEY}entry2`); + expect(callback2).toHaveBeenNthCalledWith(3, collection[`${ONYXKEYS.COLLECTION.TEST_KEY}entry3`], `${ONYXKEYS.COLLECTION.TEST_KEY}entry3`); + }); + it('should not throw any errors when passing an undefined connection or trying to access an inexistent one inside disconnect()', () => { expect(connectionsMap.size).toEqual(0); From 58ebfd907bdd040291899e2707891c943b91264c Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?F=C3=A1bio=20Henriques?= Date: Thu, 26 Sep 2024 15:49:42 +0100 Subject: [PATCH 2/2] Address review comments --- lib/OnyxConnectionManager.ts | 5 ++--- 1 file changed, 2 insertions(+), 3 deletions(-) diff --git a/lib/OnyxConnectionManager.ts b/lib/OnyxConnectionManager.ts index 3e762ab3e..0258af0e5 100644 --- a/lib/OnyxConnectionManager.ts +++ b/lib/OnyxConnectionManager.ts @@ -89,13 +89,12 @@ class OnyxConnectionManager { */ private generateConnectionID(connectOptions: ConnectOptions): string { const {key, initWithStoredValues, reuseConnection, waitForCollectionCallback} = connectOptions; - let suffix = ''; // We will generate a unique ID in any of the following situations: // - `reuseConnection` is `false`. That means the subscriber explicitly wants the connection to not be reused. // - `initWithStoredValues` is `false`. This flag changes the subscription flow when set to `false`, so the connection can't be reused. - // - `key` is a collection key` AND `waitForCollectionCallback` is `undefined/false`. This combination needs a new connection at every subscribe + // - `key` is a collection key AND `waitForCollectionCallback` is `undefined/false`. This combination needs a new connection at every subscription // in order to send all the collection entries, so the connection can't be reused. // - `withOnyxInstance` is defined inside `connectOptions`. That means the subscriber is a `withOnyx` HOC and therefore doesn't support connection reuse. if ( @@ -107,7 +106,7 @@ class OnyxConnectionManager { suffix += `,uniqueID=${Str.guid()}`; } - return `onyxKey=${connectOptions.key},initWithStoredValues=${initWithStoredValues ?? true},waitForCollectionCallback=${waitForCollectionCallback ?? false}${suffix}`; + return `onyxKey=${key},initWithStoredValues=${initWithStoredValues ?? true},waitForCollectionCallback=${waitForCollectionCallback ?? false}${suffix}`; } /**