From ab94ee9076b5708e2deb91e6bff5e9da38cb00de Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?F=C3=A1bio=20Henriques?= Date: Wed, 6 Nov 2024 08:55:25 +0000 Subject: [PATCH 1/5] Change guidelines to forbid defaulting values --- contributingGuides/STYLE.md | 57 ++++++++++++++++++++++++++++++++----- src/CONST.ts | 7 +++++ 2 files changed, 57 insertions(+), 7 deletions(-) diff --git a/contributingGuides/STYLE.md b/contributingGuides/STYLE.md index e6660d848129..755f5228a8a7 100644 --- a/contributingGuides/STYLE.md +++ b/contributingGuides/STYLE.md @@ -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 diff --git a/src/CONST.ts b/src/CONST.ts index ddf9ebad5b66..89276c51dd13 100755 --- a/src/CONST.ts +++ b/src/CONST.ts @@ -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'); @@ -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', From 088f1c34b73889f2a4e9c07e4d1d77f4f0b24f41 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?F=C3=A1bio=20Henriques?= Date: Thu, 7 Nov 2024 10:10:11 +0000 Subject: [PATCH 2/5] Address comments --- contributingGuides/STYLE.md | 12 +++++++----- 1 file changed, 7 insertions(+), 5 deletions(-) diff --git a/contributingGuides/STYLE.md b/contributingGuides/STYLE.md index 755f5228a8a7..61e848835138 100644 --- a/contributingGuides/STYLE.md +++ b/contributingGuides/STYLE.md @@ -490,14 +490,13 @@ 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 report ? report.ownerAccountID : CONST.DEFAULT_NUMBER_ID; ``` -Here are some common cases you may face when fixing your code to remove the default values. +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'. @@ -522,13 +521,16 @@ We need to change `Report.getNewerActions()` arguments to allow `undefined`. By ```diff function MoneyRequestView({report, shouldShowAnimatedBackground, readonly = false, updatedTransaction, isFromReviewDuplicates = false}: MoneyRequestViewProps) { -- const parentReportAction = parentReportActions?.[report?.parentReportActionID ?? '-1']; -+ const parentReportAction = parentReportActions?.[report?.parentReportActionID]; + 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 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. +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. ```diff function MoneyRequestView({report, shouldShowAnimatedBackground, readonly = false, updatedTransaction, isFromReviewDuplicates = false}: MoneyRequestViewProps) { From 208b743d51e999c319c031549d9790d0bd477829 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?F=C3=A1bio=20Henriques?= Date: Fri, 8 Nov 2024 11:27:41 +0000 Subject: [PATCH 3/5] Add explanation for default IDs --- contributingGuides/STYLE.md | 19 ++++++++++--------- 1 file changed, 10 insertions(+), 9 deletions(-) diff --git a/contributingGuides/STYLE.md b/contributingGuides/STYLE.md index 61e848835138..30db009b47ed 100644 --- a/contributingGuides/STYLE.md +++ b/contributingGuides/STYLE.md @@ -479,21 +479,22 @@ if (ref.current && 'getBoundingClientRect' in ref.current) { 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. +> 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. +> +> 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. + ``` ts // BAD const accountID = report?.ownerAccountID ?? -1; -const accountID = report?.ownerAccountID ?? 0; -const reportID = report?.reportID ?? '-1'; - -// BAD -report ? report.ownerAccountID : -1; +const policyID = report?.policyID ?? '-1'; +const managerID = report ? report.managerID : 0; // GOOD const accountID = report?.ownerAccountID ?? CONST.DEFAULT_NUMBER_ID; -const reportID = report?.reportID; - -// GOOD -report ? 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. From 7bcabe0c1fd6ebef1f439f4fcbe04d04b53e1a41 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?F=C3=A1bio=20Henriques?= Date: Tue, 12 Nov 2024 08:08:19 +0000 Subject: [PATCH 4/5] Remove DEFAULT_STRING_ID and add more instructions --- contributingGuides/STYLE.md | 8 +++++--- src/CONST.ts | 4 ---- 2 files changed, 5 insertions(+), 7 deletions(-) diff --git a/contributingGuides/STYLE.md b/contributingGuides/STYLE.md index 30db009b47ed..eeadce444ccf 100644 --- a/contributingGuides/STYLE.md +++ b/contributingGuides/STYLE.md @@ -477,13 +477,15 @@ if (ref.current && 'getBoundingClientRect' in ref.current) { ### Default value for inexistent IDs -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 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. -> +> > 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 diff --git a/src/CONST.ts b/src/CONST.ts index d4cafaa57666..94438afbf046 100755 --- a/src/CONST.ts +++ b/src/CONST.ts @@ -18,9 +18,6 @@ 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'); @@ -838,7 +835,6 @@ const CONST = { EMPTY_ARRAY, EMPTY_OBJECT, DEFAULT_NUMBER_ID, - DEFAULT_STRING_ID, USE_EXPENSIFY_URL, EXPENSIFY_URL, GOOGLE_MEET_URL_ANDROID: 'https://meet.google.com', From 23ccd7e4253a5b6056487b4dd7e6975872024d47 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?F=C3=A1bio=20Henriques?= Date: Tue, 12 Nov 2024 23:55:25 +0000 Subject: [PATCH 5/5] Adjust guidelines --- contributingGuides/STYLE.md | 4 ++-- src/CONST.ts | 4 +--- 2 files changed, 3 insertions(+), 5 deletions(-) diff --git a/contributingGuides/STYLE.md b/contributingGuides/STYLE.md index eeadce444ccf..c7f05e661bd2 100644 --- a/contributingGuides/STYLE.md +++ b/contributingGuides/STYLE.md @@ -533,12 +533,12 @@ function MoneyRequestView({report, shouldShowAnimatedBackground, readonly = fals > 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. +This error is inside a component, so we can't simply use early return conditions here. Instead, we can check if `report?.parentReportActionID` is defined, if it is we can safely use it to find the record inside `parentReportActions`. If it's not defined, we just return `undefined`. ```diff function MoneyRequestView({report, shouldShowAnimatedBackground, readonly = false, updatedTransaction, isFromReviewDuplicates = false}: MoneyRequestViewProps) { - const parentReportAction = parentReportActions?.[report?.parentReportActionID ?? '-1']; -+ const parentReportAction = parentReportActions?.[String(report?.parentReportActionID)]; ++ const parentReportAction = report?.parentReportActionID ? parentReportActions?.[report.parentReportActionID] : undefined; ``` ### Extract complex types diff --git a/src/CONST.ts b/src/CONST.ts index 94438afbf046..4a71e261ca22 100755 --- a/src/CONST.ts +++ b/src/CONST.ts @@ -16,8 +16,6 @@ import type PlaidBankAccount from './types/onyx/PlaidBankAccount'; const EMPTY_ARRAY = Object.freeze([]); const EMPTY_OBJECT = Object.freeze({}); -const DEFAULT_NUMBER_ID = 0; - 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'); @@ -834,7 +832,7 @@ const CONST = { CLOUDFRONT_URL, EMPTY_ARRAY, EMPTY_OBJECT, - DEFAULT_NUMBER_ID, + DEFAULT_NUMBER_ID: 0, USE_EXPENSIFY_URL, EXPENSIFY_URL, GOOGLE_MEET_URL_ANDROID: 'https://meet.google.com',