-
Notifications
You must be signed in to change notification settings - Fork 3.2k
perf: create reportAttributes derived value #58476
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
Changes from 31 commits
f5c0355
47f2872
189608f
46c2cf5
70ae349
6805b5f
15dbb15
d703077
f7083d7
c870ebe
d83d30d
c65ca4c
de67ca6
753ee26
fad7276
a50796f
92b3891
991681b
ebb56f0
a30eedc
41f9412
7a941fe
0daf3b1
5c986cd
249b7b9
88b5665
d11bb5a
5abf1b4
9156f6b
d29f863
4a9f804
fb8ea68
c1802c2
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 |
---|---|---|
@@ -0,0 +1,36 @@ | ||
import {generateReportName} from '@libs/ReportUtils'; | ||
import createOnyxDerivedValueConfig from '@userActions/OnyxDerived/createOnyxDerivedValueConfig'; | ||
import ONYXKEYS from '@src/ONYXKEYS'; | ||
import type {ReportAttributes} from '@src/types/onyx'; | ||
|
||
/** | ||
* This derived value is used to get the report attributes for the report. | ||
* Dependency on ONYXKEYS.PERSONAL_DETAILS_LIST is to ensure that the report attributes are generated after the personal details are available. | ||
*/ | ||
|
||
export default createOnyxDerivedValueConfig({ | ||
key: ONYXKEYS.DERIVED.REPORT_ATTRIBUTES, | ||
dependencies: [ONYXKEYS.COLLECTION.REPORT, ONYXKEYS.PERSONAL_DETAILS_LIST, ONYXKEYS.NVP_PREFERRED_LOCALE], | ||
compute: ([reports, personalDetails, preferredLocale], {currentValue, sourceValues}) => { | ||
if (!reports || !personalDetails || !preferredLocale) { | ||
return {}; | ||
} | ||
|
||
const reportUpdates = sourceValues?.[ONYXKEYS.COLLECTION.REPORT]; | ||
|
||
return Object.values(reportUpdates ?? reports).reduce<Record<string, ReportAttributes>>( | ||
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 have a concern, will it affect App load time? 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. App load time should not be affected, since currently the same calculations are being done in 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. Thank you @TMisiukiewicz. Let me know your thoughts on other feedbacks. |
||
(acc, report) => { | ||
if (!report) { | ||
return acc; | ||
} | ||
|
||
acc[report.reportID] = { | ||
reportName: generateReportName(report), | ||
}; | ||
|
||
return acc; | ||
}, | ||
reportUpdates && currentValue ? currentValue : {}, | ||
); | ||
}, | ||
}); |
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -10,6 +10,7 @@ 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'; | ||
import type {DerivedValueContext} from './types'; | ||
|
||
/** | ||
* Initialize all Onyx derived values, store them in Onyx, and setup listeners to update them when dependencies change. | ||
|
@@ -23,11 +24,12 @@ function init() { | |
OnyxUtils.get(key).then((storedDerivedValue) => { | ||
let derivedValue = storedDerivedValue; | ||
if (derivedValue) { | ||
Log.info(`Derived value ${derivedValue} for ${key} restored from disk`); | ||
Log.info(`Derived value for ${key} restored from disk`); | ||
} else { | ||
OnyxUtils.tupleGet(dependencies).then((values) => { | ||
// @ts-expect-error TypeScript can't confirm the shape of tupleGet's return value matches the compute function's parameters | ||
derivedValue = compute(values, {currentValue: derivedValue}); | ||
dependencyValues = values; | ||
derivedValue = compute(values, derivedValue); | ||
Onyx.set(key, derivedValue ?? null); | ||
}); | ||
} | ||
|
@@ -36,13 +38,23 @@ function init() { | |
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); | ||
const recomputeDerivedValue = (sourceKey?: string, sourceValue?: unknown) => { | ||
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. Previously, the This unlocks smarter downstream logic - This behavior only applies to collections with 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. Thank you @TMisiukiewicz |
||
const context: DerivedValueContext<typeof key, typeof dependencies> = { | ||
currentValue: derivedValue, | ||
sourceValues: undefined, | ||
}; | ||
|
||
// If we got a source key and value, add it to the sourceValues object | ||
if (sourceKey && sourceValue !== undefined) { | ||
context.sourceValues = { | ||
[sourceKey]: sourceValue, | ||
}; | ||
} | ||
// @ts-expect-error TypeScript can't confirm the shape of dependencyValues matches the compute function's parameters | ||
const newDerivedValue = compute(dependencyValues, context); | ||
Log.info(`[OnyxDerived] updating value for ${key} in Onyx`, false, {old: derivedValue ?? null, new: newDerivedValue ?? null}); | ||
derivedValue = newDerivedValue; | ||
Onyx.set(key, derivedValue ?? null); | ||
}; | ||
|
||
for (let i = 0; i < dependencies.length; i++) { | ||
|
@@ -52,10 +64,10 @@ function init() { | |
Onyx.connect({ | ||
key: dependencyOnyxKey, | ||
waitForCollectionCallback: true, | ||
callback: (value) => { | ||
Log.info(`[OnyxDerived] dependency ${dependencyOnyxKey} for derived key ${key} changed, recomputing`); | ||
callback: (value, collectionKey, sourceValue) => { | ||
Log.info(`[OnyxDerived] dependency ${collectionKey} for derived key ${key} changed, recomputing`); | ||
setDependencyValue(i, value as Parameters<typeof compute>[0][typeof i]); | ||
recomputeDerivedValue(); | ||
recomputeDerivedValue(dependencyOnyxKey, sourceValue); | ||
}, | ||
}); | ||
} else { | ||
|
@@ -64,7 +76,7 @@ function init() { | |
callback: (value) => { | ||
Log.info(`[OnyxDerived] dependency ${dependencyOnyxKey} for derived key ${key} changed, recomputing`); | ||
setDependencyValue(i, value as Parameters<typeof compute>[0][typeof i]); | ||
recomputeDerivedValue(); | ||
recomputeDerivedValue(dependencyOnyxKey); | ||
}, | ||
}); | ||
} | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -1,8 +1,15 @@ | ||
import type {OnyxValue} from 'react-native-onyx'; | ||
import type {OnyxCollection, OnyxValue} from 'react-native-onyx'; | ||
import type {NonEmptyTuple, ValueOf} from 'type-fest'; | ||
import type {OnyxDerivedValuesMapping, OnyxKey} from '@src/ONYXKEYS'; | ||
import type {OnyxCollectionKey, OnyxCollectionValuesMapping, OnyxDerivedValuesMapping, OnyxKey} from '@src/ONYXKEYS'; | ||
import type ONYXKEYS from '@src/ONYXKEYS'; | ||
|
||
type DerivedValueContext<Key extends OnyxKey, Deps extends NonEmptyTuple<Exclude<OnyxKey, Key>>> = { | ||
currentValue?: OnyxValue<Key>; | ||
sourceValues?: { | ||
[K in Deps[number] as K extends OnyxCollectionKey ? K : never]?: K extends keyof OnyxCollectionValuesMapping ? OnyxCollection<OnyxCollectionValuesMapping[K]> : never; | ||
}; | ||
}; | ||
|
||
/** | ||
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 is there any way to simplify this by creating helper types, with 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 your comment, mind checking if it is easier to read 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. thanks @TMisiukiewicz - it still borders on arcane, but is way easier to understand what's going on. Thank you! |
||
* A derived value configuration describes: | ||
* - a tuple of Onyx keys to subscribe to (dependencies), | ||
|
@@ -17,9 +24,8 @@ type OnyxDerivedValueConfig<Key extends ValueOf<typeof ONYXKEYS.DERIVED>, Deps e | |
args: { | ||
[Index in keyof Deps]: OnyxValue<Deps[Index]>; | ||
}, | ||
currentValue: OnyxValue<Key>, | ||
context: DerivedValueContext<Key, Deps>, | ||
) => OnyxDerivedValuesMapping[Key]; | ||
}; | ||
|
||
// eslint-disable-next-line import/prefer-default-export | ||
export type {OnyxDerivedValueConfig}; | ||
export type {OnyxDerivedValueConfig, DerivedValueContext}; |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,11 @@ | ||
/** | ||
* The attributes of a report. | ||
*/ | ||
type ReportAttributes = { | ||
/** | ||
* The name of the report. | ||
*/ | ||
reportName: string; | ||
}; | ||
|
||
export default ReportAttributes; |
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.
Could you elaborate more on why we need to wait for
ONYXKEYS.PERSONAL_DETAILS_LIST
to be available?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.
Some report names are generated based on information stored in personal details - this ensures we calculate them only if this information is available so we do not miss anything
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.
I see, is this one example?
App/src/libs/ReportUtils.ts
Lines 4762 to 4764 in 32492c3
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.
an example is
getGroupChatName
function that is usinggetDisplayNameForParticipant
which is using personal details. Found it out when tests forGroupChatName
were failing