facebook / react

The library for web and native user interfaces.
https://react.dev
MIT License
227.85k stars 46.51k forks source link

Reasons React would be calling render() when memo returns false #14882

Closed natew closed 4 years ago

natew commented 5 years ago

Do you want to request a feature or report a bug?

More of a good place to document some behavior.

What is the current behavior?

It's hard to isolate why this happens because it's something inside React that's making this decision. Perhaps context is changing. But my biggest frustrations lately have been due to renders happening when they shouldn't, or seem like they shouldn't.

For example I can have a component like so:

const Component = React.memo(() => <div />, (a, b) => true)

And I'll find it calling render quite often in a performance sensitive area.

It would be helpful to have docs somewhere that really go into why a component render is being called.

Further, it would be even more helpful to have a sort of "debug stream" you could hook into to see React logging reasons why things are updated in a clean fashion. That this doesn't exist already is a bit of a mystery. It would have saved a ridiculous amount of time for myself over the last years, and I'm sure many others. It would look something like:

(render) ComponentName [props changed]
(render) OtherComponentName [props changed] [state changed]

Even better would be to show some sort of tree structure. And show the results of memo/shouldComponentUpdate, and why they may have been ignored:

(render) ComponentName [memo: true | ignored due to context change in: SomeContext] 
gaearon commented 5 years ago

When you say "context" do you mean legacy context or modern context?

With legacy context we can't give any guarantees at all. You can consider its mere presence in the tree to be an opt into a deoptimized path. It's full of edge cases with over-rendering because its behavior is designed for backwards compat with the very first unstable implementation that libraries started relying on. Fixing it would create churn and definitely breaking changes.

With new context, you shouldn't be seeing unexpected renders. If you are, those are likely bugs. So we'd need a reproducing case to say for sure.

natew commented 5 years ago

Ok well this is good news for my sanity then. I thought I was going insane given it’s a simple component and fully returns true for memo. No legacy context and it wasn’t changing I don’t think. I’ll try and extract an example then.

natew commented 5 years ago

Ok, so following up.

I went and fixed two areas legacy context. Turned on strict mode and most things are good except for a few findDOMNode()'s and one componentWillMount deep in the tree and in an unrelated area.

So I was looking at a performance issue, and ran into this again. But it's not quite the same, and I just want to understand what's going on.

Excuse sloppy graphics:

image

What are reasons VirtualList updates when it's parent (SelectableList) doesn't even call render?

In general, I find it so hard to understand why things are happening in big React apps. You either have profiling which just shows slow components, or you have the tree which often gets huge and updates rapidly so you miss things, or.... nothing.

My ideal would be something where you can:

  1. "Inspect" an element in the tree to
  2. See a log of everything that happens within that tree

If you could easily have a log for every reason for an [Update] or [Render], my life would generally improve. I think I may know how to monkey-patch react-dom and make this happen, but would be curious your thoughts on if this a good thing, or how it could be done so it could go into React proper.

gaearon commented 5 years ago

Have you seen React Profiler? It gives you a commit-by-commit overview.

From your screenshot it looks like:

I can’t really help more at the moment although I agree something like a log would be helpful. (You can manually add one in the bundle but it’s not ideal.)

A reproducing case pls?

bvaughn commented 5 years ago

My ideal would be something where you can:

  1. "Inspect" an element in the tree to
  2. See a log of everything that happens within that tree

This definitely sounds like it could be useful for analyzing certain cases, but it also sounds like it could add a lot of overhead to React.

Disclaimer: I have not thought about how this could be implemented for longer than 5 minutes, but my first naive approach to this would maybe be to have the fiber scheduler report somewhere when work was scheduled for a fiber. We'd probably need to clear that entry out later if we bailed out on the work, or if it was thrown out because of other higher-priority work. We'd also need a way to later correlate that work with a subsequent commit (probably using the expiration time?). Maybe we could store and track it in a similar way as we do with the pending interactions (just without the cascading render behavior)?

Assuming the above would even work– (there might be tons of edge cases I haven't considered)– I wonder how much overhead it would add? Probably only something we'd want to do in DEV bundles, and probably only when ProfileMode was true for a tree (to further limit its impact).

bvaughn commented 5 years ago

+1 for Dan's suggestion to look at this using the DevTools Profiler UI. It won't give you all the "what scheduled this work" answer, but it probably will give you a better visualization of what bailed out and what re-rendered for a given commit.

DJanoskova commented 5 years ago

Hey, my comment might not help you but I hope it does a little. I was just battling the same issue.

I have nested routing in my app and I was trying to get my <Header /> component not to refresh when its only prop wasn't changing, yet it kept re-rendering. My memo() always returned true.

I put a console log at every route/layout wrapper level of my App (I'm a debugging master right 💪) and found out that my wrapping layout component is re-rendering all the time because it has history prop (although I'm not using it at all), simply because the component is a <Route /> in a <Switch /> so the history prop kept changing and my Layout was re-rendered all over - and it contained the poor <Header />.

From my experience, I can say that memo() indeed is working right. However, if your parent components re-render, there's not much your poor memoized child can do about it.

Have you solved this issue? Debugging every level of my app helped a lot, I saw many unnecessary renders in my wrapping layouts when they didn't need to change at all, because I wasn't even going to the different parts of the app that use different wrappers.

gaearon commented 5 years ago

@DJanoskova Btw you might like this tool for debugging: https://reactjs.org/blog/2018/09/10/introducing-the-react-profiler.html. Might be easier than logs!

natew commented 5 years ago

My understanding is that the point of memo is actually specifically for this use case: to prevent children from updating when a parent calls a re-render.

And as to the profiler, while it’s a wonderful tool, I still stand by my point that without having any explanation why, it’s fairly useless. I already know the components are being re-rendered, that’s the whole problem. What I don’t know is why they are re rendering, when it seems they should not be. Though not complaining, just noting that I’ve felt that frustration (and spent quite a bit of time moving up and down the tree logging) attempting to figure it out before.

In some cases it was due to old context usage, in some it was actually just not memoizing a context value and then doing a render (this was a big one for us, I now generally always memo contextual values), and in others it was due to some mobx reactivity which we call unstable_batchedUpdates. Actually it was ultimately a combination of all three, which made it perhaps more confusing. Though if dev tools had some way of saying: this component re rendered due to a contextual value changing, this one re rendered due to a batchedUpdates, and this one due to old context, this due to setState, etc etc, it would have been clear. I understand this is likely complex and full of overhead, just laying out the problem a bit more clearly.

bvaughn commented 5 years ago

And as to the profiler, while it’s a wonderful tool, I still stand by my point that without having any explanation why, it’s fairly useless. I already know the components are being re-rendered, that’s the whole problem.

There are a lot of cases where people don't already know that a given component is being re-rendered.

We do have plans to provide more info about the "why" in the future though. I agree that it's a little opaque at the moment. (It's common feedback.) Apologies!

