Bail out on fibers that returned false from shouldComponentUpdate #804
Conversation
backend/ReactTypeOfSideEffect.js
Outdated
| Callback: 32, // 0b00100000 | ||
| Err: 64, // 0b01000000 | ||
| Ref: 128, // 0b10000000 | ||
| }; |
There was a problem hiding this comment.
This concerns me a bit. We might re-arrange add side-effect types in the future and this could get out of sync easily (either in GitHub master, or in a specific deployed version of DevTools vs React).
Could we maybe pass these values in as a named parameter to __REACT_DEVTOOLS_GLOBAL_HOOK__.inject or something?
If we have to hard-code, it seems better to limit it to only the Bailout value (which seems unlikely to change, assuming your inline comment is followed). This way we aren't tempted to do meaningful things with the other values (which may become invalid over time).
There was a problem hiding this comment.
I did this to match TypesOfWork which I previously inlined but if I recall correctly somebody asked me to un-inline it :-). I agree about just using the values directly.
There was a problem hiding this comment.
I'm not worried about Bailout number changing because I left a comment in React itself about it, and React repo logic also depends on it being 1 specifically.
I guess let's remove others, and add notes to React repo for constants we depend on externally (there's some other ones too outside DevTools).
I'd avoid injection for this.
There was a problem hiding this comment.
I'm not worried about Bailout number changing because I left a comment in React itself about it, and React repo logic also depends on it being 1 specifically.
Agreed. 😄
|
We probably need to wait with landing this until FB www sync happens. Since we now skip fibers that don't have |
|
Which also means we need an RN sync for this 😞 |
If facebook/react#9887 gets in, this fixes #337.
Should also make DevTools faster on updates.
Will only work for Fiber.