[PropTypes / Default Props] Modify components to be compatible with new Onyx Lint rule #15590
Conversation
melvin-bot
bot
requested review from
grgia and
mollfpr
and removed request for
a team
| @@ -83,7 +87,7 @@ class WalletStatementPage extends React.Component { | |||
| } | |||
|
|
|||
| render() { | |||
| moment.locale(lodashGet(this.props, 'preferredLocale', 'en')); | |||
| moment.locale(this.props.preferredLocale); | |||
There was a problem hiding this comment.
Is this alright? Had to do this to avoid the 'unused prop' error.
There was a problem hiding this comment.
Why is the preferredLocale prop being added here?
There was a problem hiding this comment.
Based on the comment from the code added, it's used to change the modal title according to the current locale.
Reviewer Checklist
Screenshots/VideosWebMobile Web - ChromeMobile Web - SafariDesktopiOSAndroid |
src/pages/NewChatPage.js
Outdated
|
|
||
| /** Session of currently logged in user */ | ||
| session: PropTypes.shape({ | ||
| email: PropTypes.string.isRequired, | ||
| }).isRequired, | ||
| }), |
There was a problem hiding this comment.
@Prince-Mendiratta It's weird that the session is not detected as unused if it has a shape type but detected as unused if objectOf.
There was a problem hiding this comment.
@mollfpr That is super interesting, changing PropTypes.shape to PropTypes.objectOf reveals a big bunch of unused props across the app.
There was a problem hiding this comment.
There are around 20 such cases.
There was a problem hiding this comment.
redundant prop, removed.
There was a problem hiding this comment.
@Prince-Mendiratta Yeah, but it is not part of your lint so let it slide for now.
There was a problem hiding this comment.
@grgia Maybe we can create another issue to deal with removing unused Onyx keys and props across the app?
| email: null, | ||
| }, | ||
| chatReport: { | ||
| participants: [], |
There was a problem hiding this comment.
Why is the default value only participants and the type for chatReport is only reportID?
There was a problem hiding this comment.
I'm unsure what to do here, do we add participants to propTypes or reportID to defaultProps? The other instance for a similar component is:
https://github.com/Expensify/App/blob/main/src/components/ReportActionItem/IOUAction.js
There was a problem hiding this comment.
Okay, let's just fix what the lint complains about.
There was a problem hiding this comment.
It's not clear either what the function needs from the report data. I'll take a look deeper into this.
There was a problem hiding this comment.
@Prince-Mendiratta, we need all the keys from chatReport. For example, getPayMoneyRequestParams is updating the report data.
There was a problem hiding this comment.
@mollfpr I'm not too sure about the keys required in the chatReport, is this all?
/** chatReport associated with iouReport */
chatReport: PropTypes.shape({
/** Report ID associated with the transaction */
reportID: PropTypes.string,
/** The participants of this report */
participants: PropTypes.arrayOf(PropTypes.string),
/** Whether the chat report has an outstanding IOU */
hasOutstandingIOU: PropTypes.bool.isRequired,
}),
There was a problem hiding this comment.
@grgia Any suggestion on what we should put for the chatReport type?
There was a problem hiding this comment.
@Prince-Mendiratta At this moment, we can use the above snippet code.
There was a problem hiding this comment.
@mollfpr cool, updated.
|
@Prince-Mendiratta We have a conflict. |
|
@Prince-Mendiratta @mollfpr how can I best help here? Are all of the mentioned issues not reproducible locally? Maybe I need to trigger a fresh build? Or if tests are working well we can talk about a plan for final review and merge |
|
#15590 (comment) This issue only reproduces if the domain is not Screen.Recording.2023-03-21.at.19.28.20.movI tried running the web on the latest |
|
#15590 (comment) Still not able to reproduce it in DEV. @grgia @Prince-Mendiratta any idea? Here's the error from the
|
|
@mollfpr Did you update/merge main on DEV? If so then perhaps the build URL is outdated? |
|
@grgia I tried to reproduce this by merging the PR with upstream main branch, was still not able to reproduce either of the 2 issues on dev. I assume that both these issues are related to the testing domain cc @mollfpr |
|
@mollfpr @Prince-Mendiratta how do you feel about merging the conflict and then merging this? It seems like besides the URL tests, it tests fairly well? Have we addressed all the outstanding errors we encountered? |
Signed-off-by: Prince Mendiratta <[email protected]>
Signed-off-by: Prince Mendiratta <[email protected]>
|
@grgia I think it's safe to merge it; some of the concerns about the change have already been addressed and resolved it. The above issues are most likely due to the domain. |
|
@Prince-Mendiratta Could you resolve this lint error?
|
Signed-off-by: Prince Mendiratta <[email protected]>
|
@mollfpr Resolved! |
|
@Prince-Mendiratta another conflict 🤣 |
Signed-off-by: Prince Mendiratta <[email protected]>
|
@mollfpr resolved! I swear it's almost as if each time a new PR is merged on main, a conflict arises here haha 🥲 |
Signed-off-by: Prince Mendiratta <[email protected]>
|
✋ 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/grgia in version: 1.2.89-0 🚀
|
|
@Prince-Mendiratta @mollfpr @roryabraham Can you validate this internally? We are not able to easily identify each component |
|
I validated this and it seems to be working well. |
|
🚀 Deployed to production by https://github.com/luacmartins in version: 1.2.89-0 🚀
|
Details
With this PR, we are bumping
eslint-config-expensifytov2.0.31, to add the use of the new lint rule that checks and insists that any props originating fromwithOnyxshould be optional and a default value should be provided.This PR also modifies the existing components using the
withOnyxHOC so that the above mentioned code style is followed.Related PR: Expensify/eslint-config-expensify#58, more details there and in the proposal.
Fixed Issues
$ #14309
PROPOSAL: #14309 (comment)
Tests
The list of components affected by this PR are listed below, a total of 55 files, 51 components:
Each of these need to be navigated to ensure that no console errors are thrown.
Also, to test the rule itself a sample new component can be added making use of the
withOnyxHOC and ensure that no error is thrown if the coding style is followed correctly.Offline tests
N/A
QA Steps
All the components mentioned above need to be navigated to and ensure no console errors are thrown.
PR Author Checklist
### Fixed Issuessection aboveTestssectionOffline stepssectionQA stepssectiontoggleReportand notonIconClick)src/languages/*files and using the translation methodWaiting for Copylabel for a copy review on the original GH to get the correct copy.STYLE.md) were followedAvatar, I verified the components usingAvatarare working as expected)/** comment above it */thisproperly so there are no scoping issues (i.e. foronClick={this.submit}the methodthis.submitshould be bound tothisin the constructor)thisare necessary to be bound (i.e. avoidthis.submit = this.submit.bind(this);ifthis.submitis never passed to a component event handler likeonClick)StyleUtils.getBackgroundAndBorderStyle(themeColors.componentBG)Avataris modified, I verified thatAvataris working as expected in all cases)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
N/A