Skip to content

Commit df65c33

Browse files
fabioh8010fabioh8010
committed
Simplify and extract applyMerge() batching merges logic to a separate batchMergeChanges() function
1 parent befaaeb commit df65c33

3 files changed

Lines changed: 104 additions & 87 deletions

File tree

‎lib/Onyx.ts‎

Lines changed: 5 additions & 8 deletions
Original file line numberDiff line numberDiff line change
@@ -307,10 +307,7 @@ function merge<TKey extends OnyxKey>(key: TKey, changes: OnyxMergeInput<TKey>):
307307
}
308308

309309
try {
310-
// We first only merge the changes, so we can use our custom merging strategy by signaling OnyxUtils.applyMerge()
311-
// that we are batching merge changes.
312-
// We don't want to remove null values from the "batchedDeltaChanges" at the moment, this process will be done when merging
313-
// the batched changes to the existing value.
310+
// We first only merge the changes, so we use OnyxUtils.batchMergeChanges() to combine all the changes into just one.
314311
const validChanges = mergeQueue[key].filter((change) => {
315312
const {isCompatible, existingValueType, newValueType} = utils.checkCompatibilityWithExistingValue(change, existingValue);
316313
if (!isCompatible) {
@@ -322,7 +319,7 @@ function merge<TKey extends OnyxKey>(key: TKey, changes: OnyxMergeInput<TKey>):
322319
if (!validChanges.length) {
323320
return Promise.resolve();
324321
}
325-
const batchedDeltaChanges = OnyxUtils.applyMerge(undefined, validChanges, false, true, false);
322+
const batchedDeltaChanges = OnyxUtils.batchMergeChanges(validChanges);
326323

327324
// Case (1): When there is no existing value in storage, we want to set the value instead of merge it.
328325
// Case (2): The presence of a top-level `null` in the merge queue instructs us to drop the whole existing value.
@@ -353,7 +350,7 @@ function merge<TKey extends OnyxKey>(key: TKey, changes: OnyxMergeInput<TKey>):
353350
// We can remove null values from the "preMergedValue", because "null" implicates that the user wants to remove a value from storage.
354351
// Additionally, we will signal OnyxUtils.applyMerge() to replace any nested properties previously marked in "batchedDeltaChanges"
355352
// by our custom merging strategy.
356-
const preMergedValue = OnyxUtils.applyMerge(shouldSetValue ? undefined : existingValue, [batchedDeltaChanges], true, false, true);
353+
const preMergedValue = OnyxUtils.applyMerge(shouldSetValue ? undefined : existingValue, [batchedDeltaChanges]);
357354

358355
// In cache, we don't want to remove the key if it's null to improve performance and speed up the next merge.
359356
const hasChanged = cache.hasValueChanged(key, preMergedValue);
@@ -773,7 +770,7 @@ function update(data: OnyxUpdate[]): Promise<void> {
773770
// Remove the collection-related key from the updateQueue so that it won't be processed individually.
774771
delete updateQueue[key];
775772

776-
const batchedChanges = OnyxUtils.applyMerge(undefined, operations, false, true, false);
773+
const batchedChanges = OnyxUtils.batchMergeChanges(operations);
777774
if (operations[0] === null) {
778775
// eslint-disable-next-line no-param-reassign
779776
queue.set[key] = batchedChanges;
@@ -798,7 +795,7 @@ function update(data: OnyxUpdate[]): Promise<void> {
798795
});
799796

800797
Object.entries(updateQueue).forEach(([key, operations]) => {
801-
const batchedChanges = OnyxUtils.applyMerge(undefined, operations, false, true, false);
798+
const batchedChanges = OnyxUtils.batchMergeChanges(operations);
802799

803800
if (operations[0] === null) {
804801
promises.push(() => set(key, batchedChanges));

‎lib/OnyxUtils.ts‎

Lines changed: 20 additions & 11 deletions
Original file line numberDiff line numberDiff line change
@@ -1255,13 +1255,7 @@ function prepareKeyValuePairsForStorage(data: Record<OnyxKey, OnyxInput<OnyxKey>
12551255
*
12561256
* @param changes Array of changes that should be applied to the existing value
12571257
*/
1258-
function applyMerge<TValue extends OnyxInput<OnyxKey> | undefined, TChange extends OnyxInput<OnyxKey> | undefined>(
1259-
existingValue: TValue,
1260-
changes: TChange[],
1261-
shouldRemoveNestedNulls: boolean,
1262-
isBatchingMergeChanges: boolean,
1263-
shouldReplaceMarkedObjects: boolean,
1264-
): TChange {
1258+
function applyMerge<TValue extends OnyxInput<OnyxKey> | undefined, TChange extends OnyxInput<OnyxKey> | undefined>(existingValue: TValue, changes: TChange[]): TChange {
12651259
const lastChange = changes?.at(-1);
12661260

12671261
if (Array.isArray(lastChange)) {
@@ -1270,10 +1264,24 @@ function applyMerge<TValue extends OnyxInput<OnyxKey> | undefined, TChange exten
12701264

12711265
if (changes.some((change) => change && typeof change === 'object')) {
12721266
// Object values are then merged one after the other
1273-
return changes.reduce(
1274-
(modifiedData, change) => utils.fastMerge(modifiedData, change, shouldRemoveNestedNulls, isBatchingMergeChanges, shouldReplaceMarkedObjects),
1275-
(existingValue || {}) as TChange,
1276-
);
1267+
return changes.reduce((modifiedData, change) => utils.fastMerge(modifiedData, change, true, false, true), (existingValue || {}) as TChange);
1268+
}
1269+
1270+
// If we have anything else we can't merge it so we'll
1271+
// simply return the last value that was queued
1272+
return lastChange as TChange;
1273+
}
1274+
1275+
function batchMergeChanges<TChange extends OnyxInput<OnyxKey> | undefined>(changes: TChange[]): TChange {
1276+
const lastChange = changes?.at(-1);
1277+
1278+
if (Array.isArray(lastChange)) {
1279+
return lastChange;
1280+
}
1281+
1282+
if (changes.some((change) => change && typeof change === 'object')) {
1283+
// Object values are then merged one after the other
1284+
return changes.reduce((modifiedData, change) => utils.fastMerge(modifiedData, change, false, true, false), {} as TChange);
12771285
}
12781286

12791287
// If we have anything else we can't merge it so we'll
@@ -1490,6 +1498,7 @@ const OnyxUtils = {
14901498
getEvictionBlocklist,
14911499
getSkippableCollectionMemberIDs,
14921500
setSkippableCollectionMemberIDs,
1501+
batchMergeChanges,
14931502
};
14941503

14951504
GlobalSettings.addGlobalSettingsChangeListener(({enablePerformanceMetrics}) => {

‎tests/unit/onyxUtilsTest.ts‎

Lines changed: 79 additions & 68 deletions
Original file line numberDiff line numberDiff line change
@@ -3,6 +3,67 @@ import OnyxUtils from '../../lib/OnyxUtils';
33
import type {DeepRecord} from '../../lib/types';
44
import utils from '../../lib/utils';
55

6+
const testObject: DeepRecord<string, unknown> = {
7+
a: 'a',
8+
b: {
9+
c: 'c',
10+
d: {
11+
e: 'e',
12+
f: 'f',
13+
},
14+
g: 'g',
15+
},
16+
};
17+
18+
const testMergeChanges: Array<DeepRecord<string, unknown>> = [
19+
{
20+
b: {
21+
d: {
22+
h: 'h',
23+
},
24+
},
25+
},
26+
{
27+
b: {
28+
// Removing "d" object.
29+
d: null,
30+
h: 'h',
31+
},
32+
},
33+
{
34+
b: {
35+
// Adding back "d" property with a object.
36+
// The "ONYX_INTERNALS__REPLACE_OBJECT_MARK" marker property should be added here when batching merge changes.
37+
d: {
38+
i: 'i',
39+
},
40+
},
41+
},
42+
{
43+
b: {
44+
// Removing "d" object again.
45+
d: null,
46+
// Removing "g" object.
47+
g: null,
48+
},
49+
},
50+
{
51+
b: {
52+
// Adding back "d" property with a object.
53+
// The "ONYX_INTERNALS__REPLACE_OBJECT_MARK" marker property should be added here when batching merge changes.
54+
d: {
55+
i: 'i',
56+
j: 'j',
57+
},
58+
// Adding back "g" property with a object.
59+
// The "ONYX_INTERNALS__REPLACE_OBJECT_MARK" marker property should be added here when batching merge changes.
60+
g: {
61+
k: 'k',
62+
},
63+
},
64+
},
65+
];
66+
667
const ONYXKEYS = {
768
TEST_KEY: 'test',
869
COLLECTION: {
@@ -91,74 +152,20 @@ describe('OnyxUtils', () => {
91152
});
92153

93154
describe('applyMerge', () => {
94-
const testObject: DeepRecord<string, unknown> = {
95-
a: 'a',
96-
b: {
97-
c: 'c',
98-
d: {
99-
e: 'e',
100-
f: 'f',
101-
},
102-
g: 'g',
103-
},
104-
};
105-
106-
const testMergeChanges: Array<DeepRecord<string, unknown>> = [
107-
{
108-
b: {
109-
d: {
110-
h: 'h',
111-
},
112-
},
113-
},
114-
{
115-
b: {
116-
d: null,
117-
h: 'h',
118-
},
119-
},
120-
{
121-
b: {
122-
d: {
123-
i: 'i',
124-
},
125-
},
126-
},
127-
{
128-
b: {
129-
d: null,
130-
g: null,
131-
},
132-
},
133-
{
134-
b: {
135-
d: {
136-
i: 'i',
137-
j: 'j',
138-
},
139-
g: {
140-
k: 'k',
141-
},
142-
},
143-
},
144-
];
145-
146155
it("should return the last change if it's an array", () => {
147-
const result = OnyxUtils.applyMerge(testObject, [...testMergeChanges, [0, 1, 2]], false, false, true);
156+
const result = OnyxUtils.applyMerge(testObject, [...testMergeChanges, [0, 1, 2]]);
148157

149158
expect(result).toEqual([0, 1, 2]);
150159
});
151160

152161
it("should return the last change if the changes aren't objects", () => {
153-
const result = OnyxUtils.applyMerge(testObject, ['a', 0, 'b', 1], false, false, true);
162+
const result = OnyxUtils.applyMerge(testObject, ['a', 0, 'b', 1]);
154163

155164
expect(result).toEqual(1);
156165
});
157166

158-
it('should merge data correctly when batching merge changes', () => {
159-
const result = OnyxUtils.applyMerge(undefined, testMergeChanges, false, true, false);
160-
161-
expect(result).toEqual({
167+
it('should merge data correctly when applying batched changes', () => {
168+
const batchedChanges: DeepRecord<string, unknown> = {
162169
b: {
163170
d: {
164171
i: 'i',
@@ -171,37 +178,41 @@ describe('OnyxUtils', () => {
171178
k: 'k',
172179
},
173180
},
174-
});
175-
});
181+
};
176182

177-
it('should merge data correctly when applying batched changes', () => {
178-
const batchedChanges: DeepRecord<string, unknown> = {
183+
const result = OnyxUtils.applyMerge(testObject, [batchedChanges]);
184+
185+
expect(result).toEqual({
186+
a: 'a',
179187
b: {
188+
c: 'c',
180189
d: {
181190
i: 'i',
182191
j: 'j',
183-
[utils.ONYX_INTERNALS__REPLACE_OBJECT_MARK]: true,
184192
},
185193
h: 'h',
186194
g: {
187-
[utils.ONYX_INTERNALS__REPLACE_OBJECT_MARK]: true,
188195
k: 'k',
189196
},
190197
},
191-
};
198+
});
199+
});
200+
});
192201

193-
const result = OnyxUtils.applyMerge(testObject, [batchedChanges], false, false, true);
202+
describe('batchMergeChanges', () => {
203+
it('should apply the replacement markers if the we have properties with objects being removed and added back during the changes', () => {
204+
const result = OnyxUtils.batchMergeChanges(testMergeChanges);
194205

195206
expect(result).toEqual({
196-
a: 'a',
197207
b: {
198-
c: 'c',
199208
d: {
200209
i: 'i',
201210
j: 'j',
211+
[utils.ONYX_INTERNALS__REPLACE_OBJECT_MARK]: true,
202212
},
203213
h: 'h',
204214
g: {
215+
[utils.ONYX_INTERNALS__REPLACE_OBJECT_MARK]: true,
205216
k: 'k',
206217
},
207218
},

0 commit comments

Comments
 (0)