[POC] Feat/onyx get async readonly

src/components/GPSTripStateChecker/useUpdateGpsTripOnReconnect.ts

@@ -7,9 +7,11 @@ import {addressFromGpsPoint, getGpsPoints} from '@libs/GPSDraftDetailsUtils';
 import ONYXKEYS from '@src/ONYXKEYS';
 import type {GPSPoint, TrimmedGPSPoint} from '@src/types/onyx/GpsDraftDetails';
 
+import type {ReadonlyDeep} from 'type-fest';
+
 import {useEffect, useRef} from 'react';
 
-function useUpdateGpsTripOnReconnect({gpsPoints}: {gpsPoints: GPSPoint[][]}) {
+function useUpdateGpsTripOnReconnect({gpsPoints}: {gpsPoints: ReadonlyDeep<GPSPoint[][]>}) {
     const [gpsDraftDetails] = useOnyx(ONYXKEYS.GPS_DRAFT_DETAILS);
     // Mirror the latest gpsDraftDetails into a ref so the async onReconnect handler can read the newest value
     // after awaiting reverse geocoding, instead of the stale value captured when the callback started.

src/hooks/useSwitchToDelegator.ts

@@ -4,16 +4,14 @@ import {ModalActions} from '@components/Modal/Global/ModalContext';
 import {connect, disconnect} from '@libs/actions/Delegate';
 import {close as modalClose} from '@libs/actions/Modal';
 import {getGpsPoints, stopGpsTrip} from '@libs/GPSDraftDetailsUtils';
+import Onyx from '@libs/OnyxReadonly';
 
-import CONST from '@src/CONST';
 import ONYXKEYS from '@src/ONYXKEYS';
 import {isTrackingSelector} from '@src/selectors/GPSDraftDetails';
-import type {Account} from '@src/types/onyx';
 
 import useConfirmModal from './useConfirmModal';
 import useLocalize from './useLocalize';
 import useNetwork from './useNetwork';
-import useOnyx from './useOnyx';
 
 /**
  * Encapsulates the safety checks needed before switching to a delegator account:
@@ -22,6 +20,9 @@ import useOnyx from './useOnyx';
  *    to the original user, shows the "not so fast" modal.
  * 3. GPS tracking check – if a GPS trip is in progress, asks the user to confirm
  *    stopping the trip before switching.
+ *
+ * Every value this needs is read when the switch is pressed rather than subscribed to, so the hook holds no
+ * Onyx subscription and the screens mounting it do not re-render on account, session or GPS changes.
  */
 function useSwitchToDelegator() {
     const {translate} = useLocalize();
@@ -30,15 +31,6 @@ function useSwitchToDelegator() {
     const {isActingAsDelegate} = useDelegateNoAccessState();
     const {showDelegateNoAccessModal} = useDelegateNoAccessActions();
 
-    const [delegatedAccess] = useOnyx(ONYXKEYS.ACCOUNT, {selector: (account: Account | undefined) => account?.delegatedAccess});
-    const [credentials] = useOnyx(ONYXKEYS.CREDENTIALS);
-    const [stashedCredentials = CONST.EMPTY_OBJECT] = useOnyx(ONYXKEYS.STASHED_CREDENTIALS);
-    const [session] = useOnyx(ONYXKEYS.SESSION);
-    const [stashedSession] = useOnyx(ONYXKEYS.STASHED_SESSION);
-    const [activePolicyID] = useOnyx(ONYXKEYS.NVP_ACTIVE_POLICY_ID);
-    const [gpsDraftDetails] = useOnyx(ONYXKEYS.GPS_DRAFT_DETAILS);
-    const [isTrackingGPS = false] = useOnyx(ONYXKEYS.GPS_DRAFT_DETAILS, {selector: isTrackingSelector});
-
     const showOfflineModal = () => {
         showConfirmModal({
             title: translate('common.youAppearToBeOffline'),
@@ -60,29 +52,48 @@ function useSwitchToDelegator() {
             return;
         }
 
+        // Points keep arriving while the modal is open, so the trip is read again here rather than reusing
+        // the value the tracking check was made on.
+        const gpsDraftDetails = await Onyx.get(ONYXKEYS.GPS_DRAFT_DETAILS);
+
         await stopGpsTrip(false, getGpsPoints(gpsDraftDetails), true);
         switchAccount();
     };
 
-    const switchToDelegator = (email: string) => {
+    const switchToDelegator = async (email: string) => {
         if (isOffline) {
             modalClose(() => showOfflineModal());
             return;
         }
+
+        // Read in one go so every value comes from the same cache state, and before anything writes in this tick.
+        const [account, credentials, stashedCredentials, session, stashedSession, activePolicyID, gpsDraftDetails] = await Promise.all([
+            Onyx.get(ONYXKEYS.ACCOUNT),
+            Onyx.get(ONYXKEYS.CREDENTIALS),
+            Onyx.get(ONYXKEYS.STASHED_CREDENTIALS),
+            Onyx.get(ONYXKEYS.SESSION),
+            Onyx.get(ONYXKEYS.STASHED_SESSION),
+            Onyx.get(ONYXKEYS.NVP_ACTIVE_POLICY_ID),
+            Onyx.get(ONYXKEYS.GPS_DRAFT_DETAILS),
+        ]);
+
         const isReturningToOriginalUser = isActingAsDelegate && email === stashedSession?.email;
         // Chained delegation isn't supported by the backend — if we're already acting as a delegate,
         // the only legal switch is back to the original user. Anything else triggers the "Not so fast" modal.
         if (isActingAsDelegate && !isReturningToOriginalUser) {
             modalClose(() => showDelegateNoAccessModal());
             return;
         }
+
         const switchAction = isReturningToOriginalUser
             ? () => disconnect({stashedCredentials, stashedSession})
-            : () => connect({email, delegatedAccess, credentials, session, activePolicyID});
-        if (isTrackingGPS) {
+            : () => connect({email, delegatedAccess: account?.delegatedAccess, credentials, session, activePolicyID});
+
+        if (isTrackingSelector(gpsDraftDetails)) {
             modalClose(() => showGpsInProgressModal(switchAction));
             return;
         }
+
         switchAction();
     };
 

src/libs/GPSDraftDetailsUtils.ts

@@ -8,7 +8,7 @@ import type {GPSPoint, TrimmedGPSPoint} from '@src/types/onyx/GpsDraftDetails';
 import type {Routes, Waypoint} from '@src/types/onyx/Transaction';
 import geodesicDistance from '@src/utils/geodesicDistance';
 
-import type {SetRequired} from 'type-fest';
+import type {ReadonlyDeep, SetRequired} from 'type-fest';
 
 import {hasStartedLocationUpdatesAsync, reverseGeocodeAsync, stopLocationUpdatesAsync} from 'expo-location';
 
@@ -26,15 +26,15 @@ function getGPSWaypoint(gpsPoint: GPSPoint, waypointIndex: number): GPSWaypointC
     };
 }
 
-function getEffectiveDistance(gpsDraftDetails: GpsDraftDetails | undefined): number {
+function getEffectiveDistance(gpsDraftDetails: ReadonlyDeep<GpsDraftDetails> | undefined): number {
     return gpsDraftDetails?.modifiedDistance ?? gpsDraftDetails?.distanceInMeters ?? 0;
 }
 
-function getEffectiveEndPoint(gpsDraftDetails: GpsDraftDetails | undefined): GPSPoint | undefined {
+function getEffectiveEndPoint(gpsDraftDetails: ReadonlyDeep<GpsDraftDetails> | undefined): GPSPoint | undefined {
     return gpsDraftDetails?.trimmedEndPoint ?? gpsDraftDetails?.gpsPoints?.at(-1)?.at(-1);
 }
 
-function getGPSWaypoints(gpsDraftDetails: GpsDraftDetails | undefined, trimmedEndPoint?: TrimmedGPSPoint): GPSWaypointCollection {
+function getGPSWaypoints(gpsDraftDetails: ReadonlyDeep<GpsDraftDetails> | undefined, trimmedEndPoint?: TrimmedGPSPoint): GPSWaypointCollection {
     const gpsTrip = getTrimmedGpsTrip(gpsDraftDetails, trimmedEndPoint);
 
     const waypointCollection: GPSWaypointCollection = {};
@@ -65,7 +65,7 @@ function getGPSWaypoints(gpsDraftDetails: GpsDraftDetails | undefined, trimmedEn
     return waypointCollection;
 }
 
-function getGPSRoutes(gpsDraftDetails: GpsDraftDetails | undefined): Routes {
+function getGPSRoutes(gpsDraftDetails: ReadonlyDeep<GpsDraftDetails> | undefined): Routes {
     const distanceInMeters = roundToTwoDecimalPlaces(gpsDraftDetails?.modifiedDistance ?? gpsDraftDetails?.distanceInMeters ?? 0);
     const gpsCoordinates = getTrimmedGpsTrip(gpsDraftDetails);
     const coordinates: Array<Array<[number, number]>> = gpsCoordinates.map((points) => points.map(({lat, long}) => [long, lat]));
@@ -87,7 +87,7 @@ function getGPSRoutes(gpsDraftDetails: GpsDraftDetails | undefined): Routes {
  * If the trimmed end point is set, it is added to the recorded GPS coordinates according to the data
  * saved in trimmedEndPoint - this is a backend requirement to make the receipt generation easier.
  */
-function getStringifiedGPSCoordinates(gpsDraftDetails: GpsDraftDetails | undefined): string | undefined {
+function getStringifiedGPSCoordinates(gpsDraftDetails: ReadonlyDeep<GpsDraftDetails> | undefined): string | undefined {
     const trimmedEndPoint = gpsDraftDetails?.trimmedEndPoint;
     const gpsPoints = gpsDraftDetails?.gpsPoints;
 
@@ -136,15 +136,15 @@ function coordinatesToString(gpsPoint: {lat: number; long: number}): string {
     return `${gpsPoint.lat},${gpsPoint.long}`;
 }
 
-function isLastSegmentEmptyOrHasOnlyOnePoint(lastSegment: GPSPoint[]): boolean {
+function isLastSegmentEmptyOrHasOnlyOnePoint(lastSegment: ReadonlyDeep<GPSPoint[]>): boolean {
     if (lastSegment.length <= 1) {
         return true;
     }
 
     return false;
 }
 
-async function stopGpsTrip(isOffline: boolean, gpsPoints: GPSPoint[][], skipLastPointAddressFetching = false) {
+async function stopGpsTrip(isOffline: boolean, gpsPoints: ReadonlyDeep<GPSPoint[][]>, skipLastPointAddressFetching = false) {
     const isBackgroundTaskRunning = await hasStartedLocationUpdatesAsync(BACKGROUND_LOCATION_TRACKING_TASK_NAME);
 
     if (isBackgroundTaskRunning) {
@@ -196,27 +196,27 @@ async function stopGpsTrip(isOffline: boolean, gpsPoints: GPSPoint[][], skipLast
     setEndWaypointAddress({value: formattedCoordinates, type: 'coordinates'}, gpsPoints);
 }
 
-function getTotalGpsTripPoints(gpsDraftDetails: GpsDraftDetails | undefined): number {
+function getTotalGpsTripPoints(gpsDraftDetails: ReadonlyDeep<GpsDraftDetails> | undefined): number {
     return gpsDraftDetails?.gpsPoints?.flat().length ?? 0;
 }
 
-function getTotalGpsTripPointsInLastSegment(gpsPoints: GPSPoint[][]): number {
+function getTotalGpsTripPointsInLastSegment(gpsPoints: ReadonlyDeep<GPSPoint[][]>): number {
     return gpsPoints.at(-1)?.length ?? 0;
 }
 
-function isTripStopped(gpsDraftDetails: GpsDraftDetails | undefined): boolean {
+function isTripStopped(gpsDraftDetails: ReadonlyDeep<GpsDraftDetails> | undefined): boolean {
     return !gpsDraftDetails?.isTracking && getTotalGpsTripPoints(gpsDraftDetails) > 0;
 }
 
-function getGpsPoints(gpsDraftDetails: GpsDraftDetails | undefined): GPSPoint[][] {
+function getGpsPoints(gpsDraftDetails: ReadonlyDeep<GpsDraftDetails> | undefined): ReadonlyDeep<GPSPoint[][]> {
     return gpsDraftDetails?.gpsPoints ?? [[]];
 }
 
-function getFirstGpsPoint(gpsDraftDetails: GpsDraftDetails | undefined): GPSPoint | undefined {
+function getFirstGpsPoint(gpsDraftDetails: ReadonlyDeep<GpsDraftDetails> | undefined): GPSPoint | undefined {
     return gpsDraftDetails?.gpsPoints?.at(0)?.at(0);
 }
 
-function calculateTrimmedEndPoint(gpsPoints: GPSPoint[][], targetDistanceMeters: number): TrimmedGPSPoint | null {
+function calculateTrimmedEndPoint(gpsPoints: ReadonlyDeep<GPSPoint[][]>, targetDistanceMeters: number): TrimmedGPSPoint | null {
     let distanceTraveled = 0;
 
     for (let segmentIndex = 0; segmentIndex < gpsPoints.length; segmentIndex++) {
@@ -252,11 +252,19 @@ function calculateTrimmedEndPoint(gpsPoints: GPSPoint[][], targetDistanceMeters:
     return null;
 }
 
-function getTrimmedGpsTrip(gpsDraftDetails: GpsDraftDetails | undefined, trimmedEndPoint?: TrimmedGPSPoint): GPSPoint[][];
-function getTrimmedGpsTrip(gpsPoints: GPSPoint[][], trimmedEndPoint: TrimmedGPSPoint | undefined): GPSPoint[][];
-function getTrimmedGpsTrip(gpsData: GPSPoint[][] | GpsDraftDetails | undefined, trimmedEndPointProp?: TrimmedGPSPoint | undefined): GPSPoint[][] {
-    const gpsPoints = Array.isArray(gpsData) ? gpsData : getGpsPoints(gpsData);
-    const trimmedEndPoint = trimmedEndPointProp ?? (Array.isArray(gpsData) ? undefined : gpsData?.trimmedEndPoint);
+/**
+ * `Array.isArray()` is typed `(value: any) => value is any[]`, so it does not discriminate a union whose
+ * array member is read-only: the array stays in the type of the false branch. This guard does.
+ */
+function isGpsPointSegments(gpsData: ReadonlyDeep<GPSPoint[][]> | ReadonlyDeep<GpsDraftDetails> | undefined): gpsData is ReadonlyDeep<GPSPoint[][]> {
+    return Array.isArray(gpsData);
+}
+
+function getTrimmedGpsTrip(gpsDraftDetails: ReadonlyDeep<GpsDraftDetails> | undefined, trimmedEndPoint?: TrimmedGPSPoint): ReadonlyDeep<GPSPoint[][]>;
+function getTrimmedGpsTrip(gpsPoints: ReadonlyDeep<GPSPoint[][]>, trimmedEndPoint: TrimmedGPSPoint | undefined): ReadonlyDeep<GPSPoint[][]>;
+function getTrimmedGpsTrip(gpsData: ReadonlyDeep<GPSPoint[][]> | ReadonlyDeep<GpsDraftDetails> | undefined, trimmedEndPointProp?: TrimmedGPSPoint | undefined): ReadonlyDeep<GPSPoint[][]> {
+    const gpsPoints = isGpsPointSegments(gpsData) ? gpsData : getGpsPoints(gpsData);
+    const trimmedEndPoint = trimmedEndPointProp ?? (isGpsPointSegments(gpsData) ? undefined : gpsData?.trimmedEndPoint);
 
     if (!trimmedEndPoint) {
         return gpsPoints;
@@ -276,7 +284,7 @@ function getTrimmedGpsTrip(gpsData: GPSPoint[][] | GpsDraftDetails | undefined,
     return gpsPoints.slice(0, trimmedEndPointSegment).concat([updatedSegment]);
 }
 
-function gpsPointsToMapboxCoordinates(coordinates: GPSPoint[][]): Coordinate[][] {
+function gpsPointsToMapboxCoordinates(coordinates: ReadonlyDeep<GPSPoint[][]>): Coordinate[][] {
     return coordinates.map((segment): Coordinate[] => segment.map(({lat, long}) => [long, lat]));
 }
 

src/libs/OnyxReadonly.ts

@@ -0,0 +1,41 @@
+import type {OnyxKey, OnyxValue} from 'react-native-onyx';
+import type {ReadonlyDeep} from 'type-fest';
+
+import OnyxLib from 'react-native-onyx';
+
+/**
+ * The value `Onyx.get()` really hands back: the cached object itself, shared with every other reader of
+ * that key. Typing it deeply read-only is what turns the "the result of `Onyx.get()` MUST NOT be mutated"
+ * convention into a rule the compiler enforces instead of a comment reviewers have to remember.
+ */
+type OnyxReadonlyValue<TKey extends OnyxKey> = ReadonlyDeep<OnyxValue<TKey>>;
+
+/**
+ * Local experiment for the Onyx immutability audit (https://github.com/Expensify/App/issues/71206).
+ *
+ * `Onyx.get()` re-typed so a one-shot read can only produce read-only data. Runtime is untouched: this is
+ * the same `Onyx` object with a narrower type on one method, so nothing about behaviour, cache identity or
+ * bundle size changes. It lives in the app rather than in the library so the cost can be measured on real
+ * call sites before it is proposed as the library's own signature.
+ *
+ * `get()` is deliberately the only method touched. It is the read whose result reaches the fewest places, and
+ * the one whose contract already says the value must not be mutated. The rest of the API is left alone:
+ *
+ * - `useOnyx()` hands back the same shared object, but its result flows through render and hundreds of props.
+ *   Typing it read-only costs 303 errors in 165 files shallow, or 2421 in 731 deep, the latter including
+ *   ~110 that no signature change can fix (`ReadonlyDeep` breaks the shape of `ReportAction['message']`).
+ * - The writers keep their mutable input types. Widening them to accept read-only data would cost 23 errors
+ *   in 6 files and would let a value read with `get()` be written straight back, but it is a separate change
+ *   and does not have to land first. Until it does, a caller that reads with `get()` and writes the same
+ *   value back rebuilds the mutable parts itself.
+ */
+// The cast is the whole point of the file: the same object, with a narrower type on one method. There is no
+// expression TypeScript can check here, because `Promise<OnyxValue<TKey>>` and
+// `Promise<ReadonlyDeep<OnyxValue<TKey>>>` are not comparable while `TKey` is still generic.
+// eslint-disable-next-line @typescript-eslint/no-unsafe-type-assertion
+const OnyxReadonly = OnyxLib as Omit<typeof OnyxLib, 'get'> & {
+    get: <TKey extends OnyxKey>(key: TKey) => Promise<OnyxReadonlyValue<TKey>>;
+};
+
+export default OnyxReadonly;
+export type {OnyxReadonlyValue};

src/libs/actions/Delegate.ts

@@ -22,6 +22,7 @@ import type Credentials from '@src/types/onyx/Credentials';
 import type Session from '@src/types/onyx/Session';
 
 import type {OnyxEntry, OnyxUpdate} from 'react-native-onyx';
+import type {ReadonlyDeep} from 'type-fest';
 
 import HybridAppModule from '@expensify/react-native-hybrid-app';
 import Onyx from 'react-native-onyx';
@@ -76,7 +77,7 @@ function clearOnyxForDelegateTransition(): Promise<void> {
 
 type WithDelegatedAccess = {
     // Optional keeps call sites clean, but still encourages passing `account?.delegatedAccess`.
-    delegatedAccess: DelegatedAccess | undefined;
+    delegatedAccess: ReadonlyDeep<DelegatedAccess> | undefined;
 };
 
 type WithCredentials = {