natew commented 5 years ago

Perhaps worded just a bit too strongly, it’s super helpful! And the update looks very nice. Excited for the future of it. I find dev tooling so valuable. I see some huge ways it could keep improving of course so I’m jealous of future developers and all the helpful tooling I think they’ll have.

bvaughn commented 5 years ago

Hey no worries 😄 I didn't mean to sound defensive or offended. Totally wasn't.

Please feel free to share feedback for ways to further improve it over at https://github.com/bvaughn/react-devtools-experimental

DJanoskova commented 5 years ago

@gaearon The tool looks great. It will take some time until I learn to use it efficiently, but I'm definitely giving it a go.

@natew Sorry, I guess I couldn't form my thoughts into correct sentences lol. English is my second language so sometimes I have troubles expressing the things I want to say correctly.

I want to say that the component I wrapped with memo() didn't only keep re-rendering, it kept re-mounting. So memo() didn't work because it was being mounted again and again, therefore memo() was no help. Maybe your component is acting the same way and that's why memo() doesn't help. If it mounts, it doesn't care about memo(), that's why it refreshes although you always return true as the second argument. Could this be your case?

stale[bot] commented 4 years ago

This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Thank you for your contribution.

Aryk commented 4 years ago

Hey guys, I read through this thread. I'm having the same issues with React. How exactly are we supposed to garantee a component doesn't rerender if the below doesn't even work?

const Component = React.memo(() => <div />, (a, b) => true)

I notice that even with useCallback(() => {}, []) still runs on every time the component re-renders.

Is this all normal? If so what's the point of passing in [] to useCallback or even using memo?

pschiffmann commented 4 years ago

Hey! I just debugged my app when it re-rendered React.memoed components, and found out I just made a stupid mistake. So here's a heads-up if anyone else runs into the same problem.

When I declare a function like this: const Cmp = () => { return <div />; };, then the component will be named "Cmp" (the variable name) in the React DevTools "⚛️ Components" tab. However, when I wrap it in a React.memo call like this: const Cmp = React.memo(() => { return <div />; });, then the component will be named "Anonymous Memo" instead. As a workaround, I started declaring my components like this: const Cmp = React.memo(function Cmp() { return <div />; });, which restored the component name in the DevTools.

However, this will not work for recursive components! In the following component, the whole recursive tree will re-render whenever the root component re-renders:

const TreeItem = React.memo(function TreeItem({nodeId}) {
  const node = useNode(nodeId); // Fetches node from backend
  return (<div>
    {node.name}
    {node.children.map(childId => <TreeItem nodeId={childId} />)}
  </div>);
});

The reason is that inside of the component, the name TreeItem binds to the function name, not to the result of the React.memo call.

theKashey commented 4 years ago

Let me join the club

rlucha commented 4 years ago

@theKashey I'm observing the same problem image

theKashey commented 4 years ago

legacy context is our main suspect here, but it's very hard to eliminate due to different package versions used simultaneously, and which magically could reuse the same context, as long as it's just a string key.

rlucha commented 4 years ago

I'm afraid I have the same problem as we're using react-router 3.2.1 which is using the legacy context too. Well at least I know the culprit now but the update is going to hurt, thanks!

stale[bot] commented 4 years ago

This issue has been automatically marked as stale. If this issue is still affecting you, please leave any comment (for example, "bump"), and we'll keep it open. We are sorry that we haven't been able to prioritize it yet. If you have any new additional information, please include it with your comment!

stale[bot] commented 4 years ago

Closing this issue after a prolonged period of inactivity. If this issue is still present in the latest release, please create a new issue with up-to-date information. Thank you!

apieceofbart commented 3 years ago

I am having a similar problem and I believe this is a bug in react dev tools. I am using a component which has no props but is using useContext inside (new context, not the legacy one). However in react-dev-tools the message is "The parent component rendered." I think this is the exact case described here: https://github.com/facebook/react/issues/19732

TusharShahi commented 3 years ago

I have the same issue as @apieceofbart. I do not have any props and am even returning true. Although it is showing correctly that the component is re rendering, I feel the reason "The parent component rendered." is not correct.