Skip to content
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

Q: When should you NOT use React memo? #14463

Closed
rlesniak opened this issue Dec 19, 2018 · 40 comments
Closed

Q: When should you NOT use React memo? #14463

rlesniak opened this issue Dec 19, 2018 · 40 comments

Comments

@rlesniak
Copy link

I've been playing around with React 16.6.0 recently and I love the idea of React Memo, but I've been unable to find anything regarding scenarios best suited to implement it. The React docs (https://reactjs.org/docs/react-api.html#reactmemo) don't seem to suggest any implications from just throwing it on all of your functional components. Because it does a shallow comparison to figure out if it needs to re-render, is there ever going to be a situation that negatively impacts performance?

And second question: as long as everything remains pure, is there ever a situation to not wrap a functional component with React Memo?

Thank you.

@markerikson
Copy link
Contributor

I would assume that the same general advice applies for React.memo as it does for shouldComponentUpdate and PureComponent: doing comparisons does have a small cost, and there's scenarios where a component would never memoize properly (especially if it makes use of props.children). So, don't just automatically wrap everything everywhere. See how your app behaves in production mode, use React's profiling builds and the DevTools profiler to see where bottlenecks are, and strategically use these tools to optimize parts of the component tree that will actually benefit from these optimizations.

@G-Rath
Copy link

G-Rath commented Dec 20, 2018

I would also imagine it'd apply to the useMemo hook.

The main thing I'm interested in relating to this question is about the cost of class initialisation (and similar) - It's something that I see just dropped in in couple of places of the React docs by Facebook, without any evidence or references. I'm not challenging Facebook, but I'd love to read more about it, so I can know what they clearly do, and thus optimize my builds.

They do the same with hooks, where they state:

For example, classes don’t minify very well, and they make hot reloading flaky and unreliable.

Which is the first I've heard of classes messing with hot reloading - in my experience I've not actually ever had a working hot reloading system that actually does swap out small chunks of code. That could be b/c I use classes everywhere, but I've never in my readings come across anything saying 'classes are screwing up my hot reloading' or 'hey classes cost you ~5kb more than functional components'.

I think a big factor in the answer on when to use memo relates to how/when React actually does component mounting/initialization. I know that information is on hand in the React docs (and other places), but it's not really mentioned "around" memo talks, making it really easy to forget.

From what I remember, it's something like " does not create a whole new element everytime - thats why mount methods & constructors don't get called every render. Instead, React does some magic to somehow re-use or compare the instance if it exists". This could be completely wrong or muddled, but that's also kind of my point.

I'm not trying to derail this issue at all (And I do apologize for my ramblings in this comment), but I do feel it'd be nice to have some clear cut examples and discussion about where it's costing you (and saving you) to memo. It'd especially be cool to have some extreme examples, with really slow components that are "magically" speed up with memo 😁

@markerikson
Copy link
Contributor

markerikson commented Dec 20, 2018

useMemo has a different use case than React.memo / shouldComponentUpdate / PureComponent. It's not about preventing an entire component from re-rendering, it's about simply memoizing some data. Very unrelated in usage. The only similarity is that React.memo() and useMemo() both involve "if you see the same inputs as last time, don't do any extra work - return what you had before", but React.memo is for wrapping up entire components, and useMemo() is for whatever you want to return from the callback.

I'm not sure what "class initialization" has to do with React.memo. If I wrap a class component in React.memo and render <MemoizedClassComponent />, the class instance gets created either way. The difference is that there's an invisible extra layer in the component tree, and it won't re-render as often depending on the incoming props.

Classes definitely make hot reloading more difficult, because of trying to swap out prototypes and stuff. I'm not clear on all the nitty-gritty details, but look through the https://github.com/gaearon/react-hot-loader repo to see all the complexities involved.

Overall, the React team has frequently emphasized on social media that "wrapping everything in a PureComponent is a bad idea", but I agree it'd be nice to actually have that in the docs.

As far as I know, everything in https://reactjs.org/docs/optimizing-performance.html that relates to shouldComponentUpdate and PureComponent applies to React.memo as well.

You might also want to read some of the articles on optimizing React performance that I have listed at https://github.com/markerikson/react-redux-links/blob/master/react-performance.md .

@G-Rath
Copy link

G-Rath commented Dec 20, 2018

useMemo has a different use case than React.memo / shouldComponentUpdate / PureComponent. It's not about preventing an entire component from re-rendering, it's about simply memoizing some data. Very unrelated in usage. The only similarity is that React.memo() and useMemo() both involve "if you see the same inputs as last time, don't do any extra work - return what you had before", but React.memo is for wrapping up entire components, and useMemo() is for whatever you want to return from the callback.

You're completely right - that was me being stupid. I've been working too hard lately, and it's left my brain in a muddle 😬

I'm not sure what "class initialization" has to do with React.memo

(I actually technically mean instancing - sorry about the confusion)

In the hooks FAQ, they have the question Are Hooks slow because of creating functions in render?
, which they state in the answer:

Hooks avoid a lot of the overhead that classes require, like the cost of creating class instances and binding event handlers in the constructor.

For Facebook to say "the cost of creating class instances" implies to me there is a cost worth mentioning (i.e, compared to the cost of doing something like const array = [], which doestechnicallyy have a cost, but it's so insanely small no one ever talks about it), but in all my readings about props & cons of classes in JS, I've never come across anyone arguing that classes have a 'cost' worth mentioning (which I figured people would jump on, as ES6 classes were a hot topic of debate back when they were introduced).

I'm assuming that the cost is actually one React suffers with classes due to how it works, where the cost is only a factor when you're creating lots of class instances in the space of seconds all the time, but again, this isn't something I recall seeing explored or talked about in detail in the rest of the React documents.

Thanks for the links - I've not encountered them before, despite my best efforts 😕 You've given me some good reading to do!

@markerikson
Copy link
Contributor

markerikson commented Dec 20, 2018

In React 15 and earlier, function components were wrapped in classes internally, so there was no real difference in perf behavior.

In React 16, function components are now handled separately, with no class wrapping them. This means there's less overhead to rendering a function component overall than for a class component. References:

Dominic Gannaway:

Functional components should be slightly faster in React 16 as there's no instance created to wrap them (unlike React 15).

Andrew Clark:

Think of all the features class components have that we can totally skip with functional components. Constructors, refs, lifecycles, error handling. The only reason they were the same performance-wise before Fiber was technical debt in the the Stack implementation.

So, in general, an app that consistently renders N function components should take less total time to render than an app that renders N class components. How much less, I have no idea - you'd have to benchmark things.

However, all of that is mostly separate from the original question of "when should I use React.memo()?"

I understood what you meant by "class initialization" - the cost of creating a single instance of a given class component, because you're rendering it for the first time. My point is that that has nothing to do with use of React.memo, because rendering <PlainClassComponent> for the first time has basically the same cost as rendering <MemoWrappedClassComponent> for the first time. In both cases, a class component instance is created.

Also, that hooks FAQ entry is largely there because of the longstanding fear and confusion in the React community over "creating functions in render being bad for performance". See Ryan Florence's post React, Inline Functions, and Performance for thoughts on that topic.

@TidyIQ
Copy link

TidyIQ commented Apr 19, 2019

Can someone please explain (like I'm 5) WHY it's a bad idea to wrap all components in React.memo? If the props on a component haven't changed then it shouldn't re-render as it will simply be returning the exact same component as the initial render.

I also don't understand why the react docs say Do not rely on it to “prevent” a render, as this can lead to bugs. What bugs? Why else would you use React.memo if not prevent an unnecessary re-render?

For example, on my main index page I include a <Header> component that takes in title and metaDescription as props. These props never change for the page so the Header component shouldn't re-render whenever something else is changed on the index page (e.g. someone clicks a button that changes state). It seems the only way to do this is by wrapping the Header component in React.memo, but apparently the docs imply this is the incorrect use.

lucasecdb added a commit to cramkle/cramkle that referenced this issue Apr 19, 2019
Components that receive other components through props shouldn't
be pure (and by extend, use the memo HOC).

See this for more info: facebook/react#14463
@StoneCypher
Copy link

@markerikson -

So, in general, an app that consistently renders N function components should take less total time to render than an app that renders N class components. How much less, I have no idea - you'd have to benchmark things.

Dramatically, it seems.

@lukewlms
Copy link

Here's the best info I've found on this. Seems like there are few cases where you wouldn't want to use React.memo, theoretically. Though the significant extra typing in every component seems clunky.

Perhaps your parent component rerenders regularly and your child component does something impure like render the current time or fetch some data... though I'd think hooks would cover both of those well.

@panzerdp
Copy link

Regarding when to use React.memo(), I wrote an interesting blog post. Check it out.

I use the following rule of thumb:

The more often the pure functional component re-renders with the same props, the heavier and the more computationally expensive the output is, the more chances are that component needs to be wrapped in React.memo()

@mikeaustin
Copy link

I always use PureComponent for class based components. It's easy, and it just works. I've noticed that if I don't wrap basically all my components in memo(), nothing gets optimized. And since children are part of props, I had a rouge non-memoized component that cause everything to re-render().

I'm trying to like hooks, and some of them are useful, but the mental overhead is actually large. Ract.memo(), useCallback(), useState(), useRef(), useMemo(), useContext(), all used together in a < 100 line fine gets a bit hard to read. An example I've created to optimize dragging, which has a hooks version and a class version:

https://github.com/mikeaustin/optimized-drag/tree/master/src

@fbartho
Copy link

fbartho commented Aug 5, 2019

It’d be great if there was a helper to let you diff everything except for children, and have React handle diffing Children.

@ardok
Copy link

ardok commented Aug 10, 2019

If you really, really want it... You could technically put the children into useMemo...

import React, { useMemo } from 'react';

const Foo = React.memo(({ children }) => {
  return children;
});

const Hello = () => {
  const childrenTest = useMemo(() => {
    return (
      ...children here...
    );
  }, [...to watch for changes, call it AAA...]);

  return (
      <Foo {...AAA}>{childrenTest}</Foo>
  );
}

It's just going to pollute that Foo component with a bunch of props that it may not need (since you also need to pass in all the children props into Foo).

@syntaxbliss
Copy link

Let us say that we have a case like this one:

const MyComponent = React.memo(({genre, name}) => {
    const someFunction = genre => {
        return (genre === 'male') ? 'Mr.' : 'Ms.';
    }

    return (
        <h1>Hello {someFunction(genre)} {name}!</h1>
    );
});

Does it worth to create a memoized functional component when it has an additional function? Does that additional function also get memoized, or in that case I would be slowing down things by creating a new function on every render? In this example, would be better to move on to a class-based component?

@Bohmaster
Copy link

Let us say that we have a case like this one:

const MyComponent = React.memo(({genre, name}) => {
    const someFunction = genre => {
        return (genre === 'male') ? 'Mr.' : 'Ms.';
    }

    return (
        <h1>Hello {someFunction(genre)} {name}!</h1>
    );
});

Does it worth to create a memoized functional component when it has an additional function? Does that additional function also get memoized, or in that case I would be slowing down things by creating a new function on every render? In this example, would be better to move on to a class-based component?

I think you need to use 'useCallback' to memoize the extra function, pajarito.

@ajaysaini-sgvu
Copy link

ajaysaini-sgvu commented Apr 20, 2020

@Bohmaster I think he is not using someFunction as a callback to the child component. useMemo should work here.

What do you think ?

@ghost
Copy link

ghost commented May 19, 2020

@syntaxbliss React.memo will skip re-rendering the component for same props and instead take the previous memoized ReactElement "snapshot". It's like React does not call the MyComponent function again for re-render, but take its last result.

Perhaps a bit more clear statement is: the resulting ReactElement/JSX element, to which your inner someFunction function contributed something, is memoized.

I would not care about function creation cost in render for someFunction. Also you pass this inline function only to a primitive DOM node, so there is no need to ask yourself, if memoization gets broken due to creating a new function reference each render.

Maybe this answer also helps to decide, if React.memo is worth it for your case.

@marcelkalveram
Copy link

Can someone please explain (like I'm 5) WHY it's a bad idea to wrap all components in React.memo? If the props on a component haven't changed then it shouldn't re-render as it will simply be returning the exact same component as the initial render.

I'm just guessing based on what I know, but since not everyone using React embraces immutability, there might be cases where someone passes data into a non-memoised ChildComponent, then mutates that data (an object or array) and expects that component to re-render, ie. because its parent did.

As soon as you memoise that ChildComponent using React.memo, the mutated data won't trigger a re-render because of referential equality, even though its parent did re-render.

The other potential downside of React.memo is the work that goes into shallow comparison, even though for most apps that's probably negligable.

@MaxmaxmaximusAWS
Copy link

MaxmaxmaximusAWS commented Aug 14, 2020

@StoneCypher @fbartho @mikeaustin @marcelkalveram @gaearon @rlesniak @sunny @panzerdp

You should always use React.memo LITERALLY, as comparing the tree returned by the Component is always more expensive than comparing a pair of props properties)

So don't listen to anyone and wrap ALL functional components in React.memo. React.memo was originally intended to be built into the core of functional components, but it is not used by default due to the loss of backward compatibility. (Since it compares the object superficially, and you MAYBE in the component, use the nested properties of the sub-object) =)

That's it, this is the ONLY REASON why React doesn't use memo Automatically. =)

In fact, they could make version 17.0.0, which would BREAK backward compatibility, and make React.memo the default, and make some kind of function to CANCEL this behavior, for example React.deepProps =)

Stop listening to theorists, guys =) The rule is simple:

