Skip to content
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

[No QA] Change guidelines to forbid defaulting values #52094

Merged
Show file tree
Hide file tree
Changes from 5 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
64 changes: 56 additions & 8 deletions contributingGuides/STYLE.md
Original file line number Diff line number Diff line change
Expand Up @@ -477,20 +477,68 @@ 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!**

> Why? The default number ID (currently set to `0`, which matches the backend’s default) is a falsy value. This makes it compatible with conditions that check if an ID is set, such as `if (!ownerAccountID) {`. Since it’s stored as a constant, it can easily be changed across the codebase if needed.
fabioh8010 marked this conversation as resolved.
Show resolved Hide resolved
>
> However, defaulting string IDs to `'0'` breaks such conditions because `'0'` is a truthy value in JavaScript. Defaulting to `''` avoids this issue, but it can cause crashes or bugs if the ID is passed to Onyx. This is because `''` could accidentally subscribe to an entire Onyx collection instead of a single record.
>
> To address both problems, string IDs **should not have a default value**. This approach allows conditions like `if (!policyID) {` to work correctly, as `undefined` is a falsy value. At the same time, it prevents Onyx bugs: if `policyID` is used to subscribe to a specific Onyx record, a `policy_undefined` key will be used, and Onyx won’t return any records.
>
> In case you are confused or find a situation where you can't apply the rules mentioned above, please raise your question in the [`#expensify-open-source`](https://expensify.slack.com/archives/C01GTK53T8Q) Slack channel.

``` ts
// BAD
const foo = report?.reportID ?? '';
const bar = report?.reportID ?? '0';

report ? report.reportID : '0';
report ? report.reportID : '';
const accountID = report?.ownerAccountID ?? -1;
const policyID = report?.policyID ?? '-1';
const managerID = report ? report.managerID : 0;

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

Here are some common cases you may face when fixing your code to remove the old/bad 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');
neil-marcellini marked this conversation as resolved.
Show resolved Hide resolved
+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.
neil-marcellini marked this conversation as resolved.
Show resolved Hide resolved

```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 [parentReportActions] = useOnyx(`${ONYXKEYS.COLLECTION.REPORT_ACTIONS}${parentReportID}`, {
canEvict: false,
});
- 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 simply use early return conditions here. Instead, we can 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, just like `-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)];
neil-marcellini marked this conversation as resolved.
Show resolved Hide resolved
```

### Extract complex types
Expand Down
3 changes: 3 additions & 0 deletions src/CONST.ts
Original file line number Diff line number Diff line change
Expand Up @@ -16,6 +16,8 @@ import type PlaidBankAccount from './types/onyx/PlaidBankAccount';
const EMPTY_ARRAY = Object.freeze([]);
const EMPTY_OBJECT = Object.freeze({});

const DEFAULT_NUMBER_ID = 0;
fabioh8010 marked this conversation as resolved.
Show resolved Hide resolved

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 @@ -832,6 +834,7 @@ const CONST = {
CLOUDFRONT_URL,
EMPTY_ARRAY,
EMPTY_OBJECT,
DEFAULT_NUMBER_ID,
USE_EXPENSIFY_URL,
EXPENSIFY_URL,
GOOGLE_MEET_URL_ANDROID: 'https://meet.google.com',
Expand Down
Loading