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.

Sign up for GitHub

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
merged 6 commits into from
Nov 19, 2024
Merged

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

Show file tree
Hide file tree
Changes from 2 commits
Commits
Show all changes
6 commits
Select commit Hold shift + click to select a range
ab94ee9
Change guidelines to forbid defaulting values
fabioh8010 Nov 6, 2024
088f1c3
Address comments
fabioh8010 Nov 7, 2024
208b743
Add explanation for default IDs
fabioh8010 Nov 8, 2024
d1e7405
Merge branch 'main' into feature/no-default-values-guidelines
fabioh8010 Nov 12, 2024
7bcabe0
Remove DEFAULT_STRING_ID and add more instructions
fabioh8010 Nov 12, 2024
23ccd7e
Adjust guidelines
fabioh8010 Nov 12, 2024
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
59 changes: 52 additions & 7 deletions contributingGuides/STYLE.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -477,20 +477,65 @@ 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.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
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.
Use `CONST.DEFAULT_NUMBER_ID` when there is a possibility that and ID that's a number could be `null` or `undefined`.
Use `CONST.DEFAULT_STRING_ID` when there is a possibility that and ID that's a string could be `null` or `undefined`.
**Do not default IDs to any value unless absolutely necessary**

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm confused by that suggestion.

Can we please include some examples for why we have these guidelines and our reasoning for them? I think explaining the why will help people follow the suggestions.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I guess @iwiznia you meant an ID instead of and ID.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Same here as my previous comment. We all agreed in the proposal that we shouldn't default string IDs to any value, because they can be passed to Onyx when accessing records of a collection and cause bugs/crashes. And we also discussed here that in a exceptional case, if we really need to default the string ID to some value we are going to default them to ''.

Update guidelines to require developers to default all number IDs to 0 and forbid defaulting string IDs unless absolutely necessary. Include examples about how to deal with common scenarios when removing the default values.

Do you still agree with the proposed solution?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sounds good. Please also include an explanation for why we make these recommendations.

Something like "The default number ID is falsy so that checks like !someID work, and it's currently set to 0 which is the same default value we use on the backend. We use a constant so it can be easily changed across the codebase if needed.". Please explain similarly for our guidance on number IDs.

Copy link
Contributor

@iwiznia iwiznia Nov 7, 2024 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

unless absolutely necessary

Please also explain and give an example of when this is necessary

Copy link
Contributor Author

@fabioh8010 fabioh8010 Nov 8, 2024 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@neil-marcellini I've added some explanations as requested, please let me know if it's good enough

Please also explain and give an example of when this is necessary

@iwiznia Unfortunately I couldn't think of any examples defaulting the string ID would be necessary. We just added because, given the codebase's size, it could happen so we are already covering this situation in the guidelines. If you think this is not valid we can instead remove this exception and DEFAULT_STRING_ID for now.

Copy link
Contributor

@iwiznia iwiznia Nov 8, 2024 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Didn't we say in the thread you linked that we needed to default to undefined or 0 when connecting to an onyx key?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Didn't we say in the thread you linked that we needed to default to undefined or 0 when connecting to an onyx key?

That's true, but that would be not providing a default, or using a number right?

It's sounding like we really can't come up with a good case for defaulting a string. I think it would be more clear to leave that out entirely. We could add a note that if you find a reasonable exception or are confused about a case to raise your question in #expensify-open-source.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I removed DEFAULT_STRING_ID and added the note.


``` 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 reportID = report?.reportID;
fabioh8010 marked this conversation as resolved.
Show resolved Hide resolved

// GOOD
fabioh8010 marked this conversation as resolved.
Show resolved Hide resolved
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 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
7 changes: 7 additions & 0 deletions src/CONST.ts
View file
Open in desktop
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;
fabioh8010 marked this conversation as resolved.
Show resolved Hide resolved

/** 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
Loading