[No QA] Change guidelines to forbid defaulting values #52094
Conversation
fabioh8010
changed the title
melvin-bot
bot
requested review from
paultsimura
and removed request for
a team
|
@paultsimura Please copy/paste the Reviewer Checklist from here into a new comment on this PR and complete it. If you have the K2 extension, you can simply click: [this button] |
Reviewer Checklist
Screenshots/VideosAndroid: NativeAndroid: mWeb ChromeiOS: NativeiOS: mWeb SafariMacOS: Chrome / SafariMacOS: Desktop |
contributingGuides/STYLE.md
Outdated
|
|
||
| > 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. |
There was a problem hiding this comment.
May I suggest a little rephrase to make it a bit more readable?
| 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. |
There was a problem hiding this comment.
I don't get why we are suggesting this. Why not default to ""? We never want "undefined" to be used
There was a problem hiding this comment.
We are suggesting to remove the default value because parentReportActionID is a string and according to the final proposal we agreed to not default string IDs to any value, unless absolutely necessary.
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.
When using String(report?.parentReportActionID), we will have 'undefined' if report?.parentReportActionID is undefined, so we will try to access the 'undefined' record of parentReportActions. Same as '-1' or even '', this record should never exist so it's safe to do it.
However, I can change the diff example to make it clear that I was talking about Onyx usage, wdyt?
function MoneyRequestView({report, shouldShowAnimatedBackground, readonly = false, updatedTransaction, isFromReviewDuplicates = false}: MoneyRequestViewProps) {
const [parentReportActions] = useOnyx(`${ONYXKEYS.COLLECTION.REPORT_ACTIONS}${parentReportID}`);
- const parentReportAction = parentReportActions?.[report?.parentReportActionID ?? '-1'];
+ const parentReportAction = parentReportActions?.[report?.parentReportActionID];
|
@fabioh8010 is this PR the only action item for the related GH? Will we create some GH actions or diff lint rules? |
|
Removed the |
contributingGuides/STYLE.md
Outdated
| @@ -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. | |||
There was a problem hiding this comment.
| 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** |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
I guess @iwiznia you meant an ID instead of and ID.
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
unless absolutely necessary
Please also explain and give an example of when this is necessary
There was a problem hiding this comment.
@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.
There was a problem hiding this comment.
Didn't we say in the thread you linked that we needed to default to undefined or 0 when connecting to an onyx key?
There was a problem hiding this comment.
Didn't we say in the thread you linked that we needed to default to
undefinedor0when 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.
There was a problem hiding this comment.
I removed DEFAULT_STRING_ID and added the note.
contributingGuides/STYLE.md
Outdated
|
|
||
| > 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. |
There was a problem hiding this comment.
I don't get why we are suggesting this. Why not default to ""? We never want "undefined" to be used
contributingGuides/STYLE.md
Outdated
| @@ -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. | |||
There was a problem hiding this comment.
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.
@paultsimura No, I'm going to create more PRs for the ESLint rule, Onyx improvement, etc. |
|
@iwiznia @neil-marcellini @paultsimura I've made some changes and answered some of the comments, thank you all for the feedback. Could tou take another look please? |
contributingGuides/STYLE.md
Outdated
| @@ -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. | |||
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
Thank you for your latest updates, they look really good. I think the only thing remaining is to remove the default string ID and any mentions of it, assuming @iwiznia agrees.
|
@neil-marcellini If @iwiznia agree with the last discussion, we should also remove |
|
Bump @neil-marcellini 🙂 |
|
Neil was OOO last week. Hopefully we'll get an update today 😌 |
|
✋ This PR was not deployed to staging yet because QA is ongoing. It will be automatically deployed to staging after the next production release. |
|
🚀 Deployed to staging by https://github.com/neil-marcellini in version: 9.0.65-0 🚀
|
|
🚀 Deployed to production by https://github.com/chiragsalian in version: 9.0.65-5 🚀
|
Explanation of Change
As part of #50360 plan to remove default values, this PR updates the default values guidelines. Additionally, it creates two variable for string and number default IDs, thus addressing this comment.
Fixed Issues
#50360
PROPOSAL: #50360 (comment)
Tests
N/A
Offline tests
N/A
QA Steps
// TODO: These must be filled out, or the issue title must include "[No QA]."
N/A
PR Author Checklist
### Fixed Issuessection aboveTestssectionOffline stepssectionQA stepssectiontoggleReportand notonIconClick)src/languages/*files and using the translation methodSTYLE.md) were followedAvatar, I verified the components usingAvatarare working as expected)StyleUtils.getBackgroundAndBorderStyle(theme.componentBG))Avataris modified, I verified thatAvataris working as expected in all cases)Designlabel and/or tagged@Expensify/designso the design team can review the changes.ScrollViewcomponent to make it scrollable when more elements are added to the page.mainbranch was merged into this PR after a review, I tested again and verified the outcome was still expected according to theTeststeps.Screenshots/Videos
Android: Native
Android: mWeb Chrome
iOS: Native
iOS: mWeb Safari
MacOS: Chrome / Safari
MacOS: Desktop