-
Notifications
You must be signed in to change notification settings - Fork 3.1k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Onyx derived values #56891
Onyx derived values #56891
Changes from 18 commits
f57151d
0733f7d
f05f33c
8a576be
502b3c6
4c343ef
fe56c68
f505c0b
cb92cfc
4817676
05e17ff
7de6fc5
1cb4cbe
d3e7010
1ab4d35
a922c34
c045ef2
867f5e4
b96d4a6
0d08483
27ece8d
25bdbf4
aed0d95
446fead
2a76669
0a91c78
d226365
c040ec0
140c793
b9f1d50
88ba460
9e57693
5c609d7
2576aeb
bf9c5e6
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -854,3 +854,14 @@ In order to compile a production iOS build, run `npm run ios-build`, this will g | |
|
||
#### Local production build the Android app | ||
To build an APK to share run (e.g. via Slack), run `npm run android-build`, this will generate a new APK in the `android/app` folder. | ||
|
||
# Onyx derived values | ||
Onyx derived values are special Onyx keys which contain values derived from other Onyx values. These are available as a performance optimization, so that if the result of a common computation of Onyx values is needed in many places across the app, the computation can be done only as needed in a centralized location, and then shared across the app. Once created, Onyx derived values are stored and consumed just like any other Onyx value. | ||
|
||
## Creating new Onyx derived values | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. It would be nice to add a little bit more to this section to explain why you should or shouldn't use derived values. Would you mind adding a bit of details like:
|
||
1. Add the new Onyx key. The keys for Onyx derived values are stored in `ONYXKEYS.ts`, in the `ONYXKEYS.DERIVED` object. | ||
2. Declare the type for the derived value in `ONYXKEYS.ts`, in the `OnyxDerivedValuesMapping` type. | ||
3. Add the derived value config to `ONYX_DERIVED_VALUES` in `src/libs/OnyxDerived.ts`. A derived value config is defined by: | ||
1. The Onyx key for the derived value | ||
2. An array of dependent Onyx keys (which can be any keys, not including the one from the previous step. Including other derived values!) | ||
3. A `compute` function, which takes an array of dependent Onyx values (in the same order as the array of keys from the previous step), and returns a value matching the type you declared in `OnyxDerivedValuesMapping` |
Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -1,19 +1,18 @@ | ||
import type {ComponentType, ForwardedRef, ReactElement, RefAttributes} from 'react'; | ||
import React from 'react'; | ||
import {View} from 'react-native'; | ||
import type {SetOptional} from 'type-fest'; | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. These changes were only necessary because I upgraded |
||
import useThemeStyles from '@hooks/useThemeStyles'; | ||
import getComponentDisplayName from '@libs/getComponentDisplayName'; | ||
|
||
type WithToggleVisibilityViewProps = { | ||
/** Whether the content is visible. */ | ||
isVisible: boolean; | ||
isVisible?: boolean; | ||
}; | ||
|
||
export default function withToggleVisibilityView<TProps extends WithToggleVisibilityViewProps, TRef>( | ||
export default function withToggleVisibilityView<TProps, TRef>( | ||
WrappedComponent: ComponentType<TProps & RefAttributes<TRef>>, | ||
): (props: TProps & RefAttributes<TRef>) => ReactElement | null { | ||
function WithToggleVisibilityView({isVisible = false, ...rest}: SetOptional<TProps, 'isVisible'>, ref: ForwardedRef<TRef>) { | ||
): (props: TProps & WithToggleVisibilityViewProps & RefAttributes<TRef>) => ReactElement | null { | ||
function WithToggleVisibilityView({isVisible = false, ...rest}: WithToggleVisibilityViewProps, ref: ForwardedRef<TRef>) { | ||
const styles = useThemeStyles(); | ||
return ( | ||
<View | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,183 @@ | ||
/** | ||
* This file contains logic for derived Onyx keys. The idea behind derived keys is that if there is a common computation | ||
* that we're doing in many places across the app to derive some value from multiple Onyx values, we can move that | ||
* computation into this file, run it only once, and then share it across the app by storing the result of that computation in Onyx. | ||
* | ||
* The primary purpose is to optimize performance by reducing redundant computations. More info can be found in the README. | ||
*/ | ||
import type {OnyxEntry} from 'react-native-onyx'; | ||
import Onyx from 'react-native-onyx'; | ||
import OnyxUtils from 'react-native-onyx/dist/OnyxUtils'; | ||
import type {NonEmptyTuple, ValueOf} from 'type-fest'; | ||
import Log from '@libs/Log'; | ||
import {isThread} from '@libs/ReportUtils'; | ||
import CONST from '@src/CONST'; | ||
import type {GetOnyxTypeForKey, OnyxDerivedKey, OnyxDerivedValuesMapping, OnyxKey} from '@src/ONYXKEYS'; | ||
import ONYXKEYS from '@src/ONYXKEYS'; | ||
import type AssertTypesEqual from '@src/types/utils/AssertTypesEqual'; | ||
import ObjectUtils from '@src/types/utils/ObjectUtils'; | ||
import type SymmetricDifference from '@src/types/utils/SymmetricDifference'; | ||
|
||
/** | ||
* A derived value configuration describes: | ||
* - a tuple of Onyx keys to subscribe to (dependencies), | ||
* - a compute function that derives a value from the dependent Onyx values. | ||
* The compute function receives a single argument that's a tuple of the onyx values for the declared dependencies. | ||
* For example, if your dependencies are `['report_', 'account'], then compute will receive a [OnyxCollection<Report>, OnyxEntry<Account>] | ||
*/ | ||
type OnyxDerivedValueConfig<Key extends ValueOf<typeof ONYXKEYS.DERIVED>, Deps extends NonEmptyTuple<Exclude<OnyxKey, Key>>> = { | ||
key: Key; | ||
dependencies: Deps; | ||
compute: (args: { | ||
-readonly [Index in keyof Deps]: GetOnyxTypeForKey<Deps[Index]>; | ||
}) => OnyxEntry<OnyxDerivedValuesMapping[Key]>; | ||
}; | ||
|
||
/** | ||
* Helper function to create a derived value config. This function is just here to help TypeScript infer Deps, so instead of writing this: | ||
* | ||
* const conciergeChatReportIDConfig: OnyxDerivedValueConfig<[typeof ONYXKEYS.COLLECTION.REPORT, typeof ONYXKEYS.CONCIERGE_REPORT_ID]> = { | ||
* dependencies: [ONYXKEYS.COLLECTION.REPORT, ONYXKEYS.CONCIERGE_REPORT_ID], | ||
* ... | ||
* }; | ||
* | ||
* We can just write this: | ||
* | ||
* const conciergeChatReportIDConfig = createOnyxDerivedValueConfig({ | ||
* dependencies: [ONYXKEYS.COLLECTION.REPORT, ONYXKEYS.CONCIERGE_REPORT_ID] | ||
* }) | ||
*/ | ||
function createOnyxDerivedValueConfig<Key extends ValueOf<typeof ONYXKEYS.DERIVED>, Deps extends NonEmptyTuple<Exclude<OnyxKey, Key>>>( | ||
config: OnyxDerivedValueConfig<Key, Deps>, | ||
): OnyxDerivedValueConfig<Key, Deps> { | ||
return config; | ||
} | ||
|
||
/** | ||
* Global map of derived configs. | ||
* This object holds our derived value configurations. | ||
*/ | ||
const ONYX_DERIVED_VALUES = { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This should be a separate file. We will be adding more to it in future. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Updated directory structure:
|
||
[ONYXKEYS.DERIVED.CONCIERGE_CHAT_REPORT_ID]: createOnyxDerivedValueConfig({ | ||
key: ONYXKEYS.DERIVED.CONCIERGE_CHAT_REPORT_ID, | ||
dependencies: [ONYXKEYS.COLLECTION.REPORT, ONYXKEYS.CONCIERGE_REPORT_ID], | ||
compute: ([reports, conciergeChatReportID]) => { | ||
if (!reports) { | ||
return undefined; | ||
} | ||
|
||
const conciergeReport = Object.values(reports).find((report) => { | ||
if (!report?.participants || isThread(report)) { | ||
return false; | ||
} | ||
|
||
const participantAccountIDs = new Set(Object.keys(report.participants)); | ||
if (participantAccountIDs.size !== 2) { | ||
return false; | ||
} | ||
|
||
return participantAccountIDs.has(CONST.ACCOUNT_ID.CONCIERGE.toString()) || report?.reportID === conciergeChatReportID; | ||
}); | ||
|
||
return conciergeReport?.reportID; | ||
}, | ||
}), | ||
} as const; | ||
|
||
/** | ||
* This helper exists to map an array of Onyx keys such as `['report_', 'conciergeReportID']` | ||
* to the values for those keys (correctly typed) such as `[OnyxCollection<Report>, OnyxEntry<string>]` | ||
* | ||
* Note: just using .map, you'd end up with `Array<OnyxCollection<Report>|OnyxEntry<string>>`, which is not what we want. This preserves the order of the keys provided. | ||
*/ | ||
function getOnyxValues<Keys extends readonly OnyxKey[]>(keys: Keys): Promise<{[Index in keyof Keys]: GetOnyxTypeForKey<Keys[Index]>}> { | ||
roryabraham marked this conversation as resolved.
Show resolved
Hide resolved
|
||
return Promise.all(keys.map((key) => OnyxUtils.get(key))) as Promise<{[Index in keyof Keys]: GetOnyxTypeForKey<Keys[Index]>}>; | ||
} | ||
|
||
/** | ||
* Initialize all Onyx derived values, store them in Onyx, and setup listeners to update them when dependencies change. | ||
*/ | ||
function init() { | ||
for (const [key, {compute, dependencies}] of ObjectUtils.typedEntries(ONYX_DERIVED_VALUES)) { | ||
// Create an array to hold the current values for each dependency. | ||
// We cast its type to match the tuple expected by config.compute. | ||
let dependencyValues = new Array(dependencies.length) as Parameters<typeof compute>[0]; | ||
|
||
OnyxUtils.get(key).then((storedDerivedValue) => { | ||
let derivedValue = storedDerivedValue; | ||
if (derivedValue) { | ||
Log.info(`Derived value ${derivedValue} for ${key} restored from disk`); | ||
} else { | ||
getOnyxValues(dependencies).then((values) => { | ||
dependencyValues = values; | ||
derivedValue = compute(values); | ||
Onyx.set(key, derivedValue ?? null); | ||
}); | ||
} | ||
|
||
const setDependencyValue = <Index extends number>(i: Index, value: Parameters<typeof compute>[0][Index]) => { | ||
dependencyValues[i] = value; | ||
}; | ||
|
||
const recomputeDerivedValue = () => { | ||
const newDerivedValue = compute(dependencyValues); | ||
if (newDerivedValue !== derivedValue) { | ||
Log.info(`[OnyxDerived] value for key ${key} changed, updating it in Onyx`, false, {old: derivedValue, new: newDerivedValue}); | ||
derivedValue = newDerivedValue; | ||
Onyx.set(key, derivedValue ?? null); | ||
} | ||
}; | ||
|
||
for (let i = 0; i < dependencies.length; i++) { | ||
const dependencyOnyxKey = dependencies[i]; | ||
if (OnyxUtils.isCollectionKey(dependencyOnyxKey)) { | ||
Onyx.connect({ | ||
key: dependencyOnyxKey, | ||
waitForCollectionCallback: true, | ||
callback: (value) => { | ||
Log.info(`[OnyxDerived] dependency ${dependencyOnyxKey} for derived key ${key} changed, recomputing`); | ||
setDependencyValue(i, value); | ||
recomputeDerivedValue(); | ||
}, | ||
}); | ||
} else { | ||
Onyx.connect({ | ||
key: dependencyOnyxKey, | ||
callback: (value) => { | ||
Log.info(`[OnyxDerived] dependency ${dependencyOnyxKey} for derived key ${key} changed, recomputing`); | ||
setDependencyValue(i, value); | ||
recomputeDerivedValue(); | ||
}, | ||
}); | ||
} | ||
} | ||
}); | ||
} | ||
} | ||
|
||
export default init; | ||
|
||
// Note: we can't use `as const satisfies...` for ONYX_DERIVED_VALUES without losing type specificity. | ||
// So these type assertions are here to help enforce that ONYX_DERIVED_VALUES has all the keys and the correct types, | ||
// according to the type definitions for derived keys in ONYXKEYS.ts. | ||
type MismatchedDerivedKeysError = | ||
`Error: ONYX_DERIVED_VALUES does not match ONYXKEYS.DERIVED or OnyxDerivedValuesMapping. The following keys are present in one or the other, but not both: ${SymmetricDifference< | ||
keyof typeof ONYX_DERIVED_VALUES, | ||
OnyxDerivedKey | ||
>}`; | ||
// eslint-disable-next-line @typescript-eslint/no-unused-vars | ||
type KeyAssertion = AssertTypesEqual<keyof typeof ONYX_DERIVED_VALUES, OnyxDerivedKey, MismatchedDerivedKeysError>; | ||
|
||
type ExpectedDerivedValueComputeReturnTypes = { | ||
[Key in keyof OnyxDerivedValuesMapping]: OnyxEntry<OnyxDerivedValuesMapping[Key]>; | ||
}; | ||
type ActualDerivedValueComputeReturnTypes = { | ||
[Key in keyof typeof ONYX_DERIVED_VALUES]: ReturnType<(typeof ONYX_DERIVED_VALUES)[Key]['compute']>; | ||
}; | ||
type MismatchedDerivedValues = { | ||
[Key in keyof ExpectedDerivedValueComputeReturnTypes]: ExpectedDerivedValueComputeReturnTypes[Key] extends ActualDerivedValueComputeReturnTypes[Key] ? never : Key; | ||
}[keyof ExpectedDerivedValueComputeReturnTypes]; | ||
type MismatchedDerivedValuesError = | ||
`Error: ONYX_DERIVED_VALUES does not match OnyxDerivedValuesMapping. The following configs have compute functions that do not return the correct type according to OnyxDerivedValuesMapping: ${MismatchedDerivedValues}`; | ||
// eslint-disable-next-line @typescript-eslint/no-unused-vars | ||
type ComputeReturnTypeAssertion = AssertTypesEqual<MismatchedDerivedValues, never, MismatchedDerivedValuesError>; |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -1,5 +1,6 @@ | ||
import {I18nManager} from 'react-native'; | ||
import Onyx from 'react-native-onyx'; | ||
import initOnyxDerivedValues from '@libs/actions/OnyxDerived'; | ||
import intlPolyfill from '@libs/IntlPolyfill'; | ||
import {setDeviceID} from '@userActions/Device'; | ||
import CONST from '@src/CONST'; | ||
|
@@ -45,6 +46,8 @@ export default function () { | |
skippableCollectionMemberIDs: CONST.SKIPPABLE_COLLECTION_MEMBER_IDS, | ||
}); | ||
|
||
initOnyxDerivedValues(); | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Won't it add to the startup time of the app? Earlier these callbacks were async, now we are adding these subscriptions on startup. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. That's a trade-off we discussed, and I had an alternate solution that supported lazy-loading derived values. We opted for this one for simplicity. However, in practice it doesn't increase startup time, because the derived values we are using are mostly all needed during startup anyways. And if the same thing is computed N times for N re-renders during startup, v.s: just once (or more accurately, K times, where K is the number of dependencies on the derived value), then the latter will be faster. Here's a benchmark that showed that this exactly solution (applied on more derived values than the one included in this PR) improves TTI and other metrics. (ref) There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Thanks for clarifying. It is good we already discussed this. |
||
|
||
setDeviceID(); | ||
|
||
// Force app layout to work left to right because our design does not currently support devices using this mode | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Would you mind moving this to the section specifically about Onyx? https://github.com/Expensify/App?tab=readme-ov-file#onyx