From abd16fa61a15870b87d530104351d81ead0b83c4 Mon Sep 17 00:00:00 2001 From: Christoph Pader Date: Tue, 19 Sep 2023 13:05:02 +0200 Subject: [PATCH 01/37] add TODO comment --- lib/utils.js | 2 ++ 1 file changed, 2 insertions(+) diff --git a/lib/utils.js b/lib/utils.js index 140057125..e0368825b 100644 --- a/lib/utils.js +++ b/lib/utils.js @@ -88,6 +88,8 @@ function removeNullObjectValues(value) { return value; } + // TODO: Also remove nested nullish values + const objectWithoutNullObjectValues = _.omit(value, objectValue => _.isNull(objectValue)); return objectWithoutNullObjectValues; From 3a7d969b9f2bf02392b3d661611fa6e6321e0b01 Mon Sep 17 00:00:00 2001 From: Tim Golen Date: Thu, 21 Sep 2023 05:30:16 +0800 Subject: [PATCH 02/37] Testing dependent keys --- tests/unit/withOnyxTest.js | 20 ++++++++------------ 1 file changed, 8 insertions(+), 12 deletions(-) diff --git a/tests/unit/withOnyxTest.js b/tests/unit/withOnyxTest.js index 1f36cfe09..d4b00894a 100644 --- a/tests/unit/withOnyxTest.js +++ b/tests/unit/withOnyxTest.js @@ -250,18 +250,14 @@ describe('withOnyxTest', () => { Onyx.mergeCollection(ONYX_KEYS.COLLECTION.RELATED_KEY, {related_1: 'Test'}); return waitForPromisesToResolve() .then(() => { - const TestComponentWithOnyx = compose( - withOnyx({ - testObject: { - key: `${ONYX_KEYS.COLLECTION.TEST_KEY}${collectionItemID}`, - }, - }), - withOnyx({ - testThing: { - key: ({testObject}) => `${ONYX_KEYS.COLLECTION.RELATED_KEY}${testObject.id}`, - }, - }), - )(ViewWithCollections); + const TestComponentWithOnyx = withOnyx({ + testObject: { + key: `${ONYX_KEYS.COLLECTION.TEST_KEY}${collectionItemID}`, + }, + testThing: { + key: ({testObject}) => `${ONYX_KEYS.COLLECTION.RELATED_KEY}${testObject.id}`, + }, + })(ViewWithCollections); render(); }) .then(() => { From 255893ec6d3ef08349957d70dc9d88021a425f16 Mon Sep 17 00:00:00 2001 From: Tim Golen Date: Thu, 21 Sep 2023 06:34:14 +0800 Subject: [PATCH 03/37] Pass withOnyx state to key functions --- .gitignore | 1 + lib/withOnyx.js | 8 ++++---- tests/unit/withOnyxTest.js | 3 ++- 3 files changed, 7 insertions(+), 5 deletions(-) diff --git a/.gitignore b/.gitignore index 41f9bece4..4ee70c35e 100644 --- a/.gitignore +++ b/.gitignore @@ -11,6 +11,7 @@ dist/ # IDEs .idea +.vscode # Decrypted private key we do not want to commit .github/OSBotify-private-key.asc diff --git a/lib/withOnyx.js b/lib/withOnyx.js index 976152848..bbd610090 100644 --- a/lib/withOnyx.js +++ b/lib/withOnyx.js @@ -92,12 +92,12 @@ export default function (mapOnyxToState, shouldDelayUpdates = false) { this.checkEvictableKeys(); } - componentDidUpdate(prevProps) { + componentDidUpdate(prevProps, prevState) { // If any of the mappings use data from the props, then when the props change, all the // connections need to be reconnected with the new props _.each(mapOnyxToState, (mapping, propertyName) => { - const previousKey = Str.result(mapping.key, prevProps); - const newKey = Str.result(mapping.key, this.props); + const previousKey = Str.result(mapping.key, {...prevProps, ...prevState}); + const newKey = Str.result(mapping.key, {...this.props, ...this.state}); if (previousKey !== newKey) { Onyx.disconnect(this.activeConnectionIDs[previousKey], previousKey); delete this.activeConnectionIDs[previousKey]; @@ -238,7 +238,7 @@ export default function (mapOnyxToState, shouldDelayUpdates = false) { * component */ connectMappingToOnyx(mapping, statePropertyName) { - const key = Str.result(mapping.key, this.props); + const key = Str.result(mapping.key, {...this.props, ...this.state}); // eslint-disable-next-line rulesdir/prefer-onyx-connect-in-libs this.activeConnectionIDs[key] = Onyx.connect({ diff --git a/tests/unit/withOnyxTest.js b/tests/unit/withOnyxTest.js index d4b00894a..1f510ae1c 100644 --- a/tests/unit/withOnyxTest.js +++ b/tests/unit/withOnyxTest.js @@ -255,10 +255,11 @@ describe('withOnyxTest', () => { key: `${ONYX_KEYS.COLLECTION.TEST_KEY}${collectionItemID}`, }, testThing: { - key: ({testObject}) => `${ONYX_KEYS.COLLECTION.RELATED_KEY}${testObject.id}`, + key: ({testObject}) => `${ONYX_KEYS.COLLECTION.RELATED_KEY}${(testObject && testObject.id) || 0}`, }, })(ViewWithCollections); render(); + return waitForPromisesToResolve(); }) .then(() => { expect(onRender).toHaveBeenLastCalledWith({ From 5b69984bd15005172946d77a635fdab1c5058053 Mon Sep 17 00:00:00 2001 From: Tim Golen Date: Thu, 21 Sep 2023 06:38:38 +0800 Subject: [PATCH 04/37] Remove unused import --- tests/unit/withOnyxTest.js | 1 - 1 file changed, 1 deletion(-) diff --git a/tests/unit/withOnyxTest.js b/tests/unit/withOnyxTest.js index 1f510ae1c..f3c76cfd0 100644 --- a/tests/unit/withOnyxTest.js +++ b/tests/unit/withOnyxTest.js @@ -5,7 +5,6 @@ import Onyx, {withOnyx} from '../../lib'; import ViewWithText from '../components/ViewWithText'; import ViewWithCollections from '../components/ViewWithCollections'; import waitForPromisesToResolve from '../utils/waitForPromisesToResolve'; -import compose from '../../lib/compose'; import ViewWithObject from '../components/ViewWithObject'; const ONYX_KEYS = { From b3d1873365d3a31629cf20fb0bd69a47b1e3f82c Mon Sep 17 00:00:00 2001 From: Tim Golen Date: Thu, 21 Sep 2023 07:10:21 +0800 Subject: [PATCH 05/37] Filter out unrelated data before passing state --- lib/withOnyx.js | 18 +++++++++++++----- 1 file changed, 13 insertions(+), 5 deletions(-) diff --git a/lib/withOnyx.js b/lib/withOnyx.js index bbd610090..e9ea70977 100644 --- a/lib/withOnyx.js +++ b/lib/withOnyx.js @@ -93,15 +93,21 @@ export default function (mapOnyxToState, shouldDelayUpdates = false) { } componentDidUpdate(prevProps, prevState) { + // When the state is passed to the key functions with Str.result(), omit anything + // from state that was not part of the mapped keys. + const mappingPropNames = _.pluck(mapOnyxToState, 'key'); + const prevStateOnlyWithOnyxData = _.pick(prevState, mappingPropNames); + const stateOnlyWithOnyxData = _.pick(this.state, mappingPropNames); + // If any of the mappings use data from the props, then when the props change, all the // connections need to be reconnected with the new props - _.each(mapOnyxToState, (mapping, propertyName) => { - const previousKey = Str.result(mapping.key, {...prevProps, ...prevState}); - const newKey = Str.result(mapping.key, {...this.props, ...this.state}); + _.each(mapOnyxToState, (mapping, propName) => { + const previousKey = Str.result(mapping.key, {...prevProps, prevStateOnlyWithOnyxData}); + const newKey = Str.result(mapping.key, {...this.props, stateOnlyWithOnyxData}); if (previousKey !== newKey) { Onyx.disconnect(this.activeConnectionIDs[previousKey], previousKey); delete this.activeConnectionIDs[previousKey]; - this.connectMappingToOnyx(mapping, propertyName); + this.connectMappingToOnyx(mapping, propName); } }); this.checkEvictableKeys(); @@ -238,7 +244,9 @@ export default function (mapOnyxToState, shouldDelayUpdates = false) { * component */ connectMappingToOnyx(mapping, statePropertyName) { - const key = Str.result(mapping.key, {...this.props, ...this.state}); + const mappingPropNames = _.pluck(mapOnyxToState, 'key'); + const stateOnlyWithOnyxData = _.pick(this.state, mappingPropNames); + const key = Str.result(mapping.key, {...this.props, stateOnlyWithOnyxData}); // eslint-disable-next-line rulesdir/prefer-onyx-connect-in-libs this.activeConnectionIDs[key] = Onyx.connect({ From 71891de8af05cc0c7a921be19be6ea385ae984b5 Mon Sep 17 00:00:00 2001 From: Tim Golen Date: Thu, 21 Sep 2023 08:23:15 +0800 Subject: [PATCH 06/37] Add tests for nested dependencies and filter the state before passing --- lib/withOnyx.js | 18 +++++++----- tests/unit/withOnyxTest.js | 59 ++++++++++++++++++++++++++++++++------ 2 files changed, 61 insertions(+), 16 deletions(-) diff --git a/lib/withOnyx.js b/lib/withOnyx.js index e9ea70977..01221b7a0 100644 --- a/lib/withOnyx.js +++ b/lib/withOnyx.js @@ -92,18 +92,17 @@ export default function (mapOnyxToState, shouldDelayUpdates = false) { this.checkEvictableKeys(); } - componentDidUpdate(prevProps, prevState) { + componentDidUpdate() { // When the state is passed to the key functions with Str.result(), omit anything // from state that was not part of the mapped keys. - const mappingPropNames = _.pluck(mapOnyxToState, 'key'); - const prevStateOnlyWithOnyxData = _.pick(prevState, mappingPropNames); + const mappingPropNames = _.keys(mapOnyxToState); const stateOnlyWithOnyxData = _.pick(this.state, mappingPropNames); // If any of the mappings use data from the props, then when the props change, all the // connections need to be reconnected with the new props _.each(mapOnyxToState, (mapping, propName) => { - const previousKey = Str.result(mapping.key, {...prevProps, prevStateOnlyWithOnyxData}); - const newKey = Str.result(mapping.key, {...this.props, stateOnlyWithOnyxData}); + const previousKey = mapping.previousKey; + const newKey = Str.result(mapping.key, {...this.props, ...stateOnlyWithOnyxData}); if (previousKey !== newKey) { Onyx.disconnect(this.activeConnectionIDs[previousKey], previousKey); delete this.activeConnectionIDs[previousKey]; @@ -244,9 +243,14 @@ export default function (mapOnyxToState, shouldDelayUpdates = false) { * component */ connectMappingToOnyx(mapping, statePropertyName) { - const mappingPropNames = _.pluck(mapOnyxToState, 'key'); + const mappingPropNames = _.keys(mapOnyxToState); const stateOnlyWithOnyxData = _.pick(this.state, mappingPropNames); - const key = Str.result(mapping.key, {...this.props, stateOnlyWithOnyxData}); + const key = Str.result(mapping.key, {...this.props, ...stateOnlyWithOnyxData}); + + // Remember the previous key so that if it ever changes, the component will reconnect to Onyx + // in componentDidUpdate + // eslint-disable-next-line no-param-reassign + mapOnyxToState[statePropertyName].previousKey = key; // eslint-disable-next-line rulesdir/prefer-onyx-connect-in-libs this.activeConnectionIDs[key] = Onyx.connect({ diff --git a/tests/unit/withOnyxTest.js b/tests/unit/withOnyxTest.js index f3c76cfd0..c6ee21406 100644 --- a/tests/unit/withOnyxTest.js +++ b/tests/unit/withOnyxTest.js @@ -11,7 +11,10 @@ const ONYX_KEYS = { TEST_KEY: 'test', COLLECTION: { TEST_KEY: 'test_', - RELATED_KEY: 'related_', + STATIC: 'static_', + DEPENDS_ON_STATIC: 'dependsOnStatic_', + DEPENDS_ON_DEPENDS_ON_STATIC: 'dependsOnDependsOnStatic_', + DEPENDS_ON_DEPENDS_ON_DEPENDS_ON_STATIC: 'dependsOnDependsOnDependsOnStatic_', }, SIMPLE_KEY: 'simple', SIMPLE_KEY_2: 'simple2', @@ -242,27 +245,65 @@ describe('withOnyxTest', () => { }); it('should pass a prop from one connected component to another', () => { - const collectionItemID = 1; const onRender = jest.fn(); const markReadyForHydration = jest.fn(); - Onyx.mergeCollection(ONYX_KEYS.COLLECTION.TEST_KEY, {test_1: {id: 1}}); - Onyx.mergeCollection(ONYX_KEYS.COLLECTION.RELATED_KEY, {related_1: 'Test'}); + + // Given three collections with multiple items in each + Onyx.mergeCollection(ONYX_KEYS.COLLECTION.STATIC, { + static_1: {name: 'Static 1', id: 1}, + static_2: {name: 'Static 2', id: 2}, + }); + + // And one collection will depends on data being loaded from the static collection + Onyx.mergeCollection(ONYX_KEYS.COLLECTION.DEPENDS_ON_STATIC, { + dependsOnStatic_1: {name: 'dependsOnStatic 1', id: 3}, + dependsOnStatic_2: {name: 'dependsOnStatic 2', id: 4}, + }); + + // And one collection will depend on the data being loaded from the collection that depends on the static collection (multiple nested dependencies) + Onyx.mergeCollection(ONYX_KEYS.COLLECTION.DEPENDS_ON_DEPENDS_ON_STATIC, { + dependsOnDependsOnStatic_3: {name: 'dependsOnDependsOnStatic 1', id: 5}, + dependsOnDependsOnStatic_4: {name: 'dependsOnDependsOnStatic 2', id: 6}, + }); + + // And another collection with one more layer of dependency just to prove it works + Onyx.mergeCollection(ONYX_KEYS.COLLECTION.DEPENDS_ON_DEPENDS_ON_DEPENDS_ON_STATIC, { + dependsOnDependsOnDependsOnStatic_5: {name: 'dependsOnDependsOnDependsOnStatic 1'}, + dependsOnDependsOnDependsOnStatic_6: {name: 'dependsOnDependsOnDependsOnStatic 2'}, + }); + + // When a component is rendered using withOnyx and several nested dependencies on the keys return waitForPromisesToResolve() .then(() => { const TestComponentWithOnyx = withOnyx({ - testObject: { - key: `${ONYX_KEYS.COLLECTION.TEST_KEY}${collectionItemID}`, + staticObject: { + key: `${ONYX_KEYS.COLLECTION.STATIC}1`, }, - testThing: { - key: ({testObject}) => `${ONYX_KEYS.COLLECTION.RELATED_KEY}${(testObject && testObject.id) || 0}`, + dependentObject: { + key: ({staticObject}) => `${ONYX_KEYS.COLLECTION.DEPENDS_ON_STATIC}${(staticObject && staticObject.id) || 0}`, + }, + multiDependentObject: { + key: ({dependentObject}) => `${ONYX_KEYS.COLLECTION.DEPENDS_ON_DEPENDS_ON_STATIC}${(dependentObject && dependentObject.id) || 0}`, + }, + extremeMultiDependentObject: { + key: ({multiDependentObject}) => `${ONYX_KEYS.COLLECTION.DEPENDS_ON_DEPENDS_ON_DEPENDS_ON_STATIC}${(multiDependentObject && multiDependentObject.id) || 0}`, }, })(ViewWithCollections); render(); return waitForPromisesToResolve(); }) + + // Then all of the data gets properly loaded into the component as expected with the nested dependencies resolved .then(() => { expect(onRender).toHaveBeenLastCalledWith({ - collections: {}, markReadyForHydration, onRender, testObject: {id: 1}, testThing: 'Test', + markReadyForHydration, + onRender, + collections: {}, + testObject: {isDefaultProp: true}, + staticObject: {name: 'Static 1', id: 1}, + dependentObject: {name: 'dependsOnStatic 1', id: 3}, + multiDependentObject: {name: 'dependsOnDependsOnStatic 1', id: 5}, + extremeMultiDependentObject: {name: 'dependsOnDependsOnDependsOnStatic 1'}, }); }); }); From 3650021626f50483c8ccfccd0bdcd5cf1396f857 Mon Sep 17 00:00:00 2001 From: Tim Golen Date: Thu, 21 Sep 2023 08:35:47 +0800 Subject: [PATCH 07/37] DRY up method --- lib/withOnyx.js | 18 ++++++++++++------ 1 file changed, 12 insertions(+), 6 deletions(-) diff --git a/lib/withOnyx.js b/lib/withOnyx.js index 01221b7a0..58e2a87e6 100644 --- a/lib/withOnyx.js +++ b/lib/withOnyx.js @@ -20,6 +20,15 @@ function getDisplayName(component) { return component.displayName || component.name || 'Component'; } +/** + * Removes all the keys from state that are unrelated to the onyx data being mapped to the component. + * + * @param {Object} state of the component + * @param {Object} onyxToStateMapping the object holding all of the mapping configuration for the component + * @returns {Object} + */ +const getOnyxDataFromState = (state, onyxToStateMapping) => _.pick(state, _.keys(onyxToStateMapping)); + export default function (mapOnyxToState, shouldDelayUpdates = false) { // A list of keys that must be present in tempState before we can render the WrappedComponent const requiredKeysForInit = _.chain(mapOnyxToState) @@ -95,14 +104,13 @@ export default function (mapOnyxToState, shouldDelayUpdates = false) { componentDidUpdate() { // When the state is passed to the key functions with Str.result(), omit anything // from state that was not part of the mapped keys. - const mappingPropNames = _.keys(mapOnyxToState); - const stateOnlyWithOnyxData = _.pick(this.state, mappingPropNames); + const onyxDataFromState = getOnyxDataFromState(this.state, mapOnyxToState); // If any of the mappings use data from the props, then when the props change, all the // connections need to be reconnected with the new props _.each(mapOnyxToState, (mapping, propName) => { const previousKey = mapping.previousKey; - const newKey = Str.result(mapping.key, {...this.props, ...stateOnlyWithOnyxData}); + const newKey = Str.result(mapping.key, {...this.props, ...onyxDataFromState}); if (previousKey !== newKey) { Onyx.disconnect(this.activeConnectionIDs[previousKey], previousKey); delete this.activeConnectionIDs[previousKey]; @@ -243,9 +251,7 @@ export default function (mapOnyxToState, shouldDelayUpdates = false) { * component */ connectMappingToOnyx(mapping, statePropertyName) { - const mappingPropNames = _.keys(mapOnyxToState); - const stateOnlyWithOnyxData = _.pick(this.state, mappingPropNames); - const key = Str.result(mapping.key, {...this.props, ...stateOnlyWithOnyxData}); + const key = Str.result(mapping.key, {...this.props, ...getOnyxDataFromState(this.state, mapOnyxToState)}); // Remember the previous key so that if it ever changes, the component will reconnect to Onyx // in componentDidUpdate From df8f94b3536577b07a3f1d875cfd0d7054eee94f Mon Sep 17 00:00:00 2001 From: OSBotify Date: Thu, 21 Sep 2023 04:18:36 +0000 Subject: [PATCH 08/37] 1.0.85 --- package-lock.json | 4 ++-- package.json | 2 +- 2 files changed, 3 insertions(+), 3 deletions(-) diff --git a/package-lock.json b/package-lock.json index 810bbd625..71124035a 100644 --- a/package-lock.json +++ b/package-lock.json @@ -1,12 +1,12 @@ { "name": "react-native-onyx", - "version": "1.0.84", + "version": "1.0.85", "lockfileVersion": 2, "requires": true, "packages": { "": { "name": "react-native-onyx", - "version": "1.0.84", + "version": "1.0.85", "license": "MIT", "dependencies": { "ascii-table": "0.0.9", diff --git a/package.json b/package.json index 456cba28b..bdefd612b 100644 --- a/package.json +++ b/package.json @@ -1,6 +1,6 @@ { "name": "react-native-onyx", - "version": "1.0.84", + "version": "1.0.85", "author": "Expensify, Inc.", "homepage": "https://expensify.com", "description": "State management for React Native", From 053672b32d5495600f8ab10b08d33e0e338ca3fe Mon Sep 17 00:00:00 2001 From: Tim Golen Date: Thu, 21 Sep 2023 02:01:57 -0600 Subject: [PATCH 09/37] Revert "Allow infinite dependent keys when using withOnyx" --- .gitignore | 1 - lib/withOnyx.js | 30 ++++------------ tests/unit/withOnyxTest.js | 73 ++++++++++---------------------------- 3 files changed, 24 insertions(+), 80 deletions(-) diff --git a/.gitignore b/.gitignore index 4ee70c35e..41f9bece4 100644 --- a/.gitignore +++ b/.gitignore @@ -11,7 +11,6 @@ dist/ # IDEs .idea -.vscode # Decrypted private key we do not want to commit .github/OSBotify-private-key.asc diff --git a/lib/withOnyx.js b/lib/withOnyx.js index 58e2a87e6..976152848 100644 --- a/lib/withOnyx.js +++ b/lib/withOnyx.js @@ -20,15 +20,6 @@ function getDisplayName(component) { return component.displayName || component.name || 'Component'; } -/** - * Removes all the keys from state that are unrelated to the onyx data being mapped to the component. - * - * @param {Object} state of the component - * @param {Object} onyxToStateMapping the object holding all of the mapping configuration for the component - * @returns {Object} - */ -const getOnyxDataFromState = (state, onyxToStateMapping) => _.pick(state, _.keys(onyxToStateMapping)); - export default function (mapOnyxToState, shouldDelayUpdates = false) { // A list of keys that must be present in tempState before we can render the WrappedComponent const requiredKeysForInit = _.chain(mapOnyxToState) @@ -101,20 +92,16 @@ export default function (mapOnyxToState, shouldDelayUpdates = false) { this.checkEvictableKeys(); } - componentDidUpdate() { - // When the state is passed to the key functions with Str.result(), omit anything - // from state that was not part of the mapped keys. - const onyxDataFromState = getOnyxDataFromState(this.state, mapOnyxToState); - + componentDidUpdate(prevProps) { // If any of the mappings use data from the props, then when the props change, all the // connections need to be reconnected with the new props - _.each(mapOnyxToState, (mapping, propName) => { - const previousKey = mapping.previousKey; - const newKey = Str.result(mapping.key, {...this.props, ...onyxDataFromState}); + _.each(mapOnyxToState, (mapping, propertyName) => { + const previousKey = Str.result(mapping.key, prevProps); + const newKey = Str.result(mapping.key, this.props); if (previousKey !== newKey) { Onyx.disconnect(this.activeConnectionIDs[previousKey], previousKey); delete this.activeConnectionIDs[previousKey]; - this.connectMappingToOnyx(mapping, propName); + this.connectMappingToOnyx(mapping, propertyName); } }); this.checkEvictableKeys(); @@ -251,12 +238,7 @@ export default function (mapOnyxToState, shouldDelayUpdates = false) { * component */ connectMappingToOnyx(mapping, statePropertyName) { - const key = Str.result(mapping.key, {...this.props, ...getOnyxDataFromState(this.state, mapOnyxToState)}); - - // Remember the previous key so that if it ever changes, the component will reconnect to Onyx - // in componentDidUpdate - // eslint-disable-next-line no-param-reassign - mapOnyxToState[statePropertyName].previousKey = key; + const key = Str.result(mapping.key, this.props); // eslint-disable-next-line rulesdir/prefer-onyx-connect-in-libs this.activeConnectionIDs[key] = Onyx.connect({ diff --git a/tests/unit/withOnyxTest.js b/tests/unit/withOnyxTest.js index c6ee21406..1f36cfe09 100644 --- a/tests/unit/withOnyxTest.js +++ b/tests/unit/withOnyxTest.js @@ -5,16 +5,14 @@ import Onyx, {withOnyx} from '../../lib'; import ViewWithText from '../components/ViewWithText'; import ViewWithCollections from '../components/ViewWithCollections'; import waitForPromisesToResolve from '../utils/waitForPromisesToResolve'; +import compose from '../../lib/compose'; import ViewWithObject from '../components/ViewWithObject'; const ONYX_KEYS = { TEST_KEY: 'test', COLLECTION: { TEST_KEY: 'test_', - STATIC: 'static_', - DEPENDS_ON_STATIC: 'dependsOnStatic_', - DEPENDS_ON_DEPENDS_ON_STATIC: 'dependsOnDependsOnStatic_', - DEPENDS_ON_DEPENDS_ON_DEPENDS_ON_STATIC: 'dependsOnDependsOnDependsOnStatic_', + RELATED_KEY: 'related_', }, SIMPLE_KEY: 'simple', SIMPLE_KEY_2: 'simple2', @@ -245,65 +243,30 @@ describe('withOnyxTest', () => { }); it('should pass a prop from one connected component to another', () => { + const collectionItemID = 1; const onRender = jest.fn(); const markReadyForHydration = jest.fn(); - - // Given three collections with multiple items in each - Onyx.mergeCollection(ONYX_KEYS.COLLECTION.STATIC, { - static_1: {name: 'Static 1', id: 1}, - static_2: {name: 'Static 2', id: 2}, - }); - - // And one collection will depends on data being loaded from the static collection - Onyx.mergeCollection(ONYX_KEYS.COLLECTION.DEPENDS_ON_STATIC, { - dependsOnStatic_1: {name: 'dependsOnStatic 1', id: 3}, - dependsOnStatic_2: {name: 'dependsOnStatic 2', id: 4}, - }); - - // And one collection will depend on the data being loaded from the collection that depends on the static collection (multiple nested dependencies) - Onyx.mergeCollection(ONYX_KEYS.COLLECTION.DEPENDS_ON_DEPENDS_ON_STATIC, { - dependsOnDependsOnStatic_3: {name: 'dependsOnDependsOnStatic 1', id: 5}, - dependsOnDependsOnStatic_4: {name: 'dependsOnDependsOnStatic 2', id: 6}, - }); - - // And another collection with one more layer of dependency just to prove it works - Onyx.mergeCollection(ONYX_KEYS.COLLECTION.DEPENDS_ON_DEPENDS_ON_DEPENDS_ON_STATIC, { - dependsOnDependsOnDependsOnStatic_5: {name: 'dependsOnDependsOnDependsOnStatic 1'}, - dependsOnDependsOnDependsOnStatic_6: {name: 'dependsOnDependsOnDependsOnStatic 2'}, - }); - - // When a component is rendered using withOnyx and several nested dependencies on the keys + Onyx.mergeCollection(ONYX_KEYS.COLLECTION.TEST_KEY, {test_1: {id: 1}}); + Onyx.mergeCollection(ONYX_KEYS.COLLECTION.RELATED_KEY, {related_1: 'Test'}); return waitForPromisesToResolve() .then(() => { - const TestComponentWithOnyx = withOnyx({ - staticObject: { - key: `${ONYX_KEYS.COLLECTION.STATIC}1`, - }, - dependentObject: { - key: ({staticObject}) => `${ONYX_KEYS.COLLECTION.DEPENDS_ON_STATIC}${(staticObject && staticObject.id) || 0}`, - }, - multiDependentObject: { - key: ({dependentObject}) => `${ONYX_KEYS.COLLECTION.DEPENDS_ON_DEPENDS_ON_STATIC}${(dependentObject && dependentObject.id) || 0}`, - }, - extremeMultiDependentObject: { - key: ({multiDependentObject}) => `${ONYX_KEYS.COLLECTION.DEPENDS_ON_DEPENDS_ON_DEPENDS_ON_STATIC}${(multiDependentObject && multiDependentObject.id) || 0}`, - }, - })(ViewWithCollections); + const TestComponentWithOnyx = compose( + withOnyx({ + testObject: { + key: `${ONYX_KEYS.COLLECTION.TEST_KEY}${collectionItemID}`, + }, + }), + withOnyx({ + testThing: { + key: ({testObject}) => `${ONYX_KEYS.COLLECTION.RELATED_KEY}${testObject.id}`, + }, + }), + )(ViewWithCollections); render(); - return waitForPromisesToResolve(); }) - - // Then all of the data gets properly loaded into the component as expected with the nested dependencies resolved .then(() => { expect(onRender).toHaveBeenLastCalledWith({ - markReadyForHydration, - onRender, - collections: {}, - testObject: {isDefaultProp: true}, - staticObject: {name: 'Static 1', id: 1}, - dependentObject: {name: 'dependsOnStatic 1', id: 3}, - multiDependentObject: {name: 'dependsOnDependsOnStatic 1', id: 5}, - extremeMultiDependentObject: {name: 'dependsOnDependsOnDependsOnStatic 1'}, + collections: {}, markReadyForHydration, onRender, testObject: {id: 1}, testThing: 'Test', }); }); }); From be7d97dbbe8848649b15e9f7c622b3338f2542de Mon Sep 17 00:00:00 2001 From: OSBotify Date: Thu, 21 Sep 2023 08:04:50 +0000 Subject: [PATCH 10/37] 1.0.86 --- package-lock.json | 4 ++-- package.json | 2 +- 2 files changed, 3 insertions(+), 3 deletions(-) diff --git a/package-lock.json b/package-lock.json index 71124035a..0005537ff 100644 --- a/package-lock.json +++ b/package-lock.json @@ -1,12 +1,12 @@ { "name": "react-native-onyx", - "version": "1.0.85", + "version": "1.0.86", "lockfileVersion": 2, "requires": true, "packages": { "": { "name": "react-native-onyx", - "version": "1.0.85", + "version": "1.0.86", "license": "MIT", "dependencies": { "ascii-table": "0.0.9", diff --git a/package.json b/package.json index bdefd612b..4e861e01e 100644 --- a/package.json +++ b/package.json @@ -1,6 +1,6 @@ { "name": "react-native-onyx", - "version": "1.0.85", + "version": "1.0.86", "author": "Expensify, Inc.", "homepage": "https://expensify.com", "description": "State management for React Native", From 2e0c967d2caac5e2c5c0a9ce24483b12af4eea42 Mon Sep 17 00:00:00 2001 From: Christoph Pader Date: Thu, 21 Sep 2023 14:36:41 +0200 Subject: [PATCH 11/37] fix: remove nested object keys in merge and set --- lib/Onyx.js | 23 +++++++---- lib/fastMerge.js | 65 ------------------------------ lib/storage/providers/IDBKeyVal.js | 2 +- lib/utils.js | 52 ++++++++++-------------- 4 files changed, 38 insertions(+), 104 deletions(-) delete mode 100644 lib/fastMerge.js diff --git a/lib/Onyx.js b/lib/Onyx.js index d4d329e0f..4f7e3f439 100644 --- a/lib/Onyx.js +++ b/lib/Onyx.js @@ -1020,7 +1020,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); + // In case value is an object we want to remove all null values + // utils.fastMerge recursively goes through the object and removes all null values + // Passing two same object 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); @@ -1077,9 +1083,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 = true) { const lastChange = _.last(changes); if (_.isArray(existingValue) || _.isArray(lastChange)) { @@ -1090,7 +1097,7 @@ function applyMerge(existingValue, changes) { // Object values are merged one after the other // lodash adds a small overhead so we don't use it here // eslint-disable-next-line prefer-object-spread, rulesdir/prefer-underscore-method - return _.reduce(changes, (modifiedData, change) => utils.fastMerge(modifiedData, change), + return _.reduce(changes, (modifiedData, change) => utils.fastMerge(modifiedData, change, shouldRemoveNullObjectValues), existingValue || {}); } @@ -1132,21 +1139,21 @@ 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]); + let batchedChanges = applyMerge(undefined, mergeQueue[key], false); // Clean up the write queue, so we don't apply these changes again delete mergeQueue[key]; delete mergeQueuePromise[key]; // After that we merge the batched changes with the existing value - const modifiedData = utils.removeNullObjectValues(applyMerge(existingValue, [batchedChanges])); + const modifiedData = applyMerge(existingValue, [batchedChanges]); // 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); } const hasChanged = cache.hasValueChanged(key, modifiedData); diff --git a/lib/fastMerge.js b/lib/fastMerge.js deleted file mode 100644 index 2ddc5ad62..000000000 --- a/lib/fastMerge.js +++ /dev/null @@ -1,65 +0,0 @@ -import _ from 'underscore'; - -// Mostly copied from https://medium.com/@lubaka.a/how-to-remove-lodash-performance-improvement-b306669ad0e1 - -/** - * @param {mixed} val - * @returns {boolean} -*/ -function isMergeableObject(val) { - const nonNullObject = val != null ? typeof val === 'object' : false; - return (nonNullObject - && Object.prototype.toString.call(val) !== '[object RegExp]' - && Object.prototype.toString.call(val) !== '[object Date]'); -} - -/** - * @param {Object} target - * @param {Object} source - * @returns {Object} -*/ -function mergeObject(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]; - destination[key] = target[key]; - } - } - - // lodash adds a small overhead so we don't use it here - // eslint-disable-next-line rulesdir/prefer-underscore-method - const sourceKeys = Object.keys(source); - for (let i = 0; i < sourceKeys.length; ++i) { - const key = sourceKeys[i]; - if (source[key] === undefined) { - // eslint-disable-next-line no-continue - continue; - } - if (!isMergeableObject(source[key]) || !target[key]) { - destination[key] = source[key]; - } else { - // eslint-disable-next-line no-use-before-define - destination[key] = fastMerge(target[key], source[key]); - } - } - - return destination; -} - -/** - * @param {Object|Array} target - * @param {Object|Array} source - * @returns {Object|Array} -*/ -function fastMerge(target, source) { - if (_.isArray(source) || _.isNull(source) || _.isUndefined(source)) { - return source; - } - return mergeObject(target, source); -} - -export default fastMerge; diff --git a/lib/storage/providers/IDBKeyVal.js b/lib/storage/providers/IDBKeyVal.js index c72ce7507..0dd2090e4 100644 --- a/lib/storage/providers/IDBKeyVal.js +++ b/lib/storage/providers/IDBKeyVal.js @@ -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); }); diff --git a/lib/utils.js b/lib/utils.js index e0368825b..b546d546f 100644 --- a/lib/utils.js +++ b/lib/utils.js @@ -25,6 +25,7 @@ function isMergeableObject(val) { /** * @param {Object} target * @param {Object} source +* @param {Boolean} shouldRemoveNullObjectValues * @returns {Object} */ function mergeObject(target, source) { @@ -35,6 +36,11 @@ function mergeObject(target, source) { 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 + // eslint-disable-next-line no-continue + if (shouldRemoveNullObjectValues && (target[key] == null || source[key] === null)) { continue; } + destination[key] = target[key]; } } @@ -44,15 +50,16 @@ 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) { - // eslint-disable-next-line no-continue - continue; - } + + // If shouldRemoveNullObjectValues is true, we want to remove null values from the merged object + // eslint-disable-next-line no-continue + if (source[key] === undefined || (shouldRemoveNullObjectValues && source[key] === null)) { continue; } + if (!isMergeableObject(source[key]) || !target[key]) { 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); } } @@ -60,40 +67,25 @@ function mergeObject(target, source) { } /** + * 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) { // lodash adds a small overhead so we don't use it here // eslint-disable-next-line rulesdir/prefer-underscore-method if (_.isArray(source) || _.isNull(source) || _.isUndefined(source)) { 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; - } - - // TODO: Also remove nested nullish values - - const objectWithoutNullObjectValues = _.omit(value, objectValue => _.isNull(objectValue)); - - return objectWithoutNullObjectValues; + return mergeObject(target, source, shouldRemoveNullObjectValues); } -export default {removeNullObjectValues, areObjectsEmpty, fastMerge}; +export default {areObjectsEmpty, fastMerge}; From ce61889aaa224e29db000f0a8ca02e961defc7d7 Mon Sep 17 00:00:00 2001 From: Christoph Pader Date: Thu, 21 Sep 2023 14:36:53 +0200 Subject: [PATCH 12/37] feat: update tests to match removing nested null keys --- tests/unit/onyxTest.js | 62 +++++++++++++++++++++++++++++++++++++++--- 1 file changed, 58 insertions(+), 4 deletions(-) diff --git a/tests/unit/onyxTest.js b/tests/unit/onyxTest.js index 7b6f5eb2b..eb1611100 100644 --- a/tests/unit/onyxTest.js +++ b/tests/unit/onyxTest.js @@ -223,7 +223,7 @@ describe('Onyx', () => { }); }); - it('should remove top-level keys that are set to null/undefined when merging', () => { + it('should remove keys that are set to null when merging', () => { let testKeyValue; connectionID = Onyx.connect({ @@ -234,13 +234,67 @@ describe('Onyx', () => { }, }); - return Onyx.set(ONYX_KEYS.TEST_KEY, {test1: 'test1', test2: 'test2'}) + return Onyx.set(ONYX_KEYS.TEST_KEY, { + test1: { + test2: 'test2', + test3: 'test3', + }, + }) .then(() => { - expect(testKeyValue).toEqual({test1: 'test1', test2: 'test2'}); + expect(testKeyValue).toEqual({ + test1: { + test2: 'test2', + test3: 'test3', + }, + }); + return Onyx.merge(ONYX_KEYS.TEST_KEY, { + test1: { + test2: null, + }, + }); + }) + .then(() => { + expect(testKeyValue).toEqual({test1: {test3: 'test3'}}); return Onyx.merge(ONYX_KEYS.TEST_KEY, {test1: null}); }) .then(() => { - expect(testKeyValue).toEqual({test2: 'test2'}); + expect(testKeyValue).toEqual({}); + }); + }); + + it('should keep empty object when removing all nested keys during merging', () => { + let testKeyValue; + + connectionID = Onyx.connect({ + key: ONYX_KEYS.TEST_KEY, + initWithStoredValues: false, + callback: (value) => { + testKeyValue = value; + }, + }); + + return Onyx.set(ONYX_KEYS.TEST_KEY, { + test1: { + test2: 'test2', + test3: 'test3', + }, + }) + .then(() => { + expect(testKeyValue).toEqual({ + test1: { + test2: 'test2', + test3: 'test3', + }, + }); + return Onyx.merge(ONYX_KEYS.TEST_KEY, { + test1: { + test2: null, + test3: null, + }, + }); + }) + .then(() => { + expect(testKeyValue).toEqual({test1: {}}); }); }); From 328265c7f125bdc4477048f3f214f4ac8c83a8b5 Mon Sep 17 00:00:00 2001 From: Christoph Pader Date: Thu, 21 Sep 2023 14:37:12 +0200 Subject: [PATCH 13/37] fix: don't manually overwrite in fastMerge when identical --- lib/utils.js | 6 +++++- 1 file changed, 5 insertions(+), 1 deletion(-) diff --git a/lib/utils.js b/lib/utils.js index b546d546f..af9f23ee3 100644 --- a/lib/utils.js +++ b/lib/utils.js @@ -28,7 +28,9 @@ function isMergeableObject(val) { * @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 @@ -56,6 +58,8 @@ function mergeObject(target, source) { if (source[key] === undefined || (shouldRemoveNullObjectValues && source[key] === null)) { continue; } if (!isMergeableObject(source[key]) || !target[key]) { + // eslint-disable-next-line no-continue + if (targetAndSourceIdentical) { continue; } destination[key] = source[key]; } else { // eslint-disable-next-line no-use-before-define From 6b871449d98d750f4bcd58233778c86e0db1e08e Mon Sep 17 00:00:00 2001 From: Christoph Pader Date: Thu, 21 Sep 2023 14:48:28 +0200 Subject: [PATCH 14/37] fix: update comment --- lib/Onyx.js | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/lib/Onyx.js b/lib/Onyx.js index 4f7e3f439..b4eb22dc7 100644 --- a/lib/Onyx.js +++ b/lib/Onyx.js @@ -1022,7 +1022,7 @@ function set(key, value) { // In case value is an object we want to remove all null values // utils.fastMerge recursively goes through the object and removes all null values - // Passing two same object as source and target to fastMerge will not change it, but only remove the 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); From d6c4e4adb64017360d38870d69236e1262facc24 Mon Sep 17 00:00:00 2001 From: Christoph Pader Date: Thu, 21 Sep 2023 14:50:18 +0200 Subject: [PATCH 15/37] fix: remove unneccesary test --- tests/unit/onyxTest.js | 36 ------------------------------------ 1 file changed, 36 deletions(-) diff --git a/tests/unit/onyxTest.js b/tests/unit/onyxTest.js index eb1611100..696897ea6 100644 --- a/tests/unit/onyxTest.js +++ b/tests/unit/onyxTest.js @@ -262,42 +262,6 @@ describe('Onyx', () => { }); }); - it('should keep empty object when removing all nested keys during merging', () => { - let testKeyValue; - - connectionID = Onyx.connect({ - key: ONYX_KEYS.TEST_KEY, - initWithStoredValues: false, - callback: (value) => { - testKeyValue = value; - }, - }); - - return Onyx.set(ONYX_KEYS.TEST_KEY, { - test1: { - test2: 'test2', - test3: 'test3', - }, - }) - .then(() => { - expect(testKeyValue).toEqual({ - test1: { - test2: 'test2', - test3: 'test3', - }, - }); - return Onyx.merge(ONYX_KEYS.TEST_KEY, { - test1: { - test2: null, - test3: null, - }, - }); - }) - .then(() => { - expect(testKeyValue).toEqual({test1: {}}); - }); - }); - it('should overwrite an array key nested inside an object', () => { let testKeyValue; connectionID = Onyx.connect({ From a4dcbb054c9bf10d95df566ca97ec5e297c1db8e Mon Sep 17 00:00:00 2001 From: Blazej Kustra Date: Fri, 22 Sep 2023 10:35:36 +0200 Subject: [PATCH 16/37] Add types for 'Remove null values in nested objects' --- lib/Onyx.d.ts | 2 +- lib/types.d.ts | 57 +++++++++++++++++++++++++++++++++----------------- 2 files changed, 39 insertions(+), 20 deletions(-) diff --git a/lib/Onyx.d.ts b/lib/Onyx.d.ts index 0c7fdd319..d5034939e 100644 --- a/lib/Onyx.d.ts +++ b/lib/Onyx.d.ts @@ -202,7 +202,7 @@ declare function multiSet(data: Partial): Promise * @param key ONYXKEYS key * @param value Object or Array value to merge */ -declare function merge(key: TKey, value: NullableProperties>): Promise; +declare function merge(key: TKey, value: NullishDeep): Promise; /** * Clear out all the data in the store diff --git a/lib/types.d.ts b/lib/types.d.ts index c43d06403..80b586cc5 100644 --- a/lib/types.d.ts +++ b/lib/types.d.ts @@ -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 = {[key: string]: TValue | DeepRecord}; +type DeepRecord = { + [key: string]: TValue | DeepRecord; +}; /** * Represents type options to configure all Onyx methods. @@ -180,26 +183,42 @@ type OnyxEntry = TOnyxValue | null; */ type OnyxCollection = OnyxEntry>; +type NonTransformableTypes = + | BuiltIns + | ((...args: any[]) => unknown) + | Map + | Set + | ReadonlyMap + | ReadonlySet + | unknown[] + | readonly unknown[]; + /** - * The `NullableProperties` 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) => { + * return {...settings, ...savedSettings}; + * } + * + * settings = applySavedSettings({textEditor: {fontWeight: 500, fontColor: null}}); */ -type NullableProperties = { - [P in keyof T]: T[P] | null; -}; +type NullishDeep = T extends NonTransformableTypes ? T : T extends object ? NullishObjectDeep : 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 = { + [KeyType in keyof ObjectType]?: NullishDeep | null; }; + +export {CollectionKey, CollectionKeyBase, CustomTypeOptions, DeepRecord, Key, KeyValueMapping, OnyxCollection, OnyxEntry, OnyxKey, Selector, NullishDeep}; From 05f19be9c62a08c191aca835d266ec013d6d2d5b Mon Sep 17 00:00:00 2001 From: Christoph Pader Date: Fri, 22 Sep 2023 11:01:18 +0200 Subject: [PATCH 17/37] fix: overwrite nested keys with undefined --- lib/utils.js | 2 +- tests/unit/onyxCacheTest.js | 5 ++--- tests/unit/onyxTest.js | 39 +++++++++++++++++++++++++++++++++++++ 3 files changed, 42 insertions(+), 4 deletions(-) diff --git a/lib/utils.js b/lib/utils.js index af9f23ee3..5f9e55207 100644 --- a/lib/utils.js +++ b/lib/utils.js @@ -55,7 +55,7 @@ function mergeObject(target, source, shouldRemoveNullObjectValues = true) { // If shouldRemoveNullObjectValues is true, we want to remove null values from the merged object // eslint-disable-next-line no-continue - if (source[key] === undefined || (shouldRemoveNullObjectValues && source[key] === null)) { continue; } + if (shouldRemoveNullObjectValues && source[key] === null) { continue; } if (!isMergeableObject(source[key]) || !target[key]) { // eslint-disable-next-line no-continue diff --git a/tests/unit/onyxCacheTest.js b/tests/unit/onyxCacheTest.js index d48c4c26b..cc6ac8182 100644 --- a/tests/unit/onyxCacheTest.js +++ b/tests/unit/onyxCacheTest.js @@ -309,7 +309,7 @@ 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}); @@ -317,8 +317,7 @@ describe('Onyx', () => { 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', () => { diff --git a/tests/unit/onyxTest.js b/tests/unit/onyxTest.js index 696897ea6..525869386 100644 --- a/tests/unit/onyxTest.js +++ b/tests/unit/onyxTest.js @@ -262,6 +262,45 @@ describe('Onyx', () => { }); }); + it('should note remove keys that are set to undefined when merging', () => { + let testKeyValue; + + connectionID = Onyx.connect({ + key: ONYX_KEYS.TEST_KEY, + initWithStoredValues: false, + callback: (value) => { + testKeyValue = value; + }, + }); + + return Onyx.set(ONYX_KEYS.TEST_KEY, { + test1: { + test2: 'test2', + test3: 'test3', + }, + }) + .then(() => { + expect(testKeyValue).toEqual({ + test1: { + test2: 'test2', + test3: 'test3', + }, + }); + return Onyx.merge(ONYX_KEYS.TEST_KEY, { + test1: { + test2: undefined, + }, + }); + }) + .then(() => { + expect(testKeyValue).toEqual({test1: {test2: undefined, test3: 'test3'}}); + return Onyx.merge(ONYX_KEYS.TEST_KEY, {test1: undefined}); + }) + .then(() => { + expect(testKeyValue).toEqual({test1: undefined}); + }); + }); + it('should overwrite an array key nested inside an object', () => { let testKeyValue; connectionID = Onyx.connect({ From cf3da961d7710dce1e33b8748ef96af84651c31e Mon Sep 17 00:00:00 2001 From: Christoph Pader Date: Wed, 27 Sep 2023 10:16:43 +0200 Subject: [PATCH 18/37] Update lib/Onyx.js Co-authored-by: Marc Glasser --- lib/Onyx.js | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/lib/Onyx.js b/lib/Onyx.js index b4eb22dc7..6287618cb 100644 --- a/lib/Onyx.js +++ b/lib/Onyx.js @@ -1020,7 +1020,7 @@ 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.`); } - // In case value is an object we want to remove all null values + // 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; From 3b2f7eb13d8a77e1d4ec2058f49dd1e817b42bfe Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Szymon=20Kapa=C5=82a?= Date: Fri, 22 Sep 2023 10:49:39 +0200 Subject: [PATCH 19/37] make remove key O(log N) instead of O(N^2) --- lib/OnyxCache.js | 15 ++++++--------- 1 file changed, 6 insertions(+), 9 deletions(-) diff --git a/lib/OnyxCache.js b/lib/OnyxCache.js index e7f6f58fb..73af434de 100644 --- a/lib/OnyxCache.js +++ b/lib/OnyxCache.js @@ -179,16 +179,13 @@ class OnyxCache { * Remove keys that don't fall into the range of recently used keys */ removeLeastRecentlyUsedKeys() { - if (this.recentKeys.size <= this.maxRecentKeysSize) { - return; + while (this.recentKeys.size > this.maxRecentKeysSize) { + const iterator = this.recentKeys.values(); + const value = iterator.next().value + if (value !== undefined) { + this.drop(value) + } } - - // Get the last N keys by doing a negative slice - const recentlyAccessed = [...this.recentKeys].slice(-this.maxRecentKeysSize); - const storageKeys = _.keys(this.storageMap); - const keysToRemove = _.difference(storageKeys, recentlyAccessed); - - _.each(keysToRemove, this.drop); } /** From 383bb086e7428be9300f80bf34a891d0b01b3055 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Szymon=20Kapa=C5=82a?= Date: Fri, 22 Sep 2023 10:59:52 +0200 Subject: [PATCH 20/37] fix lint --- lib/OnyxCache.js | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/lib/OnyxCache.js b/lib/OnyxCache.js index 73af434de..f68315424 100644 --- a/lib/OnyxCache.js +++ b/lib/OnyxCache.js @@ -181,9 +181,9 @@ class OnyxCache { removeLeastRecentlyUsedKeys() { while (this.recentKeys.size > this.maxRecentKeysSize) { const iterator = this.recentKeys.values(); - const value = iterator.next().value + const value = iterator.next().value; if (value !== undefined) { - this.drop(value) + this.drop(value); } } } From 25318f7d149a8caf3a5684378051c9215df4c66a Mon Sep 17 00:00:00 2001 From: OSBotify Date: Fri, 22 Sep 2023 11:15:32 +0000 Subject: [PATCH 21/37] 1.0.87 --- package-lock.json | 4 ++-- package.json | 2 +- 2 files changed, 3 insertions(+), 3 deletions(-) diff --git a/package-lock.json b/package-lock.json index 0005537ff..00f143756 100644 --- a/package-lock.json +++ b/package-lock.json @@ -1,12 +1,12 @@ { "name": "react-native-onyx", - "version": "1.0.86", + "version": "1.0.87", "lockfileVersion": 2, "requires": true, "packages": { "": { "name": "react-native-onyx", - "version": "1.0.86", + "version": "1.0.87", "license": "MIT", "dependencies": { "ascii-table": "0.0.9", diff --git a/package.json b/package.json index 4e861e01e..99ca08c3a 100644 --- a/package.json +++ b/package.json @@ -1,6 +1,6 @@ { "name": "react-native-onyx", - "version": "1.0.86", + "version": "1.0.87", "author": "Expensify, Inc.", "homepage": "https://expensify.com", "description": "State management for React Native", From 372d4427cacd69100dbd417ea7c76a28d97c2dd8 Mon Sep 17 00:00:00 2001 From: Janic Duplessis Date: Wed, 20 Sep 2023 13:27:15 +0200 Subject: [PATCH 22/37] Batch initial hydration --- lib/Onyx.js | 103 ++++++++++++++++++++----------------- tests/unit/withOnyxTest.js | 5 +- 2 files changed, 58 insertions(+), 50 deletions(-) diff --git a/lib/Onyx.js b/lib/Onyx.js index 6287618cb..ff131dd29 100644 --- a/lib/Onyx.js +++ b/lib/Onyx.js @@ -49,6 +49,47 @@ let defaultKeyStates = {}; // Connections can be made before `Onyx.init`. They would wait for this task before resolving const deferredInitTask = createDeferredTask(); +let batchUpdatesPromise = null; +let batchUpdatesQueue = []; + +/** + * We are batching together onyx updates. This helps with use cases where we schedule onyx updates after each other. + * This happens for example in the Onyx.update function, where we process API responses that might contain a lot of + * update operations. Instead of calling the subscribers for each update operation, we batch them together which will + * cause react to schedule the updates at once instead of after each other. This is mainly a performance optimization. + * @returns {Promise} + */ +function maybeFlushBatchUpdates() { + if (batchUpdatesPromise) { + return batchUpdatesPromise; + } + + batchUpdatesPromise = new Promise((resolve) => { + /* We use (setTimeout, 0) here which should be called once native module calls are flushed (usually at the end of the frame) + * We may investigate if (setTimeout, 1) (which in React Native is equal to requestAnimationFrame) works even better + * then the batch will be flushed on next frame. + */ + setTimeout(() => { + const updatesCopy = batchUpdatesQueue; + batchUpdatesQueue = []; + batchUpdatesPromise = null; + unstable_batchedUpdates(() => { + updatesCopy.forEach((applyUpdates) => { + applyUpdates(); + }); + }); + + resolve(); + }, 0); + }); + return batchUpdatesPromise; +} + +function batchUpdates(updates) { + batchUpdatesQueue.push(updates); + return maybeFlushBatchUpdates(); +} + /** * Uses a selector function to return a simplified version of sourceData * @param {Mixed} sourceData @@ -639,8 +680,9 @@ function keyChanged(key, data, canUpdateSubscriber, notifyRegularSubscibers = tr * @param {String} [mapping.selector] * @param {*|null} val * @param {String} matchedKey + * @param {Boolean} batched */ -function sendDataToConnection(mapping, val, matchedKey) { +function sendDataToConnection(mapping, val, matchedKey, batched) { // If the mapping no longer exists then we should not send any data. // This means our subscriber disconnected or withOnyx wrapped component unmounted. if (!callbackToStateMapping[mapping.connectionID]) { @@ -661,7 +703,13 @@ function sendDataToConnection(mapping, val, matchedKey) { } PerformanceUtils.logSetStateCall(mapping, null, newData, 'sendDataToConnection'); - mapping.withOnyxInstance.setWithOnyxState(mapping.statePropertyName, newData); + if (batched) { + batchUpdates(() => { + mapping.withOnyxInstance.setWithOnyxState(mapping.statePropertyName, newData); + }); + } else { + mapping.withOnyxInstance.setWithOnyxState(mapping.statePropertyName, newData); + } return; } @@ -711,7 +759,7 @@ function getCollectionDataAndSendAsObject(matchingKeys, mapping) { finalObject[matchingKeys[i]] = value; return finalObject; }, {})) - .then(val => sendDataToConnection(mapping, val)); + .then(val => sendDataToConnection(mapping, val, undefined, true)); } /** @@ -766,7 +814,7 @@ function connect(mapping) { // since there are none matched. In withOnyx() we wait for all connected keys to return a value before rendering the child // component. This null value will be filtered out so that the connected component can utilize defaultProps. if (matchingKeys.length === 0) { - sendDataToConnection(mapping, null); + sendDataToConnection(mapping, null, undefined, false); return; } @@ -782,13 +830,13 @@ function connect(mapping) { // We did not opt into using waitForCollectionCallback mode so the callback is called for every matching key. for (let i = 0; i < matchingKeys.length; i++) { - get(matchingKeys[i]).then(val => sendDataToConnection(mapping, val, matchingKeys[i])); + get(matchingKeys[i]).then(val => sendDataToConnection(mapping, val, matchingKeys[i], true)); } return; } // If we are not subscribed to a collection key then there's only a single key to send an update for. - get(mapping.key).then(val => sendDataToConnection(mapping, val, mapping.key)); + get(mapping.key).then(val => sendDataToConnection(mapping, val, mapping.key, true)); return; } @@ -801,7 +849,7 @@ function connect(mapping) { } // If the subscriber is not using a collection key then we just send a single value back to the subscriber - get(mapping.key).then(val => sendDataToConnection(mapping, val, mapping.key)); + get(mapping.key).then(val => sendDataToConnection(mapping, val, mapping.key, true)); return; } @@ -835,47 +883,6 @@ function disconnect(connectionID, keyToRemoveFromEvictionBlocklist) { delete callbackToStateMapping[connectionID]; } -let batchUpdatesPromise = null; -let batchUpdatesQueue = []; - -/** - * We are batching together onyx updates. This helps with use cases where we schedule onyx updates after each other. - * This happens for example in the Onyx.update function, where we process API responses that might contain a lot of - * update operations. Instead of calling the subscribers for each update operation, we batch them together which will - * cause react to schedule the updates at once instead of after each other. This is mainly a performance optimization. - * @returns {Promise} - */ -function maybeFlushBatchUpdates() { - if (batchUpdatesPromise) { - return batchUpdatesPromise; - } - - batchUpdatesPromise = new Promise((resolve) => { - /* We use (setTimeout, 0) here which should be called once native module calls are flushed (usually at the end of the frame) - * We may investigate if (setTimeout, 1) (which in React Native is equal to requestAnimationFrame) works even better - * then the batch will be flushed on next frame. - */ - setTimeout(() => { - const updatesCopy = batchUpdatesQueue; - batchUpdatesQueue = []; - batchUpdatesPromise = null; - unstable_batchedUpdates(() => { - updatesCopy.forEach((applyUpdates) => { - applyUpdates(); - }); - }); - - resolve(); - }, 0); - }); - return batchUpdatesPromise; -} - -function batchUpdates(updates) { - batchUpdatesQueue.push(updates); - return maybeFlushBatchUpdates(); -} - /** * Schedules an update that will be appended to the macro task queue (so it doesn't update the subscribers immediately). * diff --git a/tests/unit/withOnyxTest.js b/tests/unit/withOnyxTest.js index 1f36cfe09..81cef4740 100644 --- a/tests/unit/withOnyxTest.js +++ b/tests/unit/withOnyxTest.js @@ -215,8 +215,9 @@ describe('withOnyxTest', () => { .then(() => { rerender(); - // Note, when we change the prop, we need to wait for the next tick: - return waitForPromisesToResolve(); + // Note, when we change the prop, we need to wait for one tick for the + // component to update and one tick for batching. + return waitForPromisesToResolve().then(waitForPromisesToResolve); }) .then(() => { expect(getByTestId('text-element').props.children).toEqual('test_2'); From 2e48152e12fdcf74cba38c7ba7813c386257db47 Mon Sep 17 00:00:00 2001 From: Janic Duplessis Date: Thu, 21 Sep 2023 10:00:13 +0200 Subject: [PATCH 23/37] Add comment --- lib/Onyx.js | 1 + 1 file changed, 1 insertion(+) diff --git a/lib/Onyx.js b/lib/Onyx.js index ff131dd29..561c1cd70 100644 --- a/lib/Onyx.js +++ b/lib/Onyx.js @@ -814,6 +814,7 @@ function connect(mapping) { // since there are none matched. In withOnyx() we wait for all connected keys to return a value before rendering the child // component. This null value will be filtered out so that the connected component can utilize defaultProps. if (matchingKeys.length === 0) { + // Here we cannot use batching because the null value is expected to be set immediately so that default props work. sendDataToConnection(mapping, null, undefined, false); return; } From b298ad653532999a91625a808e7da79015965fdb Mon Sep 17 00:00:00 2001 From: Janic Duplessis Date: Thu, 21 Sep 2023 10:03:32 +0200 Subject: [PATCH 24/37] Address more review feedback --- lib/Onyx.js | 11 ++++++----- 1 file changed, 6 insertions(+), 5 deletions(-) diff --git a/lib/Onyx.js b/lib/Onyx.js index 561c1cd70..deadc4084 100644 --- a/lib/Onyx.js +++ b/lib/Onyx.js @@ -679,10 +679,10 @@ function keyChanged(key, data, canUpdateSubscriber, notifyRegularSubscibers = tr * @param {Function} [mapping.callback] * @param {String} [mapping.selector] * @param {*|null} val - * @param {String} matchedKey - * @param {Boolean} batched + * @param {String|undefined} matchedKey + * @param {Boolean} isBatched */ -function sendDataToConnection(mapping, val, matchedKey, batched) { +function sendDataToConnection(mapping, val, matchedKey, isBatched) { // If the mapping no longer exists then we should not send any data. // This means our subscriber disconnected or withOnyx wrapped component unmounted. if (!callbackToStateMapping[mapping.connectionID]) { @@ -703,7 +703,7 @@ function sendDataToConnection(mapping, val, matchedKey, batched) { } PerformanceUtils.logSetStateCall(mapping, null, newData, 'sendDataToConnection'); - if (batched) { + if (isBatched) { batchUpdates(() => { mapping.withOnyxInstance.setWithOnyxState(mapping.statePropertyName, newData); }); @@ -814,7 +814,8 @@ function connect(mapping) { // since there are none matched. In withOnyx() we wait for all connected keys to return a value before rendering the child // component. This null value will be filtered out so that the connected component can utilize defaultProps. if (matchingKeys.length === 0) { - // Here we cannot use batching because the null value is expected to be set immediately so that default props work. + // Here we cannot use batching because the null value is expected to be set immediately for default props + // or they will be undefined. sendDataToConnection(mapping, null, undefined, false); return; } From 3bb6ef21b0e1b62c3658cd02db6f5d412d8416d9 Mon Sep 17 00:00:00 2001 From: Janic Duplessis Date: Fri, 22 Sep 2023 10:20:52 +0200 Subject: [PATCH 25/37] Try fix test --- tests/unit/withOnyxTest.js | 1 + 1 file changed, 1 insertion(+) diff --git a/tests/unit/withOnyxTest.js b/tests/unit/withOnyxTest.js index 81cef4740..65a052312 100644 --- a/tests/unit/withOnyxTest.js +++ b/tests/unit/withOnyxTest.js @@ -265,6 +265,7 @@ describe('withOnyxTest', () => { )(ViewWithCollections); render(); }) + .then(waitForPromisesToResolve) .then(() => { expect(onRender).toHaveBeenLastCalledWith({ collections: {}, markReadyForHydration, onRender, testObject: {id: 1}, testThing: 'Test', From 1f50b0a2daa15ae0f32ae81874984e3d9a3e6ccd Mon Sep 17 00:00:00 2001 From: OSBotify Date: Mon, 25 Sep 2023 07:41:39 +0000 Subject: [PATCH 26/37] 1.0.88 --- package-lock.json | 4 ++-- package.json | 2 +- 2 files changed, 3 insertions(+), 3 deletions(-) diff --git a/package-lock.json b/package-lock.json index 00f143756..eca7db5e7 100644 --- a/package-lock.json +++ b/package-lock.json @@ -1,12 +1,12 @@ { "name": "react-native-onyx", - "version": "1.0.87", + "version": "1.0.88", "lockfileVersion": 2, "requires": true, "packages": { "": { "name": "react-native-onyx", - "version": "1.0.87", + "version": "1.0.88", "license": "MIT", "dependencies": { "ascii-table": "0.0.9", diff --git a/package.json b/package.json index 99ca08c3a..7ab8c45da 100644 --- a/package.json +++ b/package.json @@ -1,6 +1,6 @@ { "name": "react-native-onyx", - "version": "1.0.87", + "version": "1.0.88", "author": "Expensify, Inc.", "homepage": "https://expensify.com", "description": "State management for React Native", From 9462121c08799a4d94e3b67806bab5fa34df9768 Mon Sep 17 00:00:00 2001 From: Christoph Pader Date: Thu, 21 Sep 2023 14:40:48 +0200 Subject: [PATCH 27/37] fix: use set in Storage.mergeItem --- lib/storage/providers/IDBKeyVal.js | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/lib/storage/providers/IDBKeyVal.js b/lib/storage/providers/IDBKeyVal.js index 0dd2090e4..09357d9f5 100644 --- a/lib/storage/providers/IDBKeyVal.js +++ b/lib/storage/providers/IDBKeyVal.js @@ -70,7 +70,8 @@ const provider = { * @return {Promise} */ mergeItem(key, _changes, modifiedData) { - return provider.multiMerge([[key, modifiedData]]); + // Since Onyx also merged the existing value with the changes, we can just set the value directly + return provider.setItem(key, modifiedData); }, /** From 606cdf785ffbdc8011c7665357d275b8801cc0a1 Mon Sep 17 00:00:00 2001 From: OSBotify Date: Mon, 25 Sep 2023 13:36:20 +0000 Subject: [PATCH 28/37] 1.0.89 --- package-lock.json | 4 ++-- package.json | 2 +- 2 files changed, 3 insertions(+), 3 deletions(-) diff --git a/package-lock.json b/package-lock.json index eca7db5e7..204233a4a 100644 --- a/package-lock.json +++ b/package-lock.json @@ -1,12 +1,12 @@ { "name": "react-native-onyx", - "version": "1.0.88", + "version": "1.0.89", "lockfileVersion": 2, "requires": true, "packages": { "": { "name": "react-native-onyx", - "version": "1.0.88", + "version": "1.0.89", "license": "MIT", "dependencies": { "ascii-table": "0.0.9", diff --git a/package.json b/package.json index 7ab8c45da..05781107c 100644 --- a/package.json +++ b/package.json @@ -1,6 +1,6 @@ { "name": "react-native-onyx", - "version": "1.0.88", + "version": "1.0.89", "author": "Expensify, Inc.", "homepage": "https://expensify.com", "description": "State management for React Native", From 231fa652802931eafb272bb79d86b3330b3811c7 Mon Sep 17 00:00:00 2001 From: Oscar Franco Date: Tue, 26 Sep 2023 13:30:02 +0200 Subject: [PATCH 29/37] Propagate merge events accross tabs --- lib/storage/WebStorage.js | 3 +++ 1 file changed, 3 insertions(+) diff --git a/lib/storage/WebStorage.js b/lib/storage/WebStorage.js index 6b4889f90..b3e2c11e2 100644 --- a/lib/storage/WebStorage.js +++ b/lib/storage/WebStorage.js @@ -40,6 +40,9 @@ const webStorage = { this.removeItems = keys => Storage.removeItems(keys) .then(() => raiseStorageSyncManyKeysEvent(keys)); + this.mergeItem = (key, batchedChanges, modifiedData) => Storage.mergeItem(key, batchedChanges, modifiedData) + .then(() => raiseStorageSyncEvent(key)); + // If we just call Storage.clear other tabs will have no idea which keys were available previously // so that they can call keysChanged for them. That's why we iterate over every key and raise a storage sync // event for each one From 1a3a14fa820b7c2523701aa43e9f9b51be987639 Mon Sep 17 00:00:00 2001 From: OSBotify Date: Wed, 27 Sep 2023 03:00:37 +0000 Subject: [PATCH 30/37] 1.0.90 --- package-lock.json | 4 ++-- package.json | 2 +- 2 files changed, 3 insertions(+), 3 deletions(-) diff --git a/package-lock.json b/package-lock.json index 204233a4a..6acdfa136 100644 --- a/package-lock.json +++ b/package-lock.json @@ -1,12 +1,12 @@ { "name": "react-native-onyx", - "version": "1.0.89", + "version": "1.0.90", "lockfileVersion": 2, "requires": true, "packages": { "": { "name": "react-native-onyx", - "version": "1.0.89", + "version": "1.0.90", "license": "MIT", "dependencies": { "ascii-table": "0.0.9", diff --git a/package.json b/package.json index 05781107c..3aaafbde3 100644 --- a/package.json +++ b/package.json @@ -1,6 +1,6 @@ { "name": "react-native-onyx", - "version": "1.0.89", + "version": "1.0.90", "author": "Expensify, Inc.", "homepage": "https://expensify.com", "description": "State management for React Native", From 7a9453dc292bdfa159bee59dcaf3bbf49aab234a Mon Sep 17 00:00:00 2001 From: Christoph Pader Date: Wed, 27 Sep 2023 10:16:29 +0200 Subject: [PATCH 31/37] use lodash and fix continue on next line --- lib/utils.js | 20 +++++++++++++------- 1 file changed, 13 insertions(+), 7 deletions(-) diff --git a/lib/utils.js b/lib/utils.js index 5f9e55207..a6988d7b4 100644 --- a/lib/utils.js +++ b/lib/utils.js @@ -1,4 +1,4 @@ -import * as _ from 'underscore'; +import _ from 'underscore'; function areObjectsEmpty(a, b) { return ( @@ -40,8 +40,10 @@ function mergeObject(target, source, shouldRemoveNullObjectValues = true) { const key = targetKeys[i]; // If shouldRemoveNullObjectValues is true, we want to remove null values from the merged object - // eslint-disable-next-line no-continue - if (shouldRemoveNullObjectValues && (target[key] == null || source[key] === null)) { continue; } + if (shouldRemoveNullObjectValues && (_.isNull(target[key]) || _.isNull(source[key]))) { + // eslint-disable-next-line no-continue + continue; + } destination[key] = target[key]; } @@ -54,12 +56,16 @@ function mergeObject(target, source, shouldRemoveNullObjectValues = true) { const key = sourceKeys[i]; // If shouldRemoveNullObjectValues is true, we want to remove null values from the merged object - // eslint-disable-next-line no-continue - if (shouldRemoveNullObjectValues && source[key] === null) { continue; } + if (shouldRemoveNullObjectValues && _.isNull(source[key])) { + // eslint-disable-next-line no-continue + continue; + } if (!isMergeableObject(source[key]) || !target[key]) { - // eslint-disable-next-line no-continue - if (targetAndSourceIdentical) { continue; } + if (targetAndSourceIdentical) { + // eslint-disable-next-line no-continue + continue; + } destination[key] = source[key]; } else { // eslint-disable-next-line no-use-before-define From ef8f2c4eea93fa216d1bdd3d2353b52c2f08f61f Mon Sep 17 00:00:00 2001 From: Christoph Pader Date: Wed, 27 Sep 2023 10:18:04 +0200 Subject: [PATCH 32/37] remove optional argument --- lib/Onyx.js | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/lib/Onyx.js b/lib/Onyx.js index deadc4084..9f81ca0de 100644 --- a/lib/Onyx.js +++ b/lib/Onyx.js @@ -1095,7 +1095,7 @@ function multiSet(data) { * @param {Boolean} shouldRemoveNullObjectValues * @returns {*} */ -function applyMerge(existingValue, changes, shouldRemoveNullObjectValues = true) { +function applyMerge(existingValue, changes, shouldRemoveNullObjectValues) { const lastChange = _.last(changes); if (_.isArray(existingValue) || _.isArray(lastChange)) { @@ -1155,7 +1155,7 @@ function merge(key, changes) { delete mergeQueuePromise[key]; // After that we merge the batched changes with the existing value - const modifiedData = applyMerge(existingValue, [batchedChanges]); + const modifiedData = 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. From f272eab2546d50281dfc4811262d0120c9b4e3c0 Mon Sep 17 00:00:00 2001 From: Christoph Pader Date: Wed, 27 Sep 2023 10:20:10 +0200 Subject: [PATCH 33/37] add comment --- lib/Onyx.js | 1 + 1 file changed, 1 insertion(+) diff --git a/lib/Onyx.js b/lib/Onyx.js index 9f81ca0de..b8d49f78a 100644 --- a/lib/Onyx.js +++ b/lib/Onyx.js @@ -1148,6 +1148,7 @@ 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) + // 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); // Clean up the write queue, so we don't apply these changes again From f188dd97f62d25d6b466c95c7d9159ba197ff76a Mon Sep 17 00:00:00 2001 From: Christoph Pader Date: Wed, 27 Sep 2023 10:23:11 +0200 Subject: [PATCH 34/37] add another comment --- lib/Onyx.js | 2 ++ 1 file changed, 2 insertions(+) diff --git a/lib/Onyx.js b/lib/Onyx.js index b8d49f78a..0b9bbdeeb 100644 --- a/lib/Onyx.js +++ b/lib/Onyx.js @@ -1156,6 +1156,8 @@ function merge(key, changes) { delete mergeQueuePromise[key]; // After that we merge the batched changes with the existing value + // We can remove null values from the "modifiedData", becasue "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 = applyMerge(existingValue, [batchedChanges], true); // On native platforms we use SQLite which utilises JSON_PATCH to merge changes. From b86d500cff3c39bda5c6444f5bdc6d269109691d Mon Sep 17 00:00:00 2001 From: Blazej Kustra Date: Tue, 26 Sep 2023 14:04:03 +0200 Subject: [PATCH 35/37] Update types for 'Remove null values in nested objects' --- lib/Onyx.d.ts | 12 ++++-------- 1 file changed, 4 insertions(+), 8 deletions(-) diff --git a/lib/Onyx.d.ts b/lib/Onyx.d.ts index d5034939e..c5be53c6b 100644 --- a/lib/Onyx.d.ts +++ b/lib/Onyx.d.ts @@ -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`. @@ -79,14 +78,14 @@ type OnyxUpdate = | { onyxMethod: typeof METHOD.MERGE; key: TKey; - value: PartialDeep; + value: NullishDeep; }; }[OnyxKey] | { [TKey in CollectionKeyBase]: { onyxMethod: typeof METHOD.MERGE_COLLECTION; key: TKey; - value: Record<`${TKey}${string}`, PartialDeep>; + value: Record<`${TKey}${string}`, NullishDeep>; }; }[CollectionKeyBase]; @@ -244,10 +243,7 @@ declare function clear(keysToPreserve?: OnyxKey[]): Promise; * @param collectionKey e.g. `ONYXKEYS.COLLECTION.REPORT` * @param collection Object collection keyed by individual collection member keys and values */ -declare function mergeCollection( - collectionKey: TKey, - collection: Collection>, -): Promise; +declare function mergeCollection(collectionKey: TKey, collection: Collection>): Promise; /** * Insert API responses and lifecycle data into Onyx From 64ada0d02a0db7425846da5519bf64752e09d3ff Mon Sep 17 00:00:00 2001 From: Christoph Pader Date: Thu, 28 Sep 2023 09:29:47 +0200 Subject: [PATCH 36/37] remove underscore where possible --- lib/utils.js | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/lib/utils.js b/lib/utils.js index 692fad75d..858cb2719 100644 --- a/lib/utils.js +++ b/lib/utils.js @@ -40,7 +40,7 @@ function mergeObject(target, source, shouldRemoveNullObjectValues = true) { const key = targetKeys[i]; // If shouldRemoveNullObjectValues is true, we want to remove null values from the merged object - if (shouldRemoveNullObjectValues && (_.isNull(target[key]) || _.isNull(source[key]))) { + if (shouldRemoveNullObjectValues && (target[key] === null || source[key] === null)) { // eslint-disable-next-line no-continue continue; } @@ -56,7 +56,7 @@ function mergeObject(target, source, shouldRemoveNullObjectValues = true) { const key = sourceKeys[i]; // If shouldRemoveNullObjectValues is true, we want to remove null values from the merged object - if (shouldRemoveNullObjectValues && _.isNull(source[key])) { + if (shouldRemoveNullObjectValues && source[key] === null) { // eslint-disable-next-line no-continue continue; } @@ -92,7 +92,7 @@ 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, shouldRemoveNullObjectValues); From b5a0ba87d8b40e48c47f4fdac6d0fcbc41e18423 Mon Sep 17 00:00:00 2001 From: Christoph Pader Date: Thu, 28 Sep 2023 09:52:15 +0200 Subject: [PATCH 37/37] Update lib/Onyx.js Co-authored-by: Marc Glasser --- lib/Onyx.js | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/lib/Onyx.js b/lib/Onyx.js index 501b35519..a685fb326 100644 --- a/lib/Onyx.js +++ b/lib/Onyx.js @@ -1180,7 +1180,7 @@ function merge(key, changes) { delete mergeQueuePromise[key]; // After that we merge the batched changes with the existing value - // We can remove null values from the "modifiedData", becasue "null" implicates that the user wants to remove a value from storage. + // 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);