-
Notifications
You must be signed in to change notification settings - Fork 3k
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
base: main
Are you sure you want to change the base?
Onyx derived values #56891
Conversation
@@ -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 comment
The reason will be displayed to describe this comment to others. Learn more.
These changes were only necessary because I upgraded type-fest
and a bug fix in that repo exposed existing issues with the types in this code.
src/libs/actions/OnyxDerived.ts
Outdated
* 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 comment
The 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 comment
The reason will be displayed to describe this comment to others. Learn more.
Updated directory structure:
src/libs/actions/OnyxDerived
--| configs // you can add more dervied value configs in here, one config per file
----| conciergeChatReportID.ts
--| createOnyxDerivedValueConfig.ts // this utility is in it's own file to prevent a require cycle. it's imported by all the configs, and those are imported by OnyxDerived/index.ts
--| index.ts // the main utility which handles computing all the values, setting up subscriptions, etc...
--| ONYX_DERIVED_VALUES.ts // the global map of derived value configs. Mostly just imports configs from ./configs and puts them in a map, but also includes some advanced type assertions
--| types.ts // contains reusable types for the module
if (reportID) { | ||
if (isConciergeChatReport(report)) { | ||
conciergeChatReportID = reportID; | ||
} |
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.
Shouldn't we set the Onyx ONYXKEYS.CONCIERGE_REPORT_ID
key here ?
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.
We weren't before, why would we start now?
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.
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 conciergeChatReportID
in this file so updating local variable works but in case we use this Onyx Key somewhere else, it would be good to update the Onyx data instead of local.
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 comment
The 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 conciergeChatReportID
.
But now, the local variable conciergeChatReportID
is subscribed to our derived value, and that value is updated as needed.
in case we use this Onyx Key somewhere else, it would be good to update the Onyx data instead of local
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 comment
The reason will be displayed to describe this comment to others. Learn more.
Got it.
HybridApp builds seem to be failing everywhere - doubt that's caused by this PR |
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 comment
The reason will be displayed to describe this comment to others. Learn more.
API could be improved a little, so that array isn't needed:
compute: ([reports, conciergeChatReportID]) => { | |
compute: (reports, conciergeChatReportID) => { |
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 comment
The 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 comment
The 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 comment
The 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. Promise.all
.
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.
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 comment
The reason will be displayed to describe this comment to others. Learn more.
Since we added the current value as a second arg to compute
, I think I'm going to leave the first as a tuple. That way it's not like you have n
args and the last one happens to be the current value
// 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. |
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.
Are you sure we can't use satisfies? 🤔 I feel like it works well for me:
Diff
diff --git a/src/ONYXKEYS.ts b/src/ONYXKEYS.ts
index d0555ed5412..57d9d99c810 100755
--- a/src/ONYXKEYS.ts
+++ b/src/ONYXKEYS.ts
@@ -761,6 +761,7 @@ const ONYXKEYS = {
},
DERIVED: {
CONCIERGE_CHAT_REPORT_ID: 'conciergeChatReportID',
+ CONCIERGE_CHAT_REPORT_ID2: 'conciergeChatReportID2',
},
} as const;
@@ -1085,6 +1086,7 @@ type OnyxValuesMapping = {
type OnyxDerivedValuesMapping = {
[ONYXKEYS.DERIVED.CONCIERGE_CHAT_REPORT_ID]: string;
+ [ONYXKEYS.DERIVED.CONCIERGE_CHAT_REPORT_ID2]: string;
};
type OnyxValues = OnyxValuesMapping & OnyxCollectionValuesMapping & OnyxFormValuesMapping & OnyxFormDraftValuesMapping & OnyxDerivedValuesMapping;
diff --git a/src/libs/actions/OnyxDerived/ONYX_DERIVED_VALUES.ts b/src/libs/actions/OnyxDerived/ONYX_DERIVED_VALUES.ts
index b9625e82871..e8efddbc4ab 100644
--- a/src/libs/actions/OnyxDerived/ONYX_DERIVED_VALUES.ts
+++ b/src/libs/actions/OnyxDerived/ONYX_DERIVED_VALUES.ts
@@ -1,9 +1,7 @@
-import type {OnyxEntry} from 'react-native-onyx';
-import type {OnyxDerivedKey, OnyxDerivedValuesMapping} from '@src/ONYXKEYS';
+import type {ValueOf} from 'type-fest';
import ONYXKEYS from '@src/ONYXKEYS';
-import type AssertTypesEqual from '@src/types/utils/AssertTypesEqual';
-import type SymmetricDifference from '@src/types/utils/SymmetricDifference';
import conciergeChatReportIDConfig from './configs/conciergeChatReportID';
+import type {OnyxDerivedValueConfig} from './types';
/**
* Global map of derived configs.
@@ -11,31 +9,8 @@ import conciergeChatReportIDConfig from './configs/conciergeChatReportID';
*/
const ONYX_DERIVED_VALUES = {
[ONYXKEYS.DERIVED.CONCIERGE_CHAT_REPORT_ID]: conciergeChatReportIDConfig,
-} as const;
+ [ONYXKEYS.DERIVED.CONCIERGE_CHAT_REPORT_ID2]: conciergeChatReportIDConfig,
+ // eslint-disable-next-line @typescript-eslint/no-explicit-any
+} as const satisfies Record<ValueOf<typeof ONYXKEYS.DERIVED>, OnyxDerivedValueConfig<any, any>>;
export default ONYX_DERIVED_VALUES;
-
-// 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>;
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.
Record<ValueOf, OnyxDerivedValueConfig<any, any>>
Right, but you'd actually want it to, rather than being an OnyxDerivedValueConfig<any, any>
be and OnyxDerivedValueConfig
for that specific key.
Plus, I like how this provides clearer error messages.
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.
Okaaay, I believe we can achieve it with a mapped type! Recorder a video to showcase the DX:
Screen.Recording.2025-02-24.at.11.15.31.mov
Diff as always
diff --git a/src/libs/actions/OnyxDerived/ONYX_DERIVED_VALUES.ts b/src/libs/actions/OnyxDerived/ONYX_DERIVED_VALUES.ts
index b9625e82871..bac7c7e7f8e 100644
--- a/src/libs/actions/OnyxDerived/ONYX_DERIVED_VALUES.ts
+++ b/src/libs/actions/OnyxDerived/ONYX_DERIVED_VALUES.ts
@@ -1,9 +1,8 @@
-import type {OnyxEntry} from 'react-native-onyx';
-import type {OnyxDerivedKey, OnyxDerivedValuesMapping} from '@src/ONYXKEYS';
+import type {OnyxValue} from 'react-native-onyx';
+import type {ValueOf} from 'type-fest';
import ONYXKEYS from '@src/ONYXKEYS';
-import type AssertTypesEqual from '@src/types/utils/AssertTypesEqual';
-import type SymmetricDifference from '@src/types/utils/SymmetricDifference';
import conciergeChatReportIDConfig from './configs/conciergeChatReportID';
+import type {OnyxDerivedValueConfig} from './types';
/**
* Global map of derived configs.
@@ -11,31 +10,9 @@ import conciergeChatReportIDConfig from './configs/conciergeChatReportID';
*/
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, OnyxValue<Key>>;
+};
export default ONYX_DERIVED_VALUES;
-
-// 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>;
diff --git a/src/libs/actions/OnyxDerived/createOnyxDerivedValueConfig.ts b/src/libs/actions/OnyxDerived/createOnyxDerivedValueConfig.ts
index 6c8c090ff7d..bbec3a075cf 100644
--- a/src/libs/actions/OnyxDerived/createOnyxDerivedValueConfig.ts
+++ b/src/libs/actions/OnyxDerived/createOnyxDerivedValueConfig.ts
@@ -1,3 +1,4 @@
+import type {OnyxValue} from 'react-native-onyx';
import type {NonEmptyTuple, ValueOf} from 'type-fest';
import type {OnyxKey} from '@src/ONYXKEYS';
import type ONYXKEYS from '@src/ONYXKEYS';
@@ -17,8 +18,8 @@ import type {OnyxDerivedValueConfig} from './types';
* 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> {
+export default function createOnyxDerivedValueConfig<Key extends ValueOf<typeof ONYXKEYS.DERIVED>, Deps extends NonEmptyTuple<Exclude<OnyxKey, Key>>, DerivedValue extends OnyxValue<Key>>(
+ config: OnyxDerivedValueConfig<Key, Deps, DerivedValue>,
+): OnyxDerivedValueConfig<Key, Deps, DerivedValue> {
return config;
}
diff --git a/src/libs/actions/OnyxDerived/types.ts b/src/libs/actions/OnyxDerived/types.ts
index 419a5357421..a43bba8cb4f 100644
--- a/src/libs/actions/OnyxDerived/types.ts
+++ b/src/libs/actions/OnyxDerived/types.ts
@@ -10,15 +10,15 @@ import type ONYXKEYS from '@src/ONYXKEYS';
* 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>>> = {
+type OnyxDerivedValueConfig<Key extends ValueOf<typeof ONYXKEYS.DERIVED>, Deps extends NonEmptyTuple<Exclude<OnyxKey, Key>>, DerivedValue extends OnyxValue<Key>> = {
key: Key;
dependencies: Deps;
compute: (
args: {
[Index in keyof Deps]: OnyxValue<Deps[Index]>;
},
- currentValue: OnyxValue<Key>,
- ) => OnyxValue<Key>;
+ currentValue: DerivedValue,
+ ) => DerivedValue;
};
// eslint-disable-next-line import/prefer-default-export
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.
@roryabraham Let me know if this is what you wanted to achieve 😄 With this approach ONYXKEYS' mapping is the source of truth while keeping type safety in ONYX_DERIVED_VALUES per key, please test it yourself!
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.
great work! I just pushed a variant of this that I believe is fully correct. The main difference between my version and yours is that mine does not coerce the return type of compute
to be an OnyxEntry
.
The error messages suck, in particular when you have a collection in the dependencies. But that seems like a common problem in TS in general. I tried adding some assertions to provide better error messages, but didn't get it working and don't want to sink too much time into it.
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.
Added comment in one of the threads 🚀
# Conflicts: # package-lock.json # package.json
Explanation of Change
compute
function, which receives the onyx values for all the dependent keys and must return the type defined for the derived key in ONYXKEYSAny consumers of derived values treat them just like any other Onyx key
Fixed Issues
$ https://expensify.slack.com/archives/C05LX9D6E07/p1739458915155239
Tests
I wrote and updated some automated tests that validated that this code is working, but for the sake of manual testing, some simple tests will do.
24/7 Support
in the chat subheader:this indicates that the derived value for
isConciergeChatReportID
is being correctly read.Offline tests
None.
QA Steps
24/7 Support
in the chat subheader:PR Author Checklist
### Fixed Issues
section aboveTests
sectionOffline steps
sectionQA steps
sectiontoggleReport
and notonIconClick
)src/languages/*
files and using the translation methodSTYLE.md
) were followedAvatar
, I verified the components usingAvatar
are working as expected)StyleUtils.getBackgroundAndBorderStyle(theme.componentBG)
)Avatar
is modified, I verified thatAvatar
is working as expected in all cases)Design
label and/or tagged@Expensify/design
so the design team can review the changes.ScrollView
component to make it scrollable when more elements are added to the page.main
branch was merged into this PR after a review, I tested again and verified the outcome was still expected according to theTest
steps.Screenshots/Videos
Android: Native
Android: mWeb Chrome
iOS: Native
iOS: mWeb Safari
MacOS: Chrome / Safari
MacOS: Desktop