Skip to content
This repository has been archived by the owner on Jun 26, 2020. It is now read-only.

Does "Trace React Updates" respect shouldComponentUpdate short-circuits? #337

Closed
stipsan opened this issue Feb 29, 2016 · 14 comments · Fixed by #804
Closed

Does "Trace React Updates" respect shouldComponentUpdate short-circuits? #337

stipsan opened this issue Feb 29, 2016 · 14 comments · Fixed by #804
Labels
Milestone

Comments

@stipsan
Copy link
Contributor

stipsan commented Feb 29, 2016

It looks to me as if shouldComponentUpdate checks counts as an update in the devtools.
Even if a component return false on sCU it will flash as updated. Child components of a falsy sCU won't flash, suggesting that sCU is probably not tracing renders per-se but the componentWillReceiveProps part of the lifecycle.

Here's a codepen to see what I'm talking about: http://codepen.io/stipsan/pen/grbaEr

With "Trace React Updates" flipped on I'd expect the first input to flash itself and its parent when you change the value. The second input guard itself with a sCU for its local state.
But it flashes, suggesting the sCU returned true when it actually returned false.
If the second input is changed, it'll only update its local state and the devtools will show a trace for just the second input, just as you'd expect.

If this is the intended behavior, then it should be much more clear so that lesser experienced devs isn't mislead into thinking pure components are rendering when they aren't.

@jaredly
Copy link
Contributor

jaredly commented Mar 1, 2016

you're right -- it's confusing.
This has to do with the way that the devtools instruments react to get information on prop/state updates, etc.
The hook we decorate is performUpdateIfNecessary, and we don't have a clean way of knowing whether shouldComponentUpdate returned false...

If you want to add a section to the Readme about this (maybe under FAQ) that would be great!

To actually fix this problem, we'd probably need to make a change to react core, perhaps making performUpdateIfNecessary return a bool indicating whether an update was necessary

@stipsan
Copy link
Contributor Author

stipsan commented Mar 2, 2016

Sure, I'll add a section to the FAQ about this.
Thanks for the explanation!

@wmertens
Copy link

wmertens commented Mar 8, 2016

I think it would be very nice if this was an accurate indication of updates, since it's such a visual way of verifying performance.

Will React v15 make this easier/harder?

@jaredly
Copy link
Contributor

jaredly commented Mar 8, 2016

don't think v15 will change this -- but as I said above, I don't believe it would be too much of a change to support this

@wmertens
Copy link

wmertens commented Mar 8, 2016

Well, it goes pretty deep but indeed it looks like it might be doable. What is the best way to get this in v15?

@jaredly
Copy link
Contributor

jaredly commented Mar 9, 2016

Given that v15 has an rc out, I don't imagine this would make it in as it's not a bugfix, but it won't be a breaking change so I'm sure it could get into 15.0.1 or 15.1 (depending on how they want to version it). If you want to take a whack at it, go ahead and open a PR to react and tag me in it

@iamdustan
Copy link
Contributor

Potentially relevant: facebook/react#6068

@aight8
Copy link

aight8 commented Apr 26, 2016

Definitely! Very good idea.

@johanatan
Copy link

Does this mean that the visual indicators over-represent what's actually being re-rendered? If so, how can their output be trusted/utilized effectively?

@gaearon
Copy link
Contributor

gaearon commented Feb 25, 2017

It can’t. Unfortunately the feature was added without consideration for this.

@gaearon
Copy link
Contributor

gaearon commented Feb 25, 2017

(We totally should fix it in the future. Maybe after Fiber is out.)

@gaearon
Copy link
Contributor

gaearon commented Jun 7, 2017

#804

@gaearon
Copy link
Contributor

gaearon commented Sep 29, 2017

This was fixed in React 16.

@stipsan
Copy link
Contributor Author

stipsan commented Sep 29, 2017

Indeed it was fixed 🎉

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
Projects
None yet
7 participants