diff --git a/API.md b/API.md index 0ca16b9d..1102db0a 100644 --- a/API.md +++ b/API.md @@ -25,18 +25,21 @@ If the requested key is a collection, it will return an object with all the coll
disconnect(connectionID, [keyToRemoveFromEvictionBlocklist])

Remove the listener for a react component

-
notifySubscribersOnNextTick(key, value, [canUpdateSubscriber])
-

This method mostly exists for historical reasons as this library was initially designed without a memory cache and one was added later. -For this reason, Onyx works more similar to what you might expect from a native AsyncStorage with reads, writes, etc all becoming -available async. Since we have code in our main applications that might expect things to work this way it's not safe to change this -behavior just yet.

+
maybeFlushBatchUpdates() ⇒ Promise
+

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.

-
notifyCollectionSubscribersOnNextTick(key, value)
+
scheduleSubscriberUpdate(key, value, [canUpdateSubscriber]) ⇒ Promise
+

Schedules an update that will be appended to the macro task queue (so it doesn't update the subscribers immediately).

+
+
scheduleNotifyCollectionSubscribers(key, value) ⇒ Promise

This method is similar to notifySubscribersOnNextTick but it is built for working specifically with collections so that keysChanged() is triggered for the collection and not keyChanged(). If this was not done, then the subscriber callbacks receive the data in a different format than they normally expect and it breaks code.

-
broadcastUpdate(key, value, hasChanged, method)
+
broadcastUpdate(key, value, hasChanged, method) ⇒ Promise

Notifys subscribers and writes current value to cache

hasPendingMergeForKey(key) ⇒ Boolean
@@ -154,6 +157,7 @@ Subscribes a react component's state directly to a store key | [mapping.initWithStoredValues] | Boolean | If set to false, then no data will be prefilled into the component | | [mapping.waitForCollectionCallback] | Boolean | If set to true, it will return the entire collection to the callback as a single object | | [mapping.selector] | function | THIS PARAM IS ONLY USED WITH withOnyx(). If included, this will be used to subscribe to a subset of an Onyx key's data. The sourceData and withOnyx state are passed to the selector and should return the simplified data. Using this setting on `withOnyx` can have very positive performance benefits because the component will only re-render when the subset of data changes. Otherwise, any change of data on any property would normally cause the component to re-render (and that can be expensive from a performance standpoint). | +| [mapping.initialValue] | String \| Number \| Boolean \| Object | THIS PARAM IS ONLY USED WITH withOnyx(). If included, this will be passed to the component so that something can be rendered while data is being fetched from the DB. Note that it will not cause the component to have the loading prop set to true. | | **Example** ```js @@ -178,13 +182,19 @@ Remove the listener for a react component ```js Onyx.disconnect(connectionID); ``` - + + +## maybeFlushBatchUpdates() ⇒ Promise +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. + +**Kind**: global function + -## notifySubscribersOnNextTick(key, value, [canUpdateSubscriber]) -This method mostly exists for historical reasons as this library was initially designed without a memory cache and one was added later. -For this reason, Onyx works more similar to what you might expect from a native AsyncStorage with reads, writes, etc all becoming -available async. Since we have code in our main applications that might expect things to work this way it's not safe to change this -behavior just yet. +## scheduleSubscriberUpdate(key, value, [canUpdateSubscriber]) ⇒ Promise +Schedules an update that will be appended to the macro task queue (so it doesn't update the subscribers immediately). **Kind**: global function @@ -196,11 +206,11 @@ behavior just yet. **Example** ```js -notifySubscribersOnNextTick(key, value, subscriber => subscriber.initWithStoredValues === false) +scheduleSubscriberUpdate(key, value, subscriber => subscriber.initWithStoredValues === false) ``` - + -## notifyCollectionSubscribersOnNextTick(key, value) +## scheduleNotifyCollectionSubscribers(key, value) ⇒ Promise This method is similar to notifySubscribersOnNextTick but it is built for working specifically with collections so that keysChanged() is triggered for the collection and not keyChanged(). If this was not done, then the subscriber callbacks receive the data in a different format than they normally expect and it breaks code. @@ -214,7 +224,7 @@ subscriber callbacks receive the data in a different format than they normally e -## broadcastUpdate(key, value, hasChanged, method) +## broadcastUpdate(key, value, hasChanged, method) ⇒ Promise Notifys subscribers and writes current value to cache **Kind**: global function diff --git a/README.md b/README.md index b8bec05c..eacb3b8c 100644 --- a/README.md +++ b/README.md @@ -135,7 +135,34 @@ export default withOnyx({ })(App); ``` -It is preferable to use the HOC over `Onyx.connect()` in React code as `withOnyx()` will delay the rendering of the wrapped component until all keys have been accessed and made available. +While `Onyx.connect()` gives you more control on how your component reacts as data is fetched from disk, `withOnyx()` will delay the rendering of the wrapped component until all keys/entities have been fetched and passed to the component, this can be convenient for simple cases. This however, can really delay your application if many entities are connected to the same component, you can pass an `initialValue` to each key to allow Onyx to eagerly render your component with this value. + +```javascript +export default withOnyx({ + session: { + key: ONYXKEYS.SESSION, + initialValue: {} + }, +})(App); +``` + +Additionally, if your component has many keys/entities when your component will mount but will receive many updates as data is fetched from DB and passed down to it, as every key that gets fetched will trigger a `setState` on the `withOnyx` HOC. This might cause re-renders on the initial mounting, preventing the component from mounting/rendering in reasonable time, making your app feel slow and even delaying animations. You can workaround this by passing an additional object with the `shouldDelayUpdates` property set to true. Onyx will then put all the updates in a queue until you decide when then should be applied, the component will receive a function `markReadyForHydration`. A good place to call this function is on the `onLayout` method, which gets triggered after your component has been rendered. + +```javascript +const App = ({session, markReadyForHydration}) => ( + markReadyForHydration()}> + {session.token ? Logged in : Logged out } + +); + +// Second argument to funciton is `shouldDelayUpdates` +export default withOnyx({ + session: { + key: ONYXKEYS.SESSION, + initialValue: {} + }, +}, true)(App); +``` ### Dependent Onyx Keys and withOnyx() Some components need to subscribe to multiple Onyx keys at once and sometimes, one key might rely on the data from another key. This is similar to a JOIN in SQL. diff --git a/lib/Onyx.js b/lib/Onyx.js index 19fbe9de..d4d329e0 100644 --- a/lib/Onyx.js +++ b/lib/Onyx.js @@ -5,10 +5,9 @@ import * as Logger from './Logger'; import cache from './OnyxCache'; import * as Str from './Str'; import createDeferredTask from './createDeferredTask'; -import fastMerge from './fastMerge'; import * as PerformanceUtils from './metrics/PerformanceUtils'; import Storage from './storage'; -import Utils from './utils'; +import utils from './utils'; import unstable_batchedUpdates from './batch'; // Method constants @@ -415,7 +414,7 @@ function keysChanged(collectionKey, partialCollection, notifyRegularSubscibers = // If the subscriber has a selector, then the component's state must only be updated with the data // returned by the selector. if (subscriber.selector) { - subscriber.withOnyxInstance.setState((prevState) => { + subscriber.withOnyxInstance.setStateProxy((prevState) => { const previousData = prevState[subscriber.statePropertyName]; const newData = reduceCollectionWithSelector(cachedCollection, subscriber.selector, subscriber.withOnyxInstance.state); @@ -429,7 +428,7 @@ function keysChanged(collectionKey, partialCollection, notifyRegularSubscibers = continue; } - subscriber.withOnyxInstance.setState((prevState) => { + subscriber.withOnyxInstance.setStateProxy((prevState) => { const finalCollection = _.clone(prevState[subscriber.statePropertyName] || {}); const dataKeys = _.keys(partialCollection); for (let j = 0; j < dataKeys.length; j++) { @@ -458,7 +457,7 @@ function keysChanged(collectionKey, partialCollection, notifyRegularSubscibers = // returned by the selector and the state should only change when the subset of data changes from what // it was previously. if (subscriber.selector) { - subscriber.withOnyxInstance.setState((prevState) => { + subscriber.withOnyxInstance.setStateProxy((prevState) => { const prevData = prevState[subscriber.statePropertyName]; const newData = getSubsetOfData(cachedCollection[subscriber.key], subscriber.selector, subscriber.withOnyxInstance.state); if (!deepEqual(prevData, newData)) { @@ -473,9 +472,14 @@ function keysChanged(collectionKey, partialCollection, notifyRegularSubscibers = continue; } - subscriber.withOnyxInstance.setState((prevState) => { + subscriber.withOnyxInstance.setStateProxy((prevState) => { const data = cachedCollection[subscriber.key]; const previousData = prevState[subscriber.statePropertyName]; + + // Avoids triggering unnecessary re-renders when feeding empty objects + if (utils.areObjectsEmpty(data, previousData)) { + return null; + } if (data === previousData) { return null; } @@ -548,7 +552,7 @@ function keyChanged(key, data, canUpdateSubscriber, notifyRegularSubscibers = tr // If the subscriber has a selector, then the consumer of this data must only be given the data // returned by the selector and only when the selected data has changed. if (subscriber.selector) { - subscriber.withOnyxInstance.setState((prevState) => { + subscriber.withOnyxInstance.setStateProxy((prevState) => { const prevData = prevState[subscriber.statePropertyName]; const newData = { [key]: getSubsetOfData(data, subscriber.selector, subscriber.withOnyxInstance.state), @@ -568,7 +572,7 @@ function keyChanged(key, data, canUpdateSubscriber, notifyRegularSubscibers = tr continue; } - subscriber.withOnyxInstance.setState((prevState) => { + subscriber.withOnyxInstance.setStateProxy((prevState) => { const collection = prevState[subscriber.statePropertyName] || {}; const newCollection = { ...collection, @@ -585,7 +589,7 @@ function keyChanged(key, data, canUpdateSubscriber, notifyRegularSubscibers = tr // If the subscriber has a selector, then the component's state must only be updated with the data // returned by the selector and only if the selected data has changed. if (subscriber.selector) { - subscriber.withOnyxInstance.setState((prevState) => { + subscriber.withOnyxInstance.setStateProxy((prevState) => { const previousValue = getSubsetOfData(prevState[subscriber.statePropertyName], subscriber.selector, subscriber.withOnyxInstance.state); const newValue = getSubsetOfData(data, subscriber.selector, subscriber.withOnyxInstance.state); if (!deepEqual(previousValue, newValue)) { @@ -599,8 +603,13 @@ function keyChanged(key, data, canUpdateSubscriber, notifyRegularSubscibers = tr } // If we did not match on a collection key then we just set the new data to the state property - subscriber.withOnyxInstance.setState((prevState) => { + subscriber.withOnyxInstance.setStateProxy((prevState) => { const previousData = prevState[subscriber.statePropertyName]; + + // Avoids triggering unnecessary re-renders when feeding empty objects + if (utils.areObjectsEmpty(data, previousData)) { + return null; + } if (previousData === data) { return null; } @@ -728,6 +737,9 @@ function getCollectionDataAndSendAsObject(matchingKeys, mapping) { * The sourceData and withOnyx state are passed to the selector and should return the simplified data. Using this setting on `withOnyx` can have very positive * performance benefits because the component will only re-render when the subset of data changes. Otherwise, any change of data on any property would normally * cause the component to re-render (and that can be expensive from a performance standpoint). + * @param {String | Number | Boolean | Object} [mapping.initialValue] THIS PARAM IS ONLY USED WITH withOnyx(). + * If included, this will be passed to the component so that something can be rendered while data is being fetched from the DB. + * Note that it will not cause the component to have the loading prop set to true. | * @returns {Number} an ID to use when calling disconnect */ function connect(mapping) { @@ -1008,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.`); } - const valueWithNullRemoved = Utils.removeNullObjectValues(value); + const valueWithNullRemoved = utils.removeNullObjectValues(value); const hasChanged = cache.hasValueChanged(key, valueWithNullRemoved); @@ -1078,7 +1090,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) => fastMerge(modifiedData, change), + return _.reduce(changes, (modifiedData, change) => utils.fastMerge(modifiedData, change), existingValue || {}); } @@ -1127,14 +1139,14 @@ function merge(key, changes) { delete mergeQueuePromise[key]; // After that we merge the batched changes with the existing value - const modifiedData = Utils.removeNullObjectValues(applyMerge(existingValue, [batchedChanges])); + const modifiedData = utils.removeNullObjectValues(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. if (!existingValue) { - batchedChanges = Utils.removeNullObjectValues(batchedChanges); + batchedChanges = utils.removeNullObjectValues(batchedChanges); } const hasChanged = cache.hasValueChanged(key, modifiedData); @@ -1168,7 +1180,7 @@ function initializeWithDefaultKeyStates() { .then((pairs) => { const asObject = _.object(pairs); - const merged = fastMerge(asObject, defaultKeyStates); + const merged = utils.fastMerge(asObject, defaultKeyStates); cache.merge(merged); _.each(merged, (val, key) => keyChanged(key, val)); }); diff --git a/lib/OnyxCache.js b/lib/OnyxCache.js index 1500e47e..e7f6f58f 100644 --- a/lib/OnyxCache.js +++ b/lib/OnyxCache.js @@ -1,6 +1,6 @@ import _ from 'underscore'; import {deepEqual} from 'fast-equals'; -import fastMerge from './fastMerge'; +import utils from './utils'; const isDefined = _.negate(_.isUndefined); @@ -119,7 +119,7 @@ class OnyxCache { // lodash adds a small overhead so we don't use it here // eslint-disable-next-line prefer-object-spread, rulesdir/prefer-underscore-method - this.storageMap = Object.assign({}, fastMerge(this.storageMap, data)); + this.storageMap = Object.assign({}, utils.fastMerge(this.storageMap, data)); const storageKeys = this.getAllKeys(); const mergedKeys = _.keys(data); diff --git a/lib/storage/__mocks__/index.js b/lib/storage/__mocks__/index.js index 881028ad..9f228219 100644 --- a/lib/storage/__mocks__/index.js +++ b/lib/storage/__mocks__/index.js @@ -1,5 +1,5 @@ import _ from 'underscore'; -import fastMerge from '../../fastMerge'; +import utils from '../../utils'; let storageMapInternal = {}; @@ -27,7 +27,7 @@ const idbKeyvalMock = { _.forEach(pairs, ([key, value]) => { const existingValue = storageMapInternal[key]; const newValue = _.isObject(existingValue) - ? fastMerge(existingValue, value) : value; + ? utils.fastMerge(existingValue, value) : value; set(key, newValue); }); diff --git a/lib/storage/providers/IDBKeyVal.js b/lib/storage/providers/IDBKeyVal.js index e82dd74c..c72ce750 100644 --- a/lib/storage/providers/IDBKeyVal.js +++ b/lib/storage/providers/IDBKeyVal.js @@ -11,8 +11,7 @@ import { promisifyRequest, } from 'idb-keyval'; import _ from 'underscore'; -import fastMerge from '../../fastMerge'; -import Utils from '../../utils'; +import utils from '../../utils'; // We don't want to initialize the store while the JS bundle loads as idb-keyval will try to use global.indexedDB // which might not be available in certain environments that load the bundle (e.g. electron main process). @@ -56,8 +55,8 @@ const provider = { return getValues.then((values) => { const upsertMany = _.map(pairs, ([key, value], index) => { const prev = values[index]; - const newValue = _.isObject(prev) ? fastMerge(prev, value) : value; - return promisifyRequest(store.put(Utils.removeNullObjectValues(newValue), key)); + const newValue = _.isObject(prev) ? utils.fastMerge(prev, value) : value; + return promisifyRequest(store.put(utils.removeNullObjectValues(newValue), key)); }); return Promise.all(upsertMany); }); diff --git a/lib/utils.js b/lib/utils.js index 681898be..14005712 100644 --- a/lib/utils.js +++ b/lib/utils.js @@ -1,4 +1,77 @@ -import _ from 'underscore'; +import * as _ from 'underscore'; + +function areObjectsEmpty(a, b) { + return ( + typeof a === 'object' + && typeof b === 'object' + && _.isEmpty(a) + && _.isEmpty(b) + ); +} + +// 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) { + // 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. @@ -20,4 +93,5 @@ function removeNullObjectValues(value) { return objectWithoutNullObjectValues; } -export default {removeNullObjectValues}; +export default {removeNullObjectValues, areObjectsEmpty, fastMerge}; + diff --git a/lib/withOnyx.d.ts b/lib/withOnyx.d.ts index cf377d30..91838dfe 100644 --- a/lib/withOnyx.d.ts +++ b/lib/withOnyx.d.ts @@ -148,6 +148,7 @@ declare function withOnyx( | OnyxPropMapping | OnyxPropCollectionMapping; }, + shouldDelayUpdates?: boolean, ): (component: React.ComponentType) => React.ComponentType>; export default withOnyx; diff --git a/lib/withOnyx.js b/lib/withOnyx.js index 2eaba42f..97615284 100644 --- a/lib/withOnyx.js +++ b/lib/withOnyx.js @@ -8,6 +8,7 @@ import React from 'react'; import _ from 'underscore'; import Onyx from './Onyx'; import * as Str from './Str'; +import utils from './utils'; /** * Returns the display name of a component @@ -19,7 +20,7 @@ function getDisplayName(component) { return component.displayName || component.name || 'Component'; } -export default function (mapOnyxToState) { +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) .omit(config => config.initWithStoredValues === false) @@ -28,37 +29,51 @@ export default function (mapOnyxToState) { return (WrappedComponent) => { const displayName = getDisplayName(WrappedComponent); class withOnyx extends React.Component { + pendingSetStates = []; + constructor(props) { super(props); - + this.shouldDelayUpdates = shouldDelayUpdates; this.setWithOnyxState = this.setWithOnyxState.bind(this); + this.flushPendingSetStates = this.flushPendingSetStates.bind(this); // This stores all the Onyx connection IDs to be used when the component unmounts so everything can be // disconnected. It is a key value store with the format {[mapping.key]: connectionID}. this.activeConnectionIDs = {}; - const cachedState = _.reduce(mapOnyxToState, (resultObj, mapping, propertyName) => { - const key = Str.result(mapping.key, props); - const value = Onyx.tryGetCachedValue(key, mapping); - - /** - * If we have a pending merge for a key it could mean that data is being set via Onyx.merge() and someone expects a component to have this data immediately. - * - * @example - * - * Onyx.merge('report_123', value); - * Navigation.navigate(route); // Where "route" expects the "value" to be available immediately once rendered. - * - * In reality, Onyx.merge() will only update the subscriber after all merges have been batched and the previous value is retrieved via a get() (returns a promise). - * So, we won't use the cache optimization here as it will lead us to arbitrarily defer various actions in the application code. - */ - if (value !== undefined && !Onyx.hasPendingMergeForKey(key)) { - // eslint-disable-next-line no-param-reassign - resultObj[propertyName] = value; - } + const cachedState = _.reduce( + mapOnyxToState, + (resultObj, mapping, propertyName) => { + const key = Str.result(mapping.key, props); + let value = Onyx.tryGetCachedValue(key, mapping); + if (!value && mapping.initialValue) { + value = mapping.initialValue; + } - return resultObj; - }, {}); + /** + * If we have a pending merge for a key it could mean that data is being set via Onyx.merge() and someone expects a component to have this data immediately. + * + * @example + * + * Onyx.merge('report_123', value); + * Navigation.navigate(route); // Where "route" expects the "value" to be available immediately once rendered. + * + * In reality, Onyx.merge() will only update the subscriber after all merges have been batched and the previous value is retrieved via a get() (returns a promise). + * So, we won't use the cache optimization here as it will lead us to arbitrarily defer various actions in the application code. + */ + if ( + (value !== undefined + && !Onyx.hasPendingMergeForKey(key)) + || mapping.allowStaleData + ) { + // eslint-disable-next-line no-param-reassign + resultObj[propertyName] = value; + } + + return resultObj; + }, + {}, + ); // If we have all the data we need, then we can render the component immediately cachedState.loading = _.size(cachedState) < requiredKeysForInit.length; @@ -101,47 +116,86 @@ export default function (mapOnyxToState) { }); } + setStateProxy(modifier) { + if (this.shouldDelayUpdates) { + this.pendingSetStates.push(modifier); + } else { + this.setState(modifier); + } + } + /** - * This method is used externally by sendDataToConnection to prevent unnecessary renders while a component - * still in a loading state. The temporary initial state is saved to the component instance and setState() + * This method is used by the internal raw Onyx `sendDataToConnection`, it is designed to prevent unnecessary renders while a component + * still in a "loading" (read "mounting") state. The temporary initial state is saved to the HOC instance and setState() * only called once all the necessary data has been collected. * + * There is however the possibility the component could have been updated by a call to setState() + * before the data was "initially" collected. A race condition. + * For example some update happened on some key, while onyx was still gathering the initial hydration data. + * This update is disptached directly to setStateProxy and therefore the component has the most up-to-date data + * + * This is a design flaw in Onyx itself as dispatching updates before initial hydration is not a correct event flow. + * We however need to workaround this issue in the HOC. The addition of initialValue makes things even more complex, + * since you cannot be really sure if the component has been updated before or after the initial hydration. Therefore if + * initialValue is there, we just check if the update is different than that and then try to handle it as best as we can. + * * @param {String} statePropertyName * @param {*} val */ setWithOnyxState(statePropertyName, val) { - // We might have loaded the values for the onyx keys/mappings already from the cache. - // In case we were able to load all the values upfront, the loading state will be false. - // However, Onyx.js will always call setWithOnyxState, as it doesn't know that this implementation - // already loaded the values from cache. Thus we have to check whether the value has changed - // before we set the state to prevent unnecessary renders. const prevValue = this.state[statePropertyName]; - if (!this.state.loading && prevValue === val) { - return; - } + // If the component is not loading (read "mounting"), then we can just update the state if (!this.state.loading) { - this.setState({[statePropertyName]: val}); + // Performance optimization, do not trigger update with same values + if (prevValue === val || utils.areObjectsEmpty(prevValue, val)) { + return; + } + + this.setStateProxy({[statePropertyName]: val}); return; } this.tempState[statePropertyName] = val; - // All state keys should exist and at least have a value of null - if (_.some(requiredKeysForInit, key => _.isUndefined(this.tempState[key]))) { + // If some key does not have a value yet, do not update the state yet + const tempStateIsMissingKey = _.some(requiredKeysForInit, key => _.isUndefined(this.tempState[key])); + if (tempStateIsMissingKey) { return; } - // Leave untouched previous state to avoid data loss during pre-load updates. - // This handles case when setState was called before the setWithOnyxState. - // For example, when an Onyx property was updated by keyChanged before the call of the setWithOnyxState. + const stateUpdate = {...this.tempState}; + delete this.tempState; + + // Full of hacky workarounds to prevent the race condition described above. this.setState((prevState) => { - const remainingTempState = _.omit(this.tempState, _.keys(prevState)); + const finalState = _.reduce(stateUpdate, (result, value, key) => { + if (key === 'loading') { + return result; + } - return ({...remainingTempState, loading: false}); - }); + const initialValue = mapOnyxToState[key].initialValue; - delete this.tempState; + // If initialValue is there and the state contains something different it means + // an update has already been received and we can discard the value we are trying to hydrate + if (!_.isUndefined(initialValue) && !_.isUndefined(prevState[key]) && prevState[key] !== initialValue) { + // eslint-disable-next-line no-param-reassign + result[key] = prevState[key]; + + // if value is already there (without initial value) then we can discard the value we are trying to hydrate + } else if (!_.isUndefined(prevState[key])) { + // eslint-disable-next-line no-param-reassign + result[key] = prevState[key]; + } else { + // eslint-disable-next-line no-param-reassign + result[key] = value; + } + return result; + }, {}); + + finalState.loading = false; + return finalState; + }); } /** @@ -196,7 +250,23 @@ export default function (mapOnyxToState) { }); } + flushPendingSetStates() { + if (!this.shouldDelayUpdates) { + return; + } + + this.shouldDelayUpdates = false; + + this.pendingSetStates.forEach((modifier) => { + this.setState(modifier); + }); + this.pendingSetStates = []; + } + render() { + // Remove any null values so that React replaces them with default props + const propsToPass = _.omit(this.props, _.isNull); + if (this.state.loading) { return null; } @@ -204,14 +274,12 @@ export default function (mapOnyxToState) { // Remove any internal state properties used by withOnyx // that should not be passed to a wrapped component let stateToPass = _.omit(this.state, 'loading'); - stateToPass = _.omit(stateToPass, value => _.isNull(value)); - - // Remove any null values so that React replaces them with default props - const propsToPass = _.omit(this.props, value => _.isNull(value)); + stateToPass = _.omit(stateToPass, _.isNull); // Spreading props and state is necessary in an HOC where the data cannot be predicted return ( {}, + markReadyForHydration: () => {}, }; -function ViewWithCollections(props) { +const ViewWithCollections = React.forwardRef((props, ref) => { + useImperativeHandle(ref, () => ({ + markReadyForHydration: props.markReadyForHydration, + })); + props.onRender(props); return ( @@ -30,7 +36,7 @@ function ViewWithCollections(props) { ))} ); -} +}); ViewWithCollections.propTypes = propTypes; ViewWithCollections.defaultProps = defaultProps; diff --git a/tests/unit/withOnyxTest.js b/tests/unit/withOnyxTest.js index ffa094db..1f36cfe0 100644 --- a/tests/unit/withOnyxTest.js +++ b/tests/unit/withOnyxTest.js @@ -151,7 +151,8 @@ describe('withOnyxTest', () => { }, })(ViewWithCollections); const onRender = jest.fn(); - render(); + const markReadyForHydration = jest.fn(); + render(); return waitForPromisesToResolve() .then(() => Onyx.mergeCollection(ONYX_KEYS.COLLECTION.TEST_KEY, { test_1: {list: [1, 2]}, @@ -162,7 +163,7 @@ describe('withOnyxTest', () => { .then(() => { expect(onRender).toHaveBeenCalledTimes(3); expect(onRender).toHaveBeenLastCalledWith({ - collections: {}, onRender, testObject: {isDefaultProp: true}, text: {list: [7]}, + collections: {}, markReadyForHydration, onRender, testObject: {isDefaultProp: true}, text: {list: [7]}, }); }); }); @@ -175,7 +176,8 @@ describe('withOnyxTest', () => { }, })(ViewWithCollections); const onRender = jest.fn(); - render(); + const markReadyForHydration = jest.fn(); + render(); return waitForPromisesToResolve() .then(() => Onyx.mergeCollection(ONYX_KEYS.COLLECTION.TEST_KEY, {test_4: {ID: 456}, test_5: {ID: 567}})) .then(() => Onyx.mergeCollection(ONYX_KEYS.COLLECTION.TEST_KEY, { @@ -186,7 +188,7 @@ describe('withOnyxTest', () => { .then(() => { expect(onRender).toHaveBeenCalledTimes(3); expect(onRender).toHaveBeenLastCalledWith({ - collections: {}, onRender, testObject: {isDefaultProp: true}, text: {ID: 456, Name: 'Test4'}, + collections: {}, markReadyForHydration, onRender, testObject: {isDefaultProp: true}, text: {ID: 456, Name: 'Test4'}, }); }); }); @@ -243,6 +245,7 @@ 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'}); return waitForPromisesToResolve() @@ -259,11 +262,11 @@ describe('withOnyxTest', () => { }, }), )(ViewWithCollections); - render(); + render(); }) .then(() => { expect(onRender).toHaveBeenLastCalledWith({ - collections: {}, onRender, testObject: {id: 1}, testThing: 'Test', + collections: {}, markReadyForHydration, onRender, testObject: {id: 1}, testThing: 'Test', }); }); }); @@ -272,6 +275,9 @@ describe('withOnyxTest', () => { const onRender1 = jest.fn(); const onRender2 = jest.fn(); const onRender3 = jest.fn(); + const markReadyForHydration1 = jest.fn(); + const markReadyForHydration2 = jest.fn(); + const markReadyForHydration3 = jest.fn(); // Given there is a collection with three simple items in it Onyx.mergeCollection(ONYX_KEYS.COLLECTION.TEST_KEY, { @@ -288,21 +294,21 @@ describe('withOnyxTest', () => { key: `${ONYX_KEYS.COLLECTION.TEST_KEY}1`, }, })(ViewWithCollections); - render(); + render(); const TestComponentWithOnyx2 = withOnyx({ testObject: { key: `${ONYX_KEYS.COLLECTION.TEST_KEY}2`, }, })(ViewWithCollections); - render(); + render(); const TestComponentWithOnyx3 = withOnyx({ testObject: { key: `${ONYX_KEYS.COLLECTION.TEST_KEY}3`, }, })(ViewWithCollections); - render(); + render(); }) // When a single item in the collection is updated with mergeCollection() @@ -315,19 +321,28 @@ describe('withOnyxTest', () => { // Note: each component is rendered twice. Once when it is initially rendered, and then again // when the collection is updated. That's why there are two checks here for each component. expect(onRender1).toHaveBeenCalledTimes(2); - expect(onRender1).toHaveBeenNthCalledWith(1, {collections: {}, onRender: onRender1, testObject: {ID: 1}}); - expect(onRender1).toHaveBeenNthCalledWith(2, {collections: {}, onRender: onRender1, testObject: {ID: 1, newProperty: 'yay'}}); + expect(onRender1).toHaveBeenNthCalledWith(1, { + collections: {}, markReadyForHydration: markReadyForHydration1, onRender: onRender1, testObject: {ID: 1}, + }); + expect(onRender1).toHaveBeenNthCalledWith(2, { + collections: {}, markReadyForHydration: markReadyForHydration1, onRender: onRender1, testObject: {ID: 1, newProperty: 'yay'}, + }); expect(onRender2).toHaveBeenCalledTimes(1); - expect(onRender2).toHaveBeenNthCalledWith(1, {collections: {}, onRender: onRender2, testObject: {ID: 2}}); + expect(onRender2).toHaveBeenNthCalledWith(1, { + collections: {}, markReadyForHydration: markReadyForHydration2, onRender: onRender2, testObject: {ID: 2}, + }); expect(onRender3).toHaveBeenCalledTimes(1); - expect(onRender3).toHaveBeenNthCalledWith(1, {collections: {}, onRender: onRender3, testObject: {ID: 3}}); + expect(onRender3).toHaveBeenNthCalledWith(1, { + collections: {}, markReadyForHydration: markReadyForHydration3, onRender: onRender3, testObject: {ID: 3}, + }); }); }); it('mergeCollection should merge previous props correctly to the new state', () => { const onRender = jest.fn(); + const markReadyForHydration = jest.fn(); // Given there is a collection with a simple item in it that has a `number` property set to 1 Onyx.mergeCollection(ONYX_KEYS.COLLECTION.TEST_KEY, { @@ -342,7 +357,7 @@ describe('withOnyxTest', () => { key: `${ONYX_KEYS.COLLECTION.TEST_KEY}1`, }, })(ViewWithCollections); - render(); + render(); }) // When the `number` property is updated using mergeCollection to be 2 @@ -354,8 +369,12 @@ describe('withOnyxTest', () => { // The first time it will render with number === 1 // The second time it will render with number === 2 expect(onRender).toHaveBeenCalledTimes(2); - expect(onRender).toHaveBeenNthCalledWith(1, {collections: {}, onRender, testObject: {ID: 1, number: 1}}); - expect(onRender).toHaveBeenNthCalledWith(2, {collections: {}, onRender, testObject: {ID: 1, number: 2}}); + expect(onRender).toHaveBeenNthCalledWith(1, { + collections: {}, markReadyForHydration, onRender, testObject: {ID: 1, number: 1}, + }); + expect(onRender).toHaveBeenNthCalledWith(2, { + collections: {}, markReadyForHydration, onRender, testObject: {ID: 1, number: 2}, + }); }); }); @@ -392,4 +411,69 @@ describe('withOnyxTest', () => { expect(onRender.mock.calls[1][0].simple).toBe('long_string'); }); }); + + it('initialValue should be fed into component', () => { + const onRender = jest.fn(); + const markReadyForHydration = jest.fn(); + + return waitForPromisesToResolve() + .then(() => { + // When a component subscribes to the simple key + const TestComponentWithOnyx = withOnyx({ + simple: { + key: ONYX_KEYS.SIMPLE_KEY, + initialValue: 'initialValue', + }, + })(ViewWithCollections); + render(); + }) + .then(() => { + // Then the component subscribed to the modified item should only render once + expect(onRender).toHaveBeenCalledTimes(1); + expect(onRender).toHaveBeenLastCalledWith({ + collections: {}, markReadyForHydration, onRender, testObject: {isDefaultProp: true}, simple: 'initialValue', + }); + }); + }); + + it('shouldDelayUpdates + initialValue does not feed data into component until marked ready', () => { + const onRender = jest.fn(); + const ref = React.createRef(); + + return waitForPromisesToResolve() + .then(() => { + // When a component subscribes to the simple key + const TestComponentWithOnyx = withOnyx({ + simple: { + key: ONYX_KEYS.SIMPLE_KEY, + initialValue: 'initialValue', + }, + }, + true)(ViewWithCollections); + + render(); + }) + + // component mounted with the initial value, while updates are queueing + .then(() => Onyx.merge(ONYX_KEYS.SIMPLE_KEY, 'string')) + .then(() => { + expect(onRender).toHaveBeenCalledTimes(1); + expect(onRender.mock.calls[0][0].simple).toBe('initialValue'); + + // just to test we change the value + return Onyx.merge(ONYX_KEYS.SIMPLE_KEY, 'long_string'); + }) + .then(() => { + // Component still has not updated + expect(onRender).toHaveBeenCalledTimes(1); + + // We can now tell component to update + ref.current.markReadyForHydration(); + }) + .then(() => { + // Component still has not updated + expect(onRender).toHaveBeenCalledTimes(4); + expect(onRender.mock.calls[3][0].simple).toBe('long_string'); + }); + }); });