[NoQA][AI Reviewer] Add rules based on you-might-not-need-an-effect #78563
Conversation
Summary of New Performance Rules (PERF-6 through PERF-10)[PERF-6] Derive state from propsSources: Benefits:
[PERF-7] Handle prop changes without useEffectSources: Benefits:
[PERF-8] Handle events in event handlersSources: Benefits:
[PERF-9] Avoid useEffect chainsSources: Benefits:
[PERF-10] Communicate with parent components without useEffectSources: Benefits:
Overall theme: These rules encourage avoiding CC @roryabraham |
|
|
||
| - **Condition**: Flag when useEffect updates state based on props or other state, when the value could be computed directly | ||
|
|
||
| - **Reasoning**: Computing derived values directly in the component body ensures they're always synchronized with props/state and avoids unnecessary re-renders. For expensive calculations, use `useMemo` instead of `useEffect`. Note: React Compiler can automatically memoize expensive calculations, so you may not need `useMemo` if the component is compiled—just compute directly during rendering. |
There was a problem hiding this comment.
suggestion:
| - **Reasoning**: Computing derived values directly in the component body ensures they're always synchronized with props/state and avoids unnecessary re-renders. For expensive calculations, use `useMemo` instead of `useEffect`. Note: React Compiler can automatically memoize expensive calculations, so you may not need `useMemo` if the component is compiled—just compute directly during rendering. | |
| - **Reasoning**: Computing derived values directly in the component body ensures they're always synchronized with props/state and avoids unnecessary re-renders. |
The reason I suggest this is because:
- The AI reviewer won't reliably know if a component is compiled or not
- The AI probably won't reliably know if a computation is expensive or not.
- Memoization should be a separate concern
- In my experience working with AI, it will often latch onto the wrong thing / miss the point. It's not that great at dealing in subtlety, so let's just keep it simple.
There was a problem hiding this comment.
@roryabraham Agree with the simplicity points. Let's adjust that! ✅
FYI, after merging this PR, AI reviewer should now be able to know whether component will be compiled or not.
| const [selection, setSelection] = useState(null); | ||
|
|
||
| // ✅ Good: reset during rendering based on prop | ||
| const [prevItems, setPrevItems] = useState(items); |
There was a problem hiding this comment.
NAB: I'd love an example that doesn't set previous state, because prevItems being in state in-and-of-itself is probably an anti-pattern. Even in the React docs, this example is used as "better", but there's a "best" that follows.
There was a problem hiding this comment.
Good catch, thanks @roryabraham!
I've simplified the PERF-7 rule so it now:
- revolves around controlling component resets via
keyprop - does not duplicate PERF-6 rule responsibility of derived state flow violations
- adjusted examples so they are referencing best approach in clear way
Here's the commit: 8710921
|
@roryabraham all the review comments were addressed 🫡 |
2 participants
Explanation of Change
Fixed Issues
$ #73451
PROPOSAL:
Tests
Offline tests
QA Steps
// TODO: These must be filled out, or the issue title must include "[No QA]."
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