Add Bailout effectTag for React DevTools #9887
Conversation
|
DevTools PR: facebook/react-devtools#804. It should be safe to check for |
|
|
||
| let firstEffect; | ||
| if (finishedWork.effectTag !== NoEffect) { | ||
| if (finishedWork.effectTag > Bailout) { |
There was a problem hiding this comment.
This was not necessary (it's a root) but I just did this for consistency so I can remove NoEffect import here. I can put this back.
|
I'm not confident that this is sufficient. If you tracked Updates instead of bailouts, I'd be more confident. |
|
Hmm I'm not quite following. Isn't this technically tracking "bailed out Updates"? What's the difference? |
|
In other words do you mean this leads to false positives about updates, or true negatives? Why? |
|
I'm not confident that effectTag will always be restored to NoEffect between multiple passes. I don't know if you can get both Bailout and Update. If you do, what does that mean? Does that mean that it was not a bailout? At the very least it's confusing. If you invert the new flag to be "UpdateHappened", and it doesn't restore to NoEffect, then if there's a conflict it will end up as an "UpdateHappened" which is what I think you want in the case where both happened. |
| (oldProps !== current.memoizedProps || | ||
| oldState !== current.memoizedState) | ||
| ) { | ||
| workInProgress.effectTag |= Update | Bailout; |
There was a problem hiding this comment.
Ah. I see in this case you're actually adding both.
That means that if we're doing a simple two pass reconciliation where the second one is able to reuse the work from the first render, there will be no highlighting even though the component did in fact render?
4 participants
I have not added tests since it's not observable via public API, but I verified it works with React DevTools as expected if I make the necessary changes there.
This lets us fix this feature: facebook/react-devtools#337. It also makes DevTools faster because we can confidently skip bailed trees, which is important since DevTools now traverses the complete tree.
I added this both in development and production because it doesn’t seem expensive to me, and I would prefer this feature to behave consistently. Its current pitfalls are already bad enough that people don’t trust it. I’d rather not trade them for other pitfalls. We can completely disable this in the future if we add a PROFILE build, and PROD build becomes incompatible with DevTools because of advanced optimizations. But I’d rather not break it earlier.
Example before (note how even bailed out AddTodo and other TodoItems are highlighted):
Example after (note how TodoList and one TodoItem are highlighted, others are skipped):