If your component USES DEEP COMPARING PROPS then don't use memo, otherwise ALWAYS USE, comparing TWO OBJECTS is ALWAYS CHEAPER than calling React.createElement () and comparing two trees, creating FiberNodes, and so on.

Theorists talk about what they themselves do not know, they have not analyzed the react code, not understand FRP, and do not understand what they advise =)

Always yours, one of the react developers (Now working at AWS =) ahah)

Everyone please ^ __ ^

image

@MaxmaxmaximusAWS
Copy link

MaxmaxmaximusAWS commented Aug 14, 2020

@StoneCypher and @mikeaustin

Prove your arguments, or the whole community will see that you, you could not win the PUBLIC COURT, and cowardly put dislikes. WHAT EXACTLY arguments COULD you counter my arguments?

WE as the whole React community listen to your counterarguments =))

Let's laugh right now =) we listen to your arguments

image

@StoneCypher
Copy link

Please stop tagging me

@MaxmaxmaximusAWS
Copy link

MaxmaxmaximusAWS commented Aug 14, 2020

@StoneCypher @fbartho @mikeaustin @marcelkalveram @gaearon @rlesniak @sunny @panzerdp

But I want to teach you the truth)! I don't want you to live in misconceptions about React.memo, why don't I hear gratitude, but get dislikes? I sat, wrote, explained, tried, did you all GOOD. And he did not A SINGLE EVIL. And you are NOT GRATEFUL to me FOR GOOD, but you are doing EVIL: put dislikes. Why are you FOR GOOD - answer with EVIL? are you just -- EVIL????

image

@sunny
Copy link
Contributor

sunny commented Aug 16, 2020

Please stop tagging me too, I was never in the conversation in the first place.

@MaxmaxmaximusAWS
Copy link

@sunny strange, apparently this is a GitHub bug

@rickhanlonii
Copy link
Member

@MaxmaxmaximusAWS please stop tagging people in this thread.

@MaxmaxmaximusAWS

This comment has been minimized.

@slavafomin
Copy link

To be honest, I would actually like to hear another opinion on what Mr. Maxmaxmaximus wrote in his first message (all trolling and jokes aside). It actually makes sense that comparing couple of props shallowly should consume less resources than executing the render function with all the hooks in it and building the JSX for the component (and all the sub-components). Strong/strict comparison (===) is a very simple operation.

Also, could you explain to me, will React always compare entire virtual DOM trees or only the parts that actually changed? So if I exclude the part of the virtual DOM tree using React.memo will it actually be excluded from reconciliation process? Or is it just skips the JSX generation? I guess the second, but I would like to hear a confirmation from someone more knowledgeable.

Thank you!

@MaxmaxmaximusAWS
Copy link

MaxmaxmaximusAWS commented Oct 9, 2020

@sunny

I guess the second, but I would like to hear a confirmation from someone more knowledgeable.

  1. React call component function
  2. React looks what elementts and components returned from function
  3. React create FiberNodes for all returned elements and components
  4. React call fiberNode.execute() for all created fiberNodes
  5. What returned from fiberNode.execute() function, react will "substitute" this for the fiber node
  6. FiberNode.execute() has optimisation. if you use React.memo() then fiberNode.execute() returns previous result (fibers nodes thee) and not call fiberNode.execute() of them.
  7. React.memo memorized ALL child breanch of tree, you do not need to memorize each child individually

No trolling as you requested =)

@sunny
Copy link
Contributor

sunny commented Oct 9, 2020

Again, please stop mentioning me…

@sunny
Copy link
Contributor

sunny commented Oct 9, 2020

Considering that several people have asked you to stop tagging them and that your comments have been tagged as disruptive, I'd wager you are the one with inappropriate, irrational behavior.

If you receive notifications, turn them off in the settings

I've blocked you. This is the first time in 12 years of GitHub that've had to do that… Please listen to what people are telling you and act more professionally.

@vzaidman
Copy link
Contributor

vzaidman commented Oct 14, 2020

I want to point out that from my experience, people often fall into one of the following three categories:

  1. people who don't have any idea of what memo is and never use it
  2. people who kinda know it helps with performance and uses memo almost everywhere
  3. people who try adding it in places that are expected to benefit from it

All of them usually forget to memorize some super important component here and there (the website's Header is a classic example) or memoize a component and pass it an always invalidated prop like style={{width: '100%'}} so it renders again and again.

Sometimes these places render many components, have logic and side effects. Sometimes they are rendered very often.

I think the biggest benefit of making everything pure is "keeping it simple" by removing this feature altogether, even though, in some cases, it would make the application a little slower.

@MaxmaxmaximusAWS
Copy link

MaxmaxmaximusAWS commented Oct 17, 2020

@vzaidman React.memo actually have second argument for deep comparatiom. for this reason we have to use React.memo everywhere, even for style={{width: '100%'}}

function MyComponent(props) {
  /* ender using props */
}

function areEqual(prevProps, nextProps) {
  /*
    returns true if nextProps renders
    the same result as prevProps,
    otherwise returns false
  */

  return prevProps.style.width === nextProps.style.width
  // or like this:
  return JSON.stringify(prevProps.style) === JSON.stringify(nextProps.style)
}

export default React.memo(MyComponent, areEqual);

@vzaidman
Copy link
Contributor

vzaidman commented Oct 18, 2020

This would be an overkill in 99% of cases and also hard to make sure you will not get some kind of onClick={() => and then the whole nice style comparison is useless

@MaxmaxmaximusAWS
Copy link

MaxmaxmaximusAWS commented Oct 18, 2020

People should use useCallback for callbacks, to become memo works.
Apollo and React native hooks, actually use useCallback. This is a problem for bad developers, and it should not drag us back, but it should push them forward. they should think "hmmm, but memo won't work for child components, if there is a memo, you shouldn't be lazy and wrap my callback in useCallback."

const [state, setState] = useState()

setState // it is actually memorized callback

if we use useEffect and setState inside, useEffect not triggeret every component render

const [state, setState] = useState()

useEffect(() => {
  setState(true)
}, [setState]) // this dependency never will not changed

in theory, our entire application should be wrapped in React.memo with correct props comparison settings, and use ʻuseCallback` for callbacks.

then the reactivity atom will be optimized as much as possible, and the react will render ONLY what could really change. our application is speeding up by using a lot of memory.

You can even write an extension in babel that automatically memorizes all callbacks, but I'm not a fan of "magic".

@subramen
Copy link

Due to violations of our code of conduct, we are putting a temporary block on the user @MaxmaxmaximusAWS for two weeks.

@KurtGokhan
Copy link

KurtGokhan commented Apr 29, 2021

Shouldn't memo be the default with Function Components, or provide an option to make it the default? Has this ever came up as a plan? IMO, writing memo everywhere makes the code verbose and does not behave very well with IDEs and Typescript. I have the same issue with forwardRef but at least that is not everywhere.

Well, I guess pipe operator |> will make it a bit more bearable.

@markerikson
Copy link
Contributor

markerikson commented Apr 30, 2021

@KurtGokhan the question has been asked repeatedly, in many places :) and my answer at the very top of this thread still applies. There are a number of cases where components will never memoize because the inputs are always changing, so applying it as a default blindly is not the right approach.

Also see my additional thoughts here, with some quotes from Dan:

https://blog.isquaredsoftware.com/2020/05/blogged-answers-a-mostly-complete-guide-to-react-rendering-behavior/#memoize-everything

@KurtGokhan
Copy link

Sorry for resurrecting this thread @markerikson. Just wanted to confirm if this is ever planned in the future. To me it seems like not using memo is more prone to performance issues. Especially when you realize that a state change in your root component will cause entire tree to re-render if you don't memo at all. But maybe that just sounds scary and it is actually not that bad.

When asked this question, most people emphasize that it is prone to bugs caused by mutable state (which is arguably not the intended way to use React). I now understand that there can also be performance hits by blindly using memo.

@pstachula-dev
Copy link

pstachula-dev commented Oct 19, 2021

Hello @markerikson
Can you check out my benchmark?
Because in every aspect it seems that React.memo is better. Especially when we are sure of stable argument references.

Even when you add children to the component with memo, the worst case performance will be the same

https://codesandbox.io/s/react-memo-benchmark-m2bk6?file=/src/App.js

yen-tt added a commit to yext/search-ui-react that referenced this issue Apr 14, 2022
…ches (#110)

SearchBar requires multiple re-renders due to useEffect call in useRecentSearches to setup RecentSearches instance. The changes in this pr ensures a RecentSearches is set on the first render and only re-render if there is an update to `recentSearchesLimit`. 

Findings when attempt to memoize hooks and testing in SearchBar:

Just using `useCallback` and `useMemo` doesn't really avoid unnecessary re-renders in our components. By default, the child components will always re-render if the parent component has triggered a render ([1](https://stackoverflow.com/a/40820657)). But, this can be override with shouldComponentUpdate for class-based component or with `React.memo(SomeComponent)` for functional component. `React.memo` prevents unnecessary re-renders if there's no change in the props (and no state update in the component itself), and we can use useMemo/useCallback to avoid creating new object when passing props to component each time.

I attempted to use React.memo on DropdownInput for SearchBar (had to wrap a lot of things in useCallback), but there's a lot of props and state changes from SearchBar interactions that almost certainly would require a re-render of the subcomponents. Clicking in/out of searchbar, typing/submitting/clearing inputs in some way generally trigger some setState functions due to new results and/or make updates to the different contexts that would affect the Dropdown components. Additionally, some advise that components like Dropdown with [child prop shouldn't use React.memo ](facebook/react#14463 (comment) the children prop almost always changes.

I believe there may **not** be a lot of benefits for doing useCallback throughout SearchBar to avoid re-renders in its subcomponents. It kind of make the codebase more complicated with the overhead and less readable, unless there's an actual performance bottleneck to address. So I'm a bit hesitant to add more useMemo/useCallback in our hooks and components if we don't need to.

J=SLAP-1965
TEST=manual

log number of renders in SearchBar before and after useRecentSearch changes. See that SearchBar have less re-renders on initial load.
yen-tt added a commit to yext/search-ui-react that referenced this issue Sep 28, 2022
…ches (#110)

SearchBar requires multiple re-renders due to useEffect call in useRecentSearches to setup RecentSearches instance. The changes in this pr ensures a RecentSearches is set on the first render and only re-render if there is an update to `recentSearchesLimit`. 

Findings when attempt to memoize hooks and testing in SearchBar:

Just using `useCallback` and `useMemo` doesn't really avoid unnecessary re-renders in our components. By default, the child components will always re-render if the parent component has triggered a render ([1](https://stackoverflow.com/a/40820657)). But, this can be override with shouldComponentUpdate for class-based component or with `React.memo(SomeComponent)` for functional component. `React.memo` prevents unnecessary re-renders if there's no change in the props (and no state update in the component itself), and we can use useMemo/useCallback to avoid creating new object when passing props to component each time.

I attempted to use React.memo on DropdownInput for SearchBar (had to wrap a lot of things in useCallback), but there's a lot of props and state changes from SearchBar interactions that almost certainly would require a re-render of the subcomponents. Clicking in/out of searchbar, typing/submitting/clearing inputs in some way generally trigger some setState functions due to new results and/or make updates to the different contexts that would affect the Dropdown components. Additionally, some advise that components like Dropdown with [child prop shouldn't use React.memo ](facebook/react#14463 (comment) the children prop almost always changes.

I believe there may **not** be a lot of benefits for doing useCallback throughout SearchBar to avoid re-renders in its subcomponents. It kind of make the codebase more complicated with the overhead and less readable, unless there's an actual performance bottleneck to address. So I'm a bit hesitant to add more useMemo/useCallback in our hooks and components if we don't need to.

J=SLAP-1965
TEST=manual

log number of renders in SearchBar before and after useRecentSearch changes. See that SearchBar have less re-renders on initial load.
@pauldraper
Copy link

This answered in React docs:

Optimizing with memo is only valuable when your component re-renders often with the same exact props, and its re-rendering logic is expensive...

There is no benefit to wrapping a component in memo in other cases. There is no significant harm to doing that either, so some teams choose to not think about individual cases, and memoize as much as possible.

  1. React.memo will sometimes help performance
  2. React.memo will sometimes do nothing for performance.
  3. In either case, React.memo is essentially free. (Very quick to check props equality.)

The biggest reason IMO React.memo is not enabled by default is that it can break rendering when props are mutable. However, it is best practice to not use mutable props in the first place.


React.memo is ineffective if the props change each render, e.g.

  1. Callers pass event handlers that aren't created with useCallback.
  2. Callers pass other non-primitives (objects, arrays) that aren't created with useMemo.
  3. Callers pass children that aren't created with useMemo.

That said, the cost is essentially nothing and it is up the caller to change passed props. IMO the callee should always use React.memo, and if the caller wants to take advantage of that, it can.

@facebook facebook deleted a comment from shaywiin Feb 5, 2024
@gsathya
Copy link
Contributor

gsathya commented Feb 5, 2024

I've removed the previous comment as it violates our code of conduct. As this thread has seen extensive on-topic discussion and is now becoming outdated, I'm going to lock it.

@facebook facebook locked as resolved and limited conversation to collaborators Feb 5, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

No branches or pull requests