-
Notifications
You must be signed in to change notification settings - Fork 3.2k
[PropTypes / Default Props] Modify components to be compatible with new Onyx Lint rule #15590
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
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Have added a few places where some extra attention / verification might be required, please resolve these too!
@@ -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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is this alright? Had to do this to avoid the 'unused prop' error.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why is the preferredLocale
prop being added here?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There are around 20 such cases.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
redundant prop, removed.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@Prince-Mendiratta Yeah, but it is not part of your lint so let it slide for now.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why is the default value only participants
and the type for chatReport
is only reportID?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Okay, let's just fix what the lint complains about.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@Prince-Mendiratta, we need all the keys from chatReport
. For example, getPayMoneyRequestParams
is updating the report data.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@grgia Any suggestion on what we should put for the chatReport
type?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@Prince-Mendiratta At this moment, we can use the above snippet code.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@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! |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@grgia I'll approve this in case you want to merge it.
@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]>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@mollfpr I think this is safe to merge as well. Our best bet is going to be regression tests. I am going to merge this to prevent more conflicts.
✋ 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-expensify
tov2.0.31
, to add the use of the new lint rule that checks and insists that any props originating fromwithOnyx
should be optional and a default value should be provided.This PR also modifies the existing components using the
withOnyx
HOC 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
withOnyx
HOC 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 Issues
section aboveTests
sectionOffline steps
sectionQA steps
sectiontoggleReport
and notonIconClick
)src/languages/*
files and using the translation methodWaiting for Copy
label for a copy review on the original GH to get the correct copy.STYLE.md
) were followedAvatar
, I verified the components usingAvatar
are working as expected)/** comment above it */
this
properly so there are no scoping issues (i.e. foronClick={this.submit}
the methodthis.submit
should be bound tothis
in the constructor)this
are necessary to be bound (i.e. avoidthis.submit = this.submit.bind(this);
ifthis.submit
is never passed to a component event handler likeonClick
)StyleUtils.getBackgroundAndBorderStyle(themeColors.componentBG
)Avatar
is modified, I verified thatAvatar
is working as expected in all cases)ScrollView
component to make it scrollable when more elements are added to the page.main
branch was merged into this PR after a review, I tested again and verified the outcome was still expected according to theTest
steps.Screenshots/Videos
N/A