-
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 all 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 |
---|---|---|
|
@@ -849,3 +849,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 | ||
|
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 appears like this file is only imported in one place. I suggest moving this config only to the file that needs it. |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,17 @@ | ||
import type {ValueOf} from 'type-fest'; | ||
import ONYXKEYS from '@src/ONYXKEYS'; | ||
import conciergeChatReportIDConfig from './configs/conciergeChatReportID'; | ||
import type {OnyxDerivedValueConfig} from './types'; | ||
|
||
/** | ||
* Global map of derived configs. | ||
* This object holds our derived value configurations. | ||
*/ | ||
const ONYX_DERIVED_VALUES = { | ||
[ONYXKEYS.DERIVED.CONCIERGE_CHAT_REPORT_ID]: conciergeChatReportIDConfig, | ||
} as const satisfies { | ||
// eslint-disable-next-line @typescript-eslint/no-explicit-any | ||
[Key in ValueOf<typeof ONYXKEYS.DERIVED>]: OnyxDerivedValueConfig<Key, any>; | ||
}; | ||
|
||
export default ONYX_DERIVED_VALUES; |
Original file line number | Diff line number | Diff line change | ||||
---|---|---|---|---|---|---|
@@ -0,0 +1,29 @@ | ||||||
import {isThread} from '@libs/ReportUtils'; | ||||||
import createOnyxDerivedValueConfig from '@userActions/OnyxDerived/createOnyxDerivedValueConfig'; | ||||||
import CONST from '@src/CONST'; | ||||||
import ONYXKEYS from '@src/ONYXKEYS'; | ||||||
|
||||||
export default createOnyxDerivedValueConfig({ | ||||||
key: ONYXKEYS.DERIVED.CONCIERGE_CHAT_REPORT_ID, | ||||||
dependencies: [ONYXKEYS.COLLECTION.REPORT, ONYXKEYS.CONCIERGE_REPORT_ID], | ||||||
compute: ([reports, conciergeChatReportID]) => { | ||||||
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. API could be improved a little, so that array isn't needed:
Suggested change
Full diff in case you're interested:diff --git a/src/libs/actions/OnyxDerived/configs/conciergeChatReportID.ts b/src/libs/actions/OnyxDerived/configs/conciergeChatReportID.ts
index ce246616b18..7dbe50ed25d 100644
--- a/src/libs/actions/OnyxDerived/configs/conciergeChatReportID.ts
+++ b/src/libs/actions/OnyxDerived/configs/conciergeChatReportID.ts
@@ -6,7 +6,7 @@ import ONYXKEYS from '@src/ONYXKEYS';
export default createOnyxDerivedValueConfig({
key: ONYXKEYS.DERIVED.CONCIERGE_CHAT_REPORT_ID,
dependencies: [ONYXKEYS.COLLECTION.REPORT, ONYXKEYS.CONCIERGE_REPORT_ID],
- compute: ([reports, conciergeChatReportID]) => {
+ compute: (reports, conciergeChatReportID) => {
if (!reports) {
return undefined;
}
diff --git a/src/libs/actions/OnyxDerived/index.ts b/src/libs/actions/OnyxDerived/index.ts
index a30a7a0ca0f..0b858589c3e 100644
--- a/src/libs/actions/OnyxDerived/index.ts
+++ b/src/libs/actions/OnyxDerived/index.ts
@@ -18,7 +18,7 @@ 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];
+ let dependencyValues = new Array(dependencies.length) as Parameters<typeof compute>;
OnyxUtils.get(key).then((storedDerivedValue) => {
let derivedValue = storedDerivedValue;
@@ -27,17 +27,17 @@ function init() {
} else {
OnyxUtils.tupleGet(dependencies).then((values) => {
dependencyValues = values;
- derivedValue = compute(values);
+ derivedValue = compute(...values);
Onyx.set(key, derivedValue ?? null);
});
}
- const setDependencyValue = <Index extends number>(i: Index, value: Parameters<typeof compute>[0][Index]) => {
+ const setDependencyValue = <Index extends number>(i: Index, value: Parameters<typeof compute>[Index]) => {
dependencyValues[i] = value;
};
const recomputeDerivedValue = () => {
- const newDerivedValue = compute(dependencyValues);
+ 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;
diff --git a/src/libs/actions/OnyxDerived/types.ts b/src/libs/actions/OnyxDerived/types.ts
index 3f05b6ea8c3..2aac96849a7 100644
--- a/src/libs/actions/OnyxDerived/types.ts
+++ b/src/libs/actions/OnyxDerived/types.ts
@@ -13,9 +13,11 @@ import type ONYXKEYS from '@src/ONYXKEYS';
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]: OnyxValue<Deps[Index]>;
- }) => OnyxValue<Key>;
+ compute: (
+ ...args: {
+ -readonly [Index in keyof Deps]: OnyxValue<Deps[Index]>;
+ }
+ ) => OnyxValue<Key>;
};
// eslint-disable-next-line import/prefer-default-export 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. At the same time I'm thinking if it'd be clear enough without a tuple? I think my only concern is that swapping dependencies in both these cases can break the whole pipeline. 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. But the same would happen with or without an array, right? In the end you have TS type checking to make sure you use it correctly 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. I like array-like syntax for the reason that its kind of norm for many modules. e.g. 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. Just suggesting that it is possible! I'm good with both approaches 👍 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. Since we added the current value as a second arg to
roryabraham marked this conversation as resolved.
Show resolved
Hide resolved
|
||||||
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; | ||||||
}, | ||||||
}); |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,24 @@ | ||
import type {NonEmptyTuple, ValueOf} from 'type-fest'; | ||
import type {OnyxKey} from '@src/ONYXKEYS'; | ||
import type ONYXKEYS from '@src/ONYXKEYS'; | ||
import type {OnyxDerivedValueConfig} from './types'; | ||
|
||
/** | ||
* 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] | ||
* }) | ||
*/ | ||
export default function createOnyxDerivedValueConfig<Key extends ValueOf<typeof ONYXKEYS.DERIVED>, Deps extends NonEmptyTuple<Exclude<OnyxKey, Key>>>( | ||
config: OnyxDerivedValueConfig<Key, Deps>, | ||
): OnyxDerivedValueConfig<Key, Deps> { | ||
return config; | ||
} |
Original file line number | Diff line number | Diff line change | ||||
---|---|---|---|---|---|---|
@@ -0,0 +1,76 @@ | ||||||
/** | ||||||
* 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 Onyx from 'react-native-onyx'; | ||||||
import OnyxUtils from 'react-native-onyx/dist/OnyxUtils'; | ||||||
import Log from '@libs/Log'; | ||||||
import ObjectUtils from '@src/types/utils/ObjectUtils'; | ||||||
import ONYX_DERIVED_VALUES from './ONYX_DERIVED_VALUES'; | ||||||
|
||||||
/** | ||||||
* 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) => { | ||||||
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. I am concerned this I've already seen we've struggled to keep the usage of Is there a reason why |
||||||
let derivedValue = storedDerivedValue; | ||||||
if (derivedValue) { | ||||||
Log.info(`Derived value ${derivedValue} for ${key} restored from disk`); | ||||||
} else { | ||||||
OnyxUtils.tupleGet(dependencies).then((values) => { | ||||||
dependencyValues = values; | ||||||
derivedValue = compute(values, derivedValue); | ||||||
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, derivedValue); | ||||||
if (newDerivedValue !== derivedValue) { | ||||||
Log.info(`[OnyxDerived] value for key ${key} changed, updating it in Onyx`, false, {old: derivedValue ?? null, new: newDerivedValue ?? null}); | ||||||
derivedValue = newDerivedValue; | ||||||
Onyx.set(key, derivedValue ?? null); | ||||||
} | ||||||
}; | ||||||
|
||||||
for (let i = 0; i < dependencies.length; i++) { | ||||||
// eslint-disable-next-line rulesdir/prefer-at | ||||||
const dependencyOnyxKey = dependencies[i]; | ||||||
if (OnyxUtils.isCollectionKey(dependencyOnyxKey)) { | ||||||
Onyx.connect({ | ||||||
key: dependencyOnyxKey, | ||||||
waitForCollectionCallback: true, | ||||||
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. NAB but I think we can simplify this logic by just setting the flag below and getting rid of the the if/else
Suggested change
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 seems like TS fails with this suggestion. So we'll keep the if/else here. 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 actually causes a type error. I wasn't able to figure out how to fix it. cc @fabioh8010 also spent a bit of time looking into it 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. Yeah no luck so far, I will revisit this again to see if I manage to make this work as expected |
||||||
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; |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,25 @@ | ||
import type {OnyxValue} from 'react-native-onyx'; | ||
import type {NonEmptyTuple, ValueOf} from 'type-fest'; | ||
import type {OnyxDerivedValuesMapping, OnyxKey} from '@src/ONYXKEYS'; | ||
import type ONYXKEYS from '@src/ONYXKEYS'; | ||
|
||
/** | ||
* 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: { | ||
[Index in keyof Deps]: OnyxValue<Deps[Index]>; | ||
}, | ||
currentValue: OnyxValue<Key>, | ||
) => OnyxDerivedValuesMapping[Key]; | ||
}; | ||
|
||
// eslint-disable-next-line import/prefer-default-export | ||
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. Why not use a default export here? The only reason I wouldn't use one is if you're expecting to immediately be adding more exported methods. |
||
export type {OnyxDerivedValueConfig}; |
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -234,7 +234,7 @@ Onyx.connect({ | |
}); | ||
|
||
Onyx.connect({ | ||
key: ONYXKEYS.CONCIERGE_REPORT_ID, | ||
key: ONYXKEYS.DERIVED.CONCIERGE_CHAT_REPORT_ID, | ||
callback: (value) => (conciergeChatReportID = value), | ||
}); | ||
|
||
|
@@ -1568,14 +1568,6 @@ function handleReportChanged(report: OnyxEntry<Report>) { | |
} | ||
|
||
saveReportDraftComment(preexistingReportID, draftReportComment, callback); | ||
|
||
return; | ||
} | ||
|
||
if (reportID) { | ||
if (isConciergeChatReport(report)) { | ||
conciergeChatReportID = reportID; | ||
} | ||
Comment on lines
-1575
to
-1578
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. Shouldn't we set the Onyx 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. We weren't before, why would we start now? 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. We were technically updating reference to the conciergeChatReportID with the correct id here. But it might be the case that we are only using this Then, I have a followup question on why are we removing this code? 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. We are removing this code because it was previously performing the computation done by our derived value to check if a new report is the concierge chat, and if so update the local value for But now, the local variable
The piece of data that was being modified locally here (conciergeChatReportID) is now stored centrally in Onyx (computed via its derived value config). Does that satisfy your concerns @parasharrajat? Or am I missing something? 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. Got it. |
||
} | ||
} | ||
|
||
|
@@ -4655,9 +4647,6 @@ function exportReportToCSV({reportID, transactionIDList}: ExportReportCSVParams, | |
|
||
fileDownload(ApiUtils.getCommandURL({command: WRITE_COMMANDS.EXPORT_REPORT_TO_CSV}), 'Expensify.csv', '', false, formData, CONST.NETWORK.METHOD.POST, onDownloadFailed); | ||
} | ||
function getConciergeReportID() { | ||
return conciergeChatReportID; | ||
} | ||
|
||
function setDeleteTransactionNavigateBackUrl(url: string) { | ||
Onyx.set(ONYXKEYS.NVP_DELETE_TRANSACTION_NAVIGATE_BACK_URL, url); | ||
|
@@ -4695,7 +4684,6 @@ export { | |
exportReportToCSV, | ||
exportToIntegration, | ||
flagComment, | ||
getConciergeReportID, | ||
getCurrentUserAccountID, | ||
getDraftPrivateNote, | ||
getMostRecentReportID, | ||
|
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