|
66 | 66 |
|
67 | 67 | --- |
68 | 68 |
|
69 | | -### [PERF-2] Use early returns in array iteration methods |
| 69 | +### [PERF-2] Return early before expensive work |
70 | 70 |
|
71 | | -- **Search patterns**: `.every(`, `.some(`, `.find(`, `.filter(` |
| 71 | +- **Search patterns**: Function bodies where `if (!param)` or `if (param === undefined)` appears AFTER function calls that use `param` |
72 | 72 |
|
73 | 73 | - **Condition**: Flag ONLY when ALL of these are true: |
74 | 74 |
|
75 | | - - Using .every(), .some(), .find(), .filter() or similar function |
76 | | - - Function contains an "expensive operation" (defined below) |
77 | | - - There exists a simple property check that could eliminate items earlier |
78 | | - - The simple check is performed AFTER the expensive operation |
79 | | - |
80 | | - **Expensive operations are**: |
81 | | - |
82 | | - - Function calls (except simple getters/property access) |
83 | | - - Regular expressions |
84 | | - - Object/array iterations |
85 | | - - Math calculations beyond basic arithmetic |
86 | | - |
87 | | - **Simple checks are**: |
88 | | - |
89 | | - - Property existence (!obj.prop, obj.prop === undefined) |
90 | | - - Boolean checks (obj.isActive) |
91 | | - - Primitive comparisons (obj.id === 5) |
92 | | - - Type checks (typeof, Array.isArray) |
| 75 | + - Code performs expensive work (function calls, iterations, API/Onyx reads) |
| 76 | + - A simple check could short-circuit earlier |
| 77 | + - The simple check happens AFTER the expensive work |
93 | 78 |
|
94 | 79 | **DO NOT flag if**: |
95 | 80 |
|
96 | | - - No expensive operations exist |
97 | | - - Simple checks are already done first |
98 | | - - The expensive operation MUST run for all items (e.g., for side effects) |
| 81 | + - Simple checks already come first |
| 82 | + - Validation requires the computed result |
| 83 | + - Expensive work must run for side effects |
99 | 84 |
|
100 | | -- **Reasoning**: Expensive operations can be any long-running synchronous tasks (like complex calculations) and should be avoided when simple property checks can eliminate items early. This reduces unnecessary computation and improves iteration performance, especially on large datasets. |
| 85 | +- **Reasoning**: Early returns prevent wasted computation. Validate inputs before passing them to expensive operations. |
101 | 86 |
|
102 | 87 | Good: |
103 | 88 |
|
104 | 89 | ```ts |
105 | | -const areAllTransactionsValid = transactions.every((transaction) => { |
106 | | - if (!transaction.rawData || transaction.amount <= 0) { |
107 | | - return false; |
| 90 | +function clearReportActionErrors(reportID: string, reportAction: ReportAction) { |
| 91 | + if (!reportAction?.reportActionID) { |
| 92 | + return; |
108 | 93 | } |
109 | | - const validation = validateTransaction(transaction); |
110 | | - return validation.isValid; |
111 | | -}); |
| 94 | + |
| 95 | + const originalReportID = getOriginalReportID(reportID, reportAction); |
| 96 | + // ... |
| 97 | +} |
112 | 98 | ``` |
113 | 99 |
|
114 | 100 | Bad: |
115 | 101 |
|
116 | 102 | ```ts |
117 | | -const areAllTransactionsValid = transactions.every((transaction) => { |
118 | | - const validation = validateTransaction(transaction); |
119 | | - return validation.isValid; |
120 | | -}); |
| 103 | +function clearReportActionErrors(reportID: string, reportAction: ReportAction) { |
| 104 | + const originalReportID = getOriginalReportID(reportID, reportAction); |
| 105 | + |
| 106 | + if (!reportAction?.reportActionID) { |
| 107 | + return; |
| 108 | + } |
| 109 | + // ... |
| 110 | +} |
121 | 111 | ``` |
122 | 112 |
|
123 | 113 | --- |
@@ -996,6 +986,8 @@ function ReportScreen({ params: { reportID }}) { |
996 | 986 |
|
997 | 987 | - **Reasoning**: When parent components compute and pass behavioral state to children, if a child's requirements change, then parent components must change as well, increasing coupling and causing behavior to leak across concerns. Letting components own their behavior keeps logic local, allows independent evolution, and follows the principle: "If removing a child breaks parent behavior, coupling exists." |
998 | 988 |
|
| 989 | +**Distinction from CLEAN-REACT-PATTERNS-3**: This rule is about data flow DOWN (parent → child) — "Don't pass data the child can get itself." |
| 990 | + |
999 | 991 | Good (component owns its behavior): |
1000 | 992 |
|
1001 | 993 | - Component receives only IDs and handlers |
@@ -1079,6 +1071,226 @@ In this example: |
1079 | 1071 |
|
1080 | 1072 | --- |
1081 | 1073 |
|
| 1074 | +### [CLEAN-REACT-PATTERNS-3] Design context-free component contracts |
| 1075 | + |
| 1076 | +- **Search patterns**: Callback props with consumer-specific signatures like `(index: number) => void`, props used only to extract values for callbacks, refs used to access external component state, useImperativeHandle |
| 1077 | + |
| 1078 | +- **Condition**: Flag ONLY when BOTH of these are true: |
| 1079 | + |
| 1080 | + 1. A component's interface is shaped around a specific consumer's implementation rather than abstract capabilities |
| 1081 | + 2. AND at least ONE of the following manifestations is present: |
| 1082 | + - The component receives data only to extract values for callbacks (doesn't use it for rendering) |
| 1083 | + - Callback signatures encode consumer-specific assumptions (e.g., `(index: number) => void` for navigation) |
| 1084 | + - The component accesses external state through refs or imperative handles |
| 1085 | + |
| 1086 | + **Signs of violation:** |
| 1087 | + - Callback signatures that encode consumer assumptions: `navigateToWaypoint(index: number)` instead of `onAddWaypoint()` |
| 1088 | + - Props passed only to extract values for callbacks, not for rendering (e.g., `transaction` passed just to compute `waypoints.length`) |
| 1089 | + - Imperative access to external state via refs or `useImperativeHandle` |
| 1090 | + - Component requires modification to work in a different context |
| 1091 | + |
| 1092 | + **DO NOT flag if:** |
| 1093 | + - Component signals events with data it naturally owns (e.g., `onChange(value)` for an input, `onSelectItem(item)` for a list) |
| 1094 | + - Callbacks are abstract actions the component can trigger (e.g., `onAddStop()`, `onSubmit()`) |
| 1095 | + - State coordination happens at a higher level with clear data flow |
| 1096 | + |
| 1097 | + **What makes a contract "abstract":** |
| 1098 | + - Callback describes *what happened* in component terms: `onAddStop`, `onSelect`, `onChange` |
| 1099 | + - Callback does NOT describe *what consumer should do*: `navigateToWaypoint(index)`, `updateParentState(value)` |
| 1100 | + - Props are used for rendering or internal logic, not just to compute callback arguments |
| 1101 | + - Component works without modification in a different context |
| 1102 | + |
| 1103 | +- **Reasoning**: A component's contract should expose its capabilities abstractly, not encode assumptions about how it will be used. When interfaces leak consumer-specific details, the component becomes coupled to that context and requires modification for reuse. Good contracts signal *what the component can do*, not *what the consumer needs*. |
| 1104 | + |
| 1105 | +**Distinction from CLEAN-REACT-PATTERNS-2**: PATTERNS-2 ensures components fetch their own data. This rule ensures components expose abstract capabilities, not consumer-specific interfaces. |
| 1106 | + |
| 1107 | +Good (abstract contract): |
| 1108 | + |
| 1109 | +- Interface exposes capability: "user can add a stop" |
| 1110 | +- Implementation details (index computation, navigation) stay with consumer |
| 1111 | +- Component is reusable in any context needing an "add stop" action |
| 1112 | + |
| 1113 | +```tsx |
| 1114 | +<DistanceRequestFooter |
| 1115 | + onAddStop={() => { |
| 1116 | + const nextIndex = Object.keys(transaction?.comment?.waypoints ?? {}).length; |
| 1117 | + Navigation.navigate(ROUTES.MONEY_REQUEST_STEP_WAYPOINT.getRoute(..., nextIndex.toString(), ...)); |
| 1118 | + }} |
| 1119 | +/> |
| 1120 | + |
| 1121 | +// in DistanceRequestFooter |
| 1122 | +<Button onPress={onAddStop}>{translate('distance.addStop')}</Button> |
| 1123 | +``` |
| 1124 | + |
| 1125 | +Bad (contract leaks consumer assumptions): |
| 1126 | + |
| 1127 | +- Callback `navigateToWaypointEditPage(index: number)` encodes routing assumption |
| 1128 | +- `transaction` prop exists only to compute index for callback |
| 1129 | +- Requires modification if consumer navigates differently |
| 1130 | + |
| 1131 | +```tsx |
| 1132 | +type DistanceRequestFooterProps = { |
| 1133 | + waypoints?: WaypointCollection; |
| 1134 | + navigateToWaypointEditPage: (index: number) => void; // Encodes routing assumption |
| 1135 | + transaction: OnyxEntry<Transaction>; |
| 1136 | + policy: OnyxEntry<Policy>; |
| 1137 | +}; |
| 1138 | + |
| 1139 | +// in IOURequestStepDistance |
| 1140 | +<DistanceRequestFooter |
| 1141 | + waypoints={waypoints} |
| 1142 | + navigateToWaypointEditPage={navigateToWaypointEditPage} |
| 1143 | + transaction={transaction} |
| 1144 | + policy={policy} |
| 1145 | +/> |
| 1146 | + |
| 1147 | +// in DistanceRequestFooter - computes value for consumer's callback |
| 1148 | +<Button |
| 1149 | + onPress={() => navigateToWaypointEditPage(Object.keys(transaction?.comment?.waypoints ?? {}).length)} |
| 1150 | + text={translate('distance.addStop')} |
| 1151 | +/> |
| 1152 | +``` |
| 1153 | + |
| 1154 | +Good (independent contracts): |
| 1155 | + |
| 1156 | +- Each component has a self-contained interface |
| 1157 | +- State coordination happens at composition level |
| 1158 | + |
| 1159 | +```tsx |
| 1160 | +function EditProfile() { |
| 1161 | + const [formData, setFormData] = useState<FormData>(); |
| 1162 | + return ( |
| 1163 | + <> |
| 1164 | + <Form onChangeFormData={setFormData} /> |
| 1165 | + <SaveButton onSave={() => API.save(formData)} /> |
| 1166 | + </> |
| 1167 | + ); |
| 1168 | +} |
| 1169 | +``` |
| 1170 | + |
| 1171 | +Bad (coupled contracts): |
| 1172 | + |
| 1173 | +- `SaveButton` interface requires knowledge of `Form`'s internals |
| 1174 | +- Neither component works independently |
| 1175 | + |
| 1176 | +```tsx |
| 1177 | +function SaveButton({ getSiblingFormData }: { getSiblingFormData: () => FormData }) { |
| 1178 | + const handleSave = () => { |
| 1179 | + const formData = getSiblingFormData(); // Reaches into sibling |
| 1180 | + API.save(formData); |
| 1181 | + }; |
| 1182 | + return <Button onPress={handleSave}>Save</Button>; |
| 1183 | +} |
| 1184 | + |
| 1185 | +// Parent wires siblings together |
| 1186 | +<Form ref={formRef} /> |
| 1187 | +<SaveButton getSiblingFormData={() => formRef.current?.getData()} /> |
| 1188 | +``` |
| 1189 | + |
| 1190 | +--- |
| 1191 | + |
| 1192 | +### [CLEAN-REACT-PATTERNS-5] Keep state and subscriptions narrow |
| 1193 | + |
| 1194 | +- **Search patterns**: Contexts/hooks/stores exposing large bundled objects, providers with many unrelated `useOnyx` calls, state structures mixing unrelated concerns |
| 1195 | + |
| 1196 | +- **Condition**: Flag when a state structure (context, hook, store, or subscription) bundles unrelated concerns together, causing consumers to re-render when data they don't use changes. |
| 1197 | + |
| 1198 | + **Signs of violation:** |
| 1199 | + - State provider (context, hook, or store) that bundles unrelated data (e.g., navigation state + list data + cache utilities in one structure) |
| 1200 | + - State object where properties serve different purposes and change independently |
| 1201 | + - Multiple unrelated subscriptions (`useOnyx`, `useContext`, store selectors) aggregated into a single exposed value |
| 1202 | + - Consumers of a state source that only use a subset of the provided values |
| 1203 | + |
| 1204 | + **DO NOT flag if:** |
| 1205 | + - State values are cohesive — they change together and serve the same purpose (e.g., `keyboardHeight` + `isKeyboardShown` both relate to keyboard state) |
| 1206 | + - The state structure is intentionally designed as an aggregation point and consumers use most/all values |
| 1207 | + - Individual `useOnyx` calls without selectors — this is covered by [PERF-11] |
| 1208 | + |
| 1209 | +- **Reasoning**: When unrelated pieces of data are grouped into a single state structure, if an unused part changes, then all consumers re-render unnecessarily. This silently expands render scope, increases coupling, and makes performance regressions hard to detect. Structuring state around cohesive concerns ensures render scope stays predictable and changes remain local. |
| 1210 | + |
| 1211 | +**Distinction from PERF-11**: PERF-11 addresses individual `useOnyx` selector usage. This rule addresses state structure — how multiple values are grouped and exposed to consumers via contexts, hooks, or stores. |
| 1212 | + |
| 1213 | +**Distinction from CLEAN-REACT-PATTERNS-2**: PATTERNS-2 addresses data flow direction — parent shouldn't fetch data just to pass to children. This rule addresses how state is structured and grouped within any state provider. |
| 1214 | + |
| 1215 | +Good (cohesive state — all values serve one purpose): |
| 1216 | + |
| 1217 | +- All state relates to one concern (keyboard) |
| 1218 | +- Values change together — no wasted re-renders |
| 1219 | +- Derived state computed inline, not stored separately |
| 1220 | + |
| 1221 | +```tsx |
| 1222 | +type KeyboardStateContextValue = { |
| 1223 | + isKeyboardShown: boolean; |
| 1224 | + isKeyboardActive: boolean; |
| 1225 | + keyboardHeight: number; |
| 1226 | +}; |
| 1227 | + |
| 1228 | +function KeyboardStateProvider({children}: ChildrenProps) { |
| 1229 | + const [keyboardHeight, setKeyboardHeight] = useState(0); |
| 1230 | + const [isKeyboardActive, setIsKeyboardActive] = useState(false); |
| 1231 | + |
| 1232 | + useEffect(() => { |
| 1233 | + const showListener = KeyboardEvents.addListener('keyboardDidShow', (e) => { |
| 1234 | + setKeyboardHeight(e.height); |
| 1235 | + setIsKeyboardActive(true); |
| 1236 | + }); |
| 1237 | + const hideListener = KeyboardEvents.addListener('keyboardDidHide', () => { |
| 1238 | + setKeyboardHeight(0); |
| 1239 | + setIsKeyboardActive(false); |
| 1240 | + }); |
| 1241 | + return () => { |
| 1242 | + showListener.remove(); |
| 1243 | + hideListener.remove(); |
| 1244 | + }; |
| 1245 | + }, []); |
| 1246 | + |
| 1247 | + const contextValue = useMemo(() => ({ |
| 1248 | + keyboardHeight, |
| 1249 | + isKeyboardShown: keyboardHeight !== 0, // Derived, not separate state |
| 1250 | + isKeyboardActive, |
| 1251 | + }), [keyboardHeight, isKeyboardActive]); |
| 1252 | + |
| 1253 | + return <KeyboardStateContext.Provider value={contextValue}>{children}</KeyboardStateContext.Provider>; |
| 1254 | +} |
| 1255 | +``` |
| 1256 | + |
| 1257 | +Bad (grab-bag state — bundles unrelated concerns): |
| 1258 | + |
| 1259 | +- State provider subscribes to many unrelated Onyx collections |
| 1260 | +- Exposed value mixes navigation state, list data, membership data, and cache utilities |
| 1261 | +- Any consumer re-renders when ANY subscribed value changes |
| 1262 | + |
| 1263 | +```tsx |
| 1264 | +function SidebarOrderedReportsContextProvider({children}) { |
| 1265 | + // ❌ Many unrelated Onyx subscriptions bundled together |
| 1266 | + const [priorityMode] = useOnyx(ONYXKEYS.NVP_PRIORITY_MODE); |
| 1267 | + const [chatReports] = useOnyx(ONYXKEYS.COLLECTION.REPORT); |
| 1268 | + const [policies] = useOnyx(ONYXKEYS.COLLECTION.POLICY); |
| 1269 | + const [transactions] = useOnyx(ONYXKEYS.COLLECTION.TRANSACTION); |
| 1270 | + const [transactionViolations] = useOnyx(ONYXKEYS.COLLECTION.TRANSACTION_VIOLATIONS); |
| 1271 | + const [reportNameValuePairs] = useOnyx(ONYXKEYS.COLLECTION.REPORT_NAME_VALUE_PAIRS); |
| 1272 | + const [reportsDrafts] = useOnyx(ONYXKEYS.COLLECTION.REPORT_DRAFT_COMMENT); |
| 1273 | + const [betas] = useOnyx(ONYXKEYS.BETAS); |
| 1274 | + const [reportAttributes] = useOnyx(ONYXKEYS.DERIVED.REPORT_ATTRIBUTES); |
| 1275 | + |
| 1276 | + // ❌ Context value mixes unrelated concerns |
| 1277 | + const contextValue = { |
| 1278 | + orderedReports, // List data |
| 1279 | + orderedReportIDs, // List data |
| 1280 | + currentReportID, // Navigation state |
| 1281 | + policyMemberAccountIDs, // Policy membership |
| 1282 | + clearLHNCache, // Cache management utility |
| 1283 | + }; |
| 1284 | + |
| 1285 | + return <Context.Provider value={contextValue}>{children}</Context.Provider>; |
| 1286 | +} |
| 1287 | + |
| 1288 | +// A component needing only currentReportID re-renders when orderedReports changes |
| 1289 | +// A component needing only policyMemberAccountIDs re-renders when navigation changes |
| 1290 | +``` |
| 1291 | + |
| 1292 | +--- |
| 1293 | + |
1082 | 1294 | ## Instructions |
1083 | 1295 |
|
1084 | 1296 | 1. **First, get the list of changed files and their diffs:** |
|
0 commit comments