im working on a relatively large React codebase and I've seen that the previous developers used memo and usecallback liberally with the thought that by using these would improve performance. Obviously not, here is an example
export default function QuestionHelper({ text }: { text: string }) {
const [show, setShow] = useState<boolean>(false);
const open = useCallback(() => setShow(true), [setShow]);
const close = useCallback(() => setShow(false), [setShow]);
return (
<span style={{ marginLeft: 4 }}>
<Tooltip show={show} text={text}>
<QuestionWrapper
onClick={open}
onMouseEnter={open}
onMouseLeave={close}
>
<Question size={16} />
</QuestionWrapper>
</Tooltip>
</span>
);
}
The app does work right now. But i'm getting push back of not fixing it because the app works as intended. How can I make a case that we should remove all unecessary memo and usecallback in our code?
Is there an objective way of telling the benefits of removing these?
So removing the one in your example actually COULD potentially change things. If you memoize it with useCallback, then the open function persists between renders, which means the function holds the same place in memory each render cycle. If you removed it and just had
const open = () => setShow(true);
Then every time the component renders, this function is re-created, and thus has a new place in memory.
What's the implication of that? Well, let's say hypothetically your QuestionWrapper component export is wrapped with React.memo, since unnecessary re-renders can cause performance issues for this particular component. Well when comparing functions with === how does Javascript compare objects (functions are technically objects in JS)? It does a "reference comparison", i.e. it compares the address in memory of the 2 inputs to see if it's the same object. In the current code, while memoized with useCallback, the reference won't change between re-renders. But if you remove that, then it's reference WILL change every re-render, and React.memo would no longer work for QuestionWrapper since it would think it's getting a new object every time, even though the actual function and its logic doesn't ever change.
Now, maybe in your app that wouldn't matter, maybe QuestionWrapper can re-render lots of times without performance issues, or maybe that component isn't memoized anyway so it doesn't matter, but the point is, in general: don't try to refactor/optimize things that aren't causing issues, unless there would be big implications down the line for not doing.
In an ideal world, you would assess this case and yeah, maybe removing some of the memoization would technically be the best choice. However in the real world, you inherit code bases that are full of crap that isn't ideal, but you just work with it for several reasons, such as:
and various other reasons. I understand that sub-optimal stuff can be a bit annoying and we'd like our code to be super sleek, well-structured and optimized, but unfortunately we have to deal with the reality that in large, real-world applications code is often full of stuff like this. Some of it may just be laziness or ignorance on other developers' part, or some of it may actually be that way for a good reason that might not be immediately evident.
You're always free to voice concern, but unless you're quite experienced and fully understand the implications of such refactoring, and have literally nothing better to do, then if everyone is saying to just leave it as is, it's best to do so. There's almost certainly better uses of your time, and if you do something like accidentally break an application or introduce bugs just so you could do tiny optimization or fix things that weren't even causing any issues, you'll make yourself quite unpopular.
Bsides, in my experience with React, bugs re. memoization can be a real pain in the ass to track down due to the bizarre behavior they can cause.