[Lint] Replace PERF-5 ai reviewer rule with no-deep-equal-in-memo eslint equivalent#76364
Conversation
|
|
Codecov Report❌ Looks like you've decreased code coverage for some files. Please write tests to increase, or at least maintain, the existing level of code coverage. See our documentation here for how to interpret this table.
|
|
Update:
@mountiny marking this as ready for review in order to get the feedback from C+ |
|
Hi @brunovjk, this PR introduces new eslint rule which prohibits After runnig linter, there are couple of components found that violates this, however I don't have full confidence whether they are correct and should be left intact or they can be replaced with shallow comparisons. Please take a look at eslint output and share your thoughts on above. Thanks! ❤️ |
|
@kacper-mikolajczak — nice work, I think we shouldn’t blindly replace every deepEqual. I suggest a per-instance triage:
Make sense? Thank you. |
no-deep-equal-in-memo eslint equivalentno-deep-equal-in-memo eslint equivalent
|
@brunovjk I find approach like that feasible for this PR, thanks! I have implemented all the desired changes:
Let me know what you think of introduced changes. |
|
@brunovjk the Apart from that, PR is ready for further review. |
JmillsExpensify
left a comment
There was a problem hiding this comment.
No product review required.
| @@ -1,3 +1,4 @@ | |||
| /* eslint-disable rulesdir/no-deep-equal-in-memo */ | |||
There was a problem hiding this comment.
Why disable the rule at the beginning of the file? Thank you.
There was a problem hiding this comment.
Otherwise, we'd need to disable in multiple lines. Let me know what is the recommended approach here, thanks!
There was a problem hiding this comment.
I think ideally we disable it in the lines individually as then nobody will be forced to observe this rule in this file after that
| deepEqual(prevProps.contextValue, nextProps.contextValue) && | ||
| deepEqual(prevProps.parentReportAction, nextProps.parentReportAction) && | ||
| prevProps.contextValue === nextProps.contextValue && | ||
| prevProps.parentReport === nextProps.parentReport && |
There was a problem hiding this comment.
@kacper-mikolajczak you changed the prop here from parentReportAction to parentReport, sorry, but was that intentional? Thank you.
There was a problem hiding this comment.
That was not intended at all. Thanks for spotting this one out ❤️ Fixed ✅
Reviewer Checklist
Screenshots/VideosAndroid: HybridAppUploading 76364_android_native.mov… Android: mWeb ChromeUploading 76364_android_web.mov… iOS: HybridApp76364_ios_native.moviOS: mWeb SafariUploading 76364_ios_web.mov… MacOS: Chrome / Safari76364_web_chrome.mov |
|
@kacper-mikolajczak Won't we need QA here? To ensure that none of these latest changes introduce regressions. Thank you. |
|
+1 lets try to write test steps for these cases |
package.json
Outdated
| "eslint": "^9.36.0", | ||
| "eslint-config-airbnb-typescript": "^18.0.0", | ||
| "eslint-config-expensify": "2.0.100", | ||
| "eslint-config-expensify": "github:Expensify/eslint-config-expensify#0432ea0ed4da293811502d6aabc60190d24ad1ad", |
There was a problem hiding this comment.
Why'd you change this to a GitHub ref? Was there some problem with the npm-published version?
There was a problem hiding this comment.
Thanks Rory! ❤️ Resolving it in a moment
There was a problem hiding this comment.
That was for testing of the total lint time on the PR 🙌
|
Can you also please check the failing gh action please? |
Sure, checking this after merging latest main and bumping up linter. |
package.json
Outdated
| "eslint": "^9.36.0", | ||
| "eslint-config-airbnb-typescript": "^18.0.0", | ||
| "eslint-config-expensify": "2.0.100", | ||
| "eslint-config-expensify": "^2.0.101", |
There was a problem hiding this comment.
Whoops, please don't use the ^:
| "eslint-config-expensify": "^2.0.101", | |
| "eslint-config-expensify": "2.0.101", |
We don't use proper semver in most of our dependencies, meaning that we almost never want ^ because it makes it so that we might unintentionally upgrade the dependency to anything less than 3.0. I don't understand why this is such a popular practice, as it also just makes it much less clear what package version is actually installed
There was a problem hiding this comment.
Agree on that one! I think it goes back to how npm i by default allows minor/patch bumps and it often slips past when doing quick changes. Thanks for spotting this ❤️
@roryabraham To avoid this in future, here's a quick fix for linter README: Expensify/eslint-config-expensify#173
Co-authored-by: Rory Abraham <47436092+roryabraham@users.noreply.github.com>
Coming from: Expensify/App#76364 (comment) We don't want devs to have different eslint configs installed by accident.
|
@mountiny we've got this approved and ready - seems like failing ESLint check is caused by out of scope issue. Let me know if we can merge this 🚀 |
mountiny
left a comment
There was a problem hiding this comment.
Thanks, the failing eslint changed is unrelated code to these changes to limit the scope we will skip that now
| @@ -1,3 +1,4 @@ | |||
| /* eslint-disable rulesdir/no-deep-equal-in-memo */ | |||
There was a problem hiding this comment.
I think ideally we disable it in the lines individually as then nobody will be forced to observe this rule in this file after that
|
✋ 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/mountiny in version: 9.2.79-0 🚀
|
|
@kacper-mikolajczak Is there a new way to add participants while creating expenses, step 6 from test 1 and step 2 from test 6 are a bit confusing. Could you please explain that for me, thanks. |
|
🚀 Deployed to staging by https://github.com/mountiny in version: 9.2.81-0 🚀
|
|
@mountiny @kacper-mikolajczak @brunovjk
Is there a new way to add participants while creating expenses, step 6 from test 1 and step 2 from test 6 are a bit confusing. |
|
Thanks for spotting this, it should be adjusted the same way incorrect steps reported before by Bruno. Removed unclear steps ✅ Sorry for the confusion! |
|
🚀 Deployed to production by https://github.com/AndrewGable in version: 9.2.81-5 🚀
|
|
🚀 Deployed to production by https://github.com/AndrewGable in version: 9.2.81-5 🚀
|
Explanation of Change
Fixed Issues
$ #76362
PROPOSAL:
Tests
Offline tests
QA Steps
1. Money Request Flow (affected:
MoneyRequestConfirmationList,MoneyRequestConfirmationListFooter)Steps:
2. Report Screen (affected:
ReportScreen,ReportFooter)Steps:
3. Report Action Items (affected:
PureReportActionItem,ReportActionItemContentCreated)Steps:
4. Context Menu (affected:
BaseReportActionContextMenu)Steps:
5. Popover Menu (affected:
PopoverMenu)Steps:
6. Option Row / Participant Selection (affected:
OptionRow,MoneyRequestParticipantsSelector,MoneyRequestAttendeeSelector)Attendee selection:
PR Author Checklist
### Fixed Issuessection aboveTestssectionOffline stepssectionQA stepssectioncanBeMissingparam foruseOnyxtoggleReportand notonIconClick)src/languages/*files and using the translation methodSTYLE.md) were followedAvatar, I verified the components usingAvatarare working as expected)StyleUtils.getBackgroundAndBorderStyle(theme.componentBG))npm run compress-svg)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