Skip to content

Commit

Permalink
Change guidelines to forbid defaulting values
Browse files Browse the repository at this point in the history
  • Loading branch information
fabioh8010 committed Nov 6, 2024
1 parent 5a63cc5 commit ab94ee9
Show file tree
Hide file tree
Showing 2 changed files with 57 additions and 7 deletions.
57 changes: 50 additions & 7 deletions contributingGuides/STYLE.md
Original file line number Diff line number Diff line change
Expand Up @@ -477,20 +477,63 @@ if (ref.current && 'getBoundingClientRect' in ref.current) {

### Default value for inexistent IDs

Use `'-1'` or `-1` when there is a possibility that the ID property of an Onyx value could be `null` or `undefined`.
Use `CONST.DEFAULT_NUMBER_ID` when there is a possibility that the number ID property of an Onyx value could be `null` or `undefined`. **Do not default string IDs to any value unless absolutely necessary**, in case it's necessary use `CONST.DEFAULT_STRING_ID` instead.

``` ts
// BAD
const foo = report?.reportID ?? '';
const bar = report?.reportID ?? '0';
const accountID = report?.ownerAccountID ?? -1;
const accountID = report?.ownerAccountID ?? 0;
const reportID = report?.reportID ?? '-1';

report ? report.reportID : '0';
report ? report.reportID : '';
// BAD
report ? report.ownerAccountID : -1;

// GOOD
const accountID = report?.ownerAccountID ?? CONST.DEFAULT_NUMBER_ID;
const accountID = report?.ownerAccountID ?? CONST.DEFAULT_NUMBER_ID;
const reportID = report?.reportID;

// GOOD
const foo = report?.reportID ?? '-1';
report ? report.ownerAccountID : CONST.DEFAULT_NUMBER_ID;
```

Here are some common cases you may face when fixing your code to remove the default values.

#### **Case 1**: Argument of type 'string | undefined' is not assignable to parameter of type 'string'.

```diff
-Report.getNewerActions(newestActionCurrentReport?.reportID ?? '-1', newestActionCurrentReport?.reportActionID ?? '-1');
+Report.getNewerActions(newestActionCurrentReport?.reportID, newestActionCurrentReport?.reportActionID);
```

> error TS2345: Argument of type 'string | undefined' is not assignable to parameter of type 'string'. Type 'undefined' is not assignable to type 'string'.

We need to change `Report.getNewerActions()` arguments to allow `undefined`. By doing that we could add a condition that return early if one of the parameters are falsy, preventing the code (which is expecting defined IDs) from executing.

```diff
-function getNewerActions(reportID: string, reportActionID: string) {
+function getNewerActions(reportID: string | undefined, reportActionID: string | undefined) {
+ if (!reportID || !reportActionID) {
+ return;
+ }
```
#### **Case 2**: Type 'undefined' cannot be used as an index type.
```diff
function MoneyRequestView({report, shouldShowAnimatedBackground, readonly = false, updatedTransaction, isFromReviewDuplicates = false}: MoneyRequestViewProps) {
- const parentReportAction = parentReportActions?.[report?.parentReportActionID ?? '-1'];
+ const parentReportAction = parentReportActions?.[report?.parentReportActionID];
```
> error TS2538: Type 'undefined' cannot be used as an index type.
This error is inside a component, so we can't just make conditions with early returns here. We can instead use `String(report?.parentReportActionID)` to try to convert the value to `string`. If the value is `undefined` the result string will be `'undefined'`, which will be used to find a record inside `parentReportActions` and, same as `-1`, would find nothing.
report ? report.reportID : '-1';
```diff
function MoneyRequestView({report, shouldShowAnimatedBackground, readonly = false, updatedTransaction, isFromReviewDuplicates = false}: MoneyRequestViewProps) {
- const parentReportAction = parentReportActions?.[report?.parentReportActionID ?? '-1'];
+ const parentReportAction = parentReportActions?.[String(report?.parentReportActionID)];
```
### Extract complex types
Expand Down
7 changes: 7 additions & 0 deletions src/CONST.ts
Original file line number Diff line number Diff line change
Expand Up @@ -16,6 +16,11 @@ import type PlaidBankAccount from './types/onyx/PlaidBankAccount';
const EMPTY_ARRAY = Object.freeze([]);
const EMPTY_OBJECT = Object.freeze({});

const DEFAULT_NUMBER_ID = 0;

/** Only default a string ID to this value if absolutely necessary! */
const DEFAULT_STRING_ID = '';

const CLOUDFRONT_DOMAIN = 'cloudfront.net';
const CLOUDFRONT_URL = `https://d2k5nsl2zxldvw.${CLOUDFRONT_DOMAIN}`;
const ACTIVE_EXPENSIFY_URL = Url.addTrailingForwardSlash(Config?.NEW_EXPENSIFY_URL ?? 'https://new.expensify.com');
Expand Down Expand Up @@ -833,6 +838,8 @@ const CONST = {
CLOUDFRONT_URL,
EMPTY_ARRAY,
EMPTY_OBJECT,
DEFAULT_NUMBER_ID,
DEFAULT_STRING_ID,
USE_EXPENSIFY_URL,
GOOGLE_MEET_URL_ANDROID: 'https://meet.google.com',
GOOGLE_DOC_IMAGE_LINK_MATCH: 'googleusercontent.com',
Expand Down

0 comments on commit ab94ee9

Please sign in to comment.