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

Remove null values in nested objects #353

Merged
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
40 commits
Select commit Hold shift + click to select a range
abd16fa
add TODO comment
Sep 19, 2023
3a7d969
Testing dependent keys
tgolen Sep 20, 2023
255893e
Pass withOnyx state to key functions
tgolen Sep 20, 2023
5b69984
Remove unused import
tgolen Sep 20, 2023
b3d1873
Filter out unrelated data before passing state
tgolen Sep 20, 2023
71891de
Add tests for nested dependencies and filter the state before passing
tgolen Sep 21, 2023
3650021
DRY up method
tgolen Sep 21, 2023
df8f94b
1.0.85
OSBotify Sep 21, 2023
053672b
Revert "Allow infinite dependent keys when using withOnyx"
tgolen Sep 21, 2023
be7d97d
1.0.86
OSBotify Sep 21, 2023
2e0c967
fix: remove nested object keys in merge and set
Sep 21, 2023
ce61889
feat: update tests to match removing nested null keys
Sep 21, 2023
328265c
fix: don't manually overwrite in fastMerge when identical
Sep 21, 2023
a70b43c
Merge branch 'main' into @chrispader/remove-nested-nullish-object-keys
Sep 21, 2023
6b87144
fix: update comment
Sep 21, 2023
d6c4e4a
fix: remove unneccesary test
Sep 21, 2023
a4dcbb0
Add types for 'Remove null values in nested objects'
blazejkustra Sep 22, 2023
05f19be
fix: overwrite nested keys with undefined
Sep 22, 2023
cf3da96
Update lib/Onyx.js
chrispader Sep 27, 2023
3b2f7eb
make remove key O(log N) instead of O(N^2)
Szymon20000 Sep 22, 2023
383bb08
fix lint
Szymon20000 Sep 22, 2023
25318f7
1.0.87
OSBotify Sep 22, 2023
372d442
Batch initial hydration
janicduplessis Sep 20, 2023
2e48152
Add comment
janicduplessis Sep 21, 2023
b298ad6
Address more review feedback
janicduplessis Sep 21, 2023
3bb6ef2
Try fix test
janicduplessis Sep 22, 2023
1f50b0a
1.0.88
OSBotify Sep 25, 2023
9462121
fix: use set in Storage.mergeItem
Sep 21, 2023
606cdf7
1.0.89
OSBotify Sep 25, 2023
231fa65
Propagate merge events accross tabs
ospfranco Sep 26, 2023
1a3a14f
1.0.90
OSBotify Sep 27, 2023
7a9453d
use lodash and fix continue on next line
Sep 27, 2023
ef8f2c4
remove optional argument
Sep 27, 2023
f272eab
add comment
Sep 27, 2023
f188dd9
add another comment
Sep 27, 2023
b86d500
Update types for 'Remove null values in nested objects'
blazejkustra Sep 26, 2023
cde461f
Merge branch 'main' into @chrispader/remove-nested-nullish-object-keys
Sep 27, 2023
c28c741
Merge branch 'main' into @chrispader/remove-nested-nullish-object-keys
Sep 28, 2023
64ada0d
remove underscore where possible
Sep 28, 2023
b5a0ba8
Update lib/Onyx.js
chrispader Sep 28, 2023
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
14 changes: 5 additions & 9 deletions lib/Onyx.d.ts
Original file line number Diff line number Diff line change
@@ -1,7 +1,6 @@
import {Component} from 'react';
import {PartialDeep} from 'type-fest';
import * as Logger from './Logger';
import {CollectionKey, CollectionKeyBase, DeepRecord, KeyValueMapping, OnyxCollection, OnyxEntry, OnyxKey, NullableProperties} from './types';
import {CollectionKey, CollectionKeyBase, DeepRecord, KeyValueMapping, NullishDeep, OnyxCollection, OnyxEntry, OnyxKey} from './types';

/**
* Represents a mapping object where each `OnyxKey` maps to either a value of its corresponding type in `KeyValueMapping` or `null`.
Expand Down Expand Up @@ -79,14 +78,14 @@ type OnyxUpdate =
| {
onyxMethod: typeof METHOD.MERGE;
key: TKey;
value: PartialDeep<KeyValueMapping[TKey]>;
value: NullishDeep<KeyValueMapping[TKey]>;
};
}[OnyxKey]
| {
[TKey in CollectionKeyBase]: {
onyxMethod: typeof METHOD.MERGE_COLLECTION;
key: TKey;
value: Record<`${TKey}${string}`, PartialDeep<KeyValueMapping[TKey]>>;
value: Record<`${TKey}${string}`, NullishDeep<KeyValueMapping[TKey]>>;
};
}[CollectionKeyBase];

Expand Down Expand Up @@ -202,7 +201,7 @@ declare function multiSet(data: Partial<NullableKeyValueMapping>): Promise<void>
* @param key ONYXKEYS key
* @param value Object or Array value to merge
*/
declare function merge<TKey extends OnyxKey>(key: TKey, value: NullableProperties<PartialDeep<KeyValueMapping[TKey]>>): Promise<void>;
declare function merge<TKey extends OnyxKey>(key: TKey, value: NullishDeep<KeyValueMapping[TKey]>): Promise<void>;

/**
* Clear out all the data in the store
Expand Down Expand Up @@ -244,10 +243,7 @@ declare function clear(keysToPreserve?: OnyxKey[]): Promise<void>;
* @param collectionKey e.g. `ONYXKEYS.COLLECTION.REPORT`
* @param collection Object collection keyed by individual collection member keys and values
*/
declare function mergeCollection<TKey extends CollectionKeyBase, TMap>(
collectionKey: TKey,
collection: Collection<TKey, TMap, PartialDeep<KeyValueMapping[TKey]>>,
): Promise<void>;
declare function mergeCollection<TKey extends CollectionKeyBase, TMap>(collectionKey: TKey, collection: Collection<TKey, TMap, NullishDeep<KeyValueMapping[TKey]>>): Promise<void>;

/**
* Insert API responses and lifecycle data into Onyx
Expand Down
27 changes: 18 additions & 9 deletions lib/Onyx.js
chrispader marked this conversation as resolved.
Show resolved Hide resolved
Original file line number Diff line number Diff line change
Expand Up @@ -1041,7 +1041,13 @@ function set(key, value) {
Logger.logAlert(`Onyx.set() called after Onyx.merge() for key: ${key}. It is recommended to use set() or merge() not both.`);
}

const valueWithNullRemoved = utils.removeNullObjectValues(value);
// We can remove all null values in an object by merging it with itself
// utils.fastMerge recursively goes through the object and removes all null values
// Passing two identical objects as source and target to fastMerge will not change it, but only remove the null values
let valueWithNullRemoved = value;
if (typeof value === 'object' && !_.isArray(value)) {
valueWithNullRemoved = utils.fastMerge(value, value);
}

const hasChanged = cache.hasValueChanged(key, valueWithNullRemoved);

Expand Down Expand Up @@ -1098,9 +1104,10 @@ function multiSet(data) {
* @private
* @param {*} existingValue
* @param {Array<*>} changes Array of changes that should be applied to the existing value
* @param {Boolean} shouldRemoveNullObjectValues
* @returns {*}
*/
function applyMerge(existingValue, changes) {
function applyMerge(existingValue, changes, shouldRemoveNullObjectValues) {
const lastChange = _.last(changes);

if (_.isArray(lastChange)) {
Expand All @@ -1109,7 +1116,7 @@ function applyMerge(existingValue, changes) {

if (_.some(changes, _.isObject)) {
// Object values are then merged one after the other
return _.reduce(changes, (modifiedData, change) => utils.fastMerge(modifiedData, change),
return _.reduce(changes, (modifiedData, change) => utils.fastMerge(modifiedData, change, shouldRemoveNullObjectValues),
existingValue || {});
}

Expand Down Expand Up @@ -1157,7 +1164,8 @@ function merge(key, changes) {
.then((existingValue) => {
try {
// We first only merge the changes, so we can provide these to the native implementation (SQLite uses only delta changes in "JSON_PATCH" to merge)
let batchedChanges = applyMerge(undefined, mergeQueue[key]);
// We don't want to remove null values from the "batchedChanges", because SQLite uses them to remove keys from storage natively.
let batchedChanges = applyMerge(undefined, mergeQueue[key], false);
chrispader marked this conversation as resolved.
Show resolved Hide resolved

if (_.isNull(batchedChanges)) {
return remove(key);
Expand All @@ -1172,15 +1180,16 @@ function merge(key, changes) {
delete mergeQueuePromise[key];

// After that we merge the batched changes with the existing value
const updatedValue = shouldOverwriteExistingValue ? batchedChanges : applyMerge(existingValue, [batchedChanges]);
const modifiedData = utils.removeNullObjectValues(updatedValue);
// We can remove null values from the "modifiedData", because "null" implicates that the user wants to remove a value from storage.
// The "modifiedData" will be directly "set" in storage instead of being merged
const modifiedData = shouldOverwriteExistingValue ? batchedChanges : applyMerge(existingValue, [batchedChanges], true);

// On native platforms we use SQLite which utilises JSON_PATCH to merge changes.
// JSON_PATCH generally removes top-level nullish values from the stored object.
// When there is no existing value though, SQLite will just insert the changes as a new value and thus the top-level nullish values won't be removed.
// Therefore we need to remove nullish values from the `batchedChanges` which are sent to the SQLite, if no existing value is present.
// When there is no existing value though, SQLite will just insert the changes as a new value and thus the null values won't be removed.
// Therefore we need to remove null values from the `batchedChanges` which are sent to the SQLite, if no existing value is present.
if (!existingValue) {
batchedChanges = utils.removeNullObjectValues(batchedChanges);
batchedChanges = applyMerge(undefined, mergeQueue[key], true);
chrispader marked this conversation as resolved.
Show resolved Hide resolved
}

const hasChanged = cache.hasValueChanged(key, modifiedData);
Expand Down
2 changes: 1 addition & 1 deletion lib/storage/providers/IDBKeyVal.js
Original file line number Diff line number Diff line change
Expand Up @@ -56,7 +56,7 @@ const provider = {
const upsertMany = _.map(pairs, ([key, value], index) => {
const prev = values[index];
const newValue = _.isObject(prev) ? utils.fastMerge(prev, value) : value;
return promisifyRequest(store.put(utils.removeNullObjectValues(newValue), key));
return promisifyRequest(store.put(newValue, key));
});
return Promise.all(upsertMany);
});
Expand Down
57 changes: 38 additions & 19 deletions lib/types.d.ts
Original file line number Diff line number Diff line change
@@ -1,10 +1,13 @@
import {Merge} from 'type-fest';
import {BuiltIns} from 'type-fest/source/internal';

/**
* Represents a deeply nested record. It maps keys to values,
* and those values can either be of type `TValue` or further nested `DeepRecord` instances.
*/
type DeepRecord<TKey extends string | number | symbol, TValue> = {[key: string]: TValue | DeepRecord<TKey, TValue>};
type DeepRecord<TKey extends string | number | symbol, TValue> = {
[key: string]: TValue | DeepRecord<TKey, TValue>;
};

/**
* Represents type options to configure all Onyx methods.
Expand Down Expand Up @@ -180,26 +183,42 @@ type OnyxEntry<TOnyxValue> = TOnyxValue | null;
*/
type OnyxCollection<TOnyxValue> = OnyxEntry<Record<string, TOnyxValue | null>>;

type NonTransformableTypes =
| BuiltIns
| ((...args: any[]) => unknown)
| Map<unknown, unknown>
| Set<unknown>
| ReadonlyMap<unknown, unknown>
| ReadonlySet<unknown>
| unknown[]
| readonly unknown[];

/**
* The `NullableProperties<T>` sets the values of all properties in `T` to be nullable (i.e., `| null`).
* It doesn't recurse into nested property values, this means it applies the nullability only to the top-level properties.
* Create a type from another type with all keys and nested keys set to optional or null.
*
* @example
* const settings: Settings = {
* textEditor: {
* fontSize: 14;
* fontColor: '#000000';
* fontWeight: 400;
* }
* autosave: true;
* };
*
* @template T The type of the properties to convert to nullable properties.
* const applySavedSettings = (savedSettings: NullishDeep<Settings>) => {
* return {...settings, ...savedSettings};
* }
*
* settings = applySavedSettings({textEditor: {fontWeight: 500, fontColor: null}});
*/
type NullableProperties<T> = {
[P in keyof T]: T[P] | null;
};
type NullishDeep<T> = T extends NonTransformableTypes ? T : T extends object ? NullishObjectDeep<T> : unknown;

export {
CollectionKey,
CollectionKeyBase,
CustomTypeOptions,
DeepRecord,
Key,
KeyValueMapping,
OnyxCollection,
OnyxEntry,
OnyxKey,
Selector,
NullableProperties,
/**
Same as `NullishDeep`, but accepts only `object`s as inputs. Internal helper for `NullishDeep`.
*/
type NullishObjectDeep<ObjectType extends object> = {
[KeyType in keyof ObjectType]?: NullishDeep<ObjectType[KeyType]> | null;
};

export {CollectionKey, CollectionKeyBase, CustomTypeOptions, DeepRecord, Key, KeyValueMapping, OnyxCollection, OnyxEntry, OnyxKey, Selector, NullishDeep};
60 changes: 32 additions & 28 deletions lib/utils.js
Original file line number Diff line number Diff line change
@@ -1,4 +1,4 @@
import * as _ from 'underscore';
import _ from 'underscore';

function areObjectsEmpty(a, b) {
return (
Expand All @@ -25,16 +25,26 @@ function isMergeableObject(val) {
/**
* @param {Object} target
* @param {Object} source
* @param {Boolean} shouldRemoveNullObjectValues
* @returns {Object}
*/
function mergeObject(target, source) {
function mergeObject(target, source, shouldRemoveNullObjectValues = true) {
const targetAndSourceIdentical = target === source;

const destination = {};
if (isMergeableObject(target)) {
// lodash adds a small overhead so we don't use it here
// eslint-disable-next-line rulesdir/prefer-underscore-method
const targetKeys = Object.keys(target);
for (let i = 0; i < targetKeys.length; ++i) {
const key = targetKeys[i];

// If shouldRemoveNullObjectValues is true, we want to remove null values from the merged object
if (shouldRemoveNullObjectValues && (target[key] === null || source[key] === null)) {
// eslint-disable-next-line no-continue
continue;
}

destination[key] = target[key];
}
}
Expand All @@ -44,55 +54,49 @@ function mergeObject(target, source) {
const sourceKeys = Object.keys(source);
for (let i = 0; i < sourceKeys.length; ++i) {
const key = sourceKeys[i];
if (source[key] === undefined) {

// If shouldRemoveNullObjectValues is true, we want to remove null values from the merged object
if (shouldRemoveNullObjectValues && source[key] === null) {
// eslint-disable-next-line no-continue
continue;
}

if (!isMergeableObject(source[key]) || !target[key]) {
if (targetAndSourceIdentical) {
// eslint-disable-next-line no-continue
continue;
}
destination[key] = source[key];
} else {
// eslint-disable-next-line no-use-before-define
destination[key] = fastMerge(target[key], source[key]);
destination[key] = fastMerge(target[key], source[key], shouldRemoveNullObjectValues);
}
}

return destination;
}

/**
* Merges two objects and removes null values if "shouldRemoveNullObjectValues" is set to true
*
* We generally want to remove null values from objects written to disk and cache, because it decreases the amount of data stored in memory and on disk.
* On native, when merging an existing value with new changes, SQLite will use JSON_PATCH, which removes top-level nullish values.
* To be consistent with the behaviour for merge, we'll also want to remove null values for "set" operations.
*
* @param {Object|Array} target
* @param {Object|Array} source
* @param {Boolean} shouldRemoveNullObjectValues
* @returns {Object|Array}
*/
function fastMerge(target, source) {
function fastMerge(target, source, shouldRemoveNullObjectValues = true) {
// We have to ignore arrays and nullish values here,
// otherwise "mergeObject" will throw an error,
// because it expects an object as "source"
if (_.isArray(source) || _.isNull(source) || _.isUndefined(source)) {
if (_.isArray(source) || source === null || source === undefined) {
return source;
}
return mergeObject(target, source);
}

/**
* We generally want to remove top-level nullish values from objects written to disk and cache, because it decreases the amount of data stored in memory and on disk.
* On native, when merging an existing value with new changes, SQLite will use JSON_PATCH, which removes top-level nullish values.
* To be consistent with the behaviour for merge, we'll also want to remove nullish values for "set" operations.
* On web, IndexedDB will keep the top-level keys along with a null value and this uses up storage and memory.
* This method will ensure that keys for null values are removed before an object is written to disk and cache so that all platforms are storing the data in the same efficient way.
* @private
* @param {*} value
* @returns {*}
*/
function removeNullObjectValues(value) {
if (_.isArray(value) || !_.isObject(value)) {
return value;
}

const objectWithoutNullObjectValues = _.omit(value, objectValue => _.isNull(objectValue));

return objectWithoutNullObjectValues;
return mergeObject(target, source, shouldRemoveNullObjectValues);
}

export default {removeNullObjectValues, areObjectsEmpty, fastMerge};
export default {areObjectsEmpty, fastMerge};

5 changes: 2 additions & 3 deletions tests/unit/onyxCacheTest.js
Original file line number Diff line number Diff line change
Expand Up @@ -309,16 +309,15 @@ describe('Onyx', () => {
expect(cache.getValue('mockKey')).toEqual({value: 'myMockObject'});
});

it('Should do nothing to a key which value is `undefined`', () => {
it('Should merge a key with `undefined`', () => {
// Given cache with existing data
cache.set('mockKey', {ID: 5});

// When merge is called key value pair and the value is undefined
cache.merge({mockKey: undefined});

// Then the key should still be in cache and the value unchanged
expect(cache.hasCacheForKey('mockKey')).toBe(true);
expect(cache.getValue('mockKey')).toEqual({ID: 5});
expect(cache.getValue('mockKey')).toEqual(undefined);
});

it('Should update storageKeys when new keys are created', () => {
Expand Down
Loading
Loading