New feature flags to help detect unexpected lifecycle side effects #11587
Conversation
| @@ -168,6 +171,11 @@ export default function( | |||
| ); | |||
| stopPhaseTimer(); | |||
|
|
|||
| // Simulate an async bailout/interruption by invoking lifecycle twice. | |||
| if (invokePrecommitLifecycleHooksTwice) { | |||
There was a problem hiding this comment.
What's the rationale for putting the second call after the timer has ended?
There was a problem hiding this comment.
I think most of these are after the timer. Didn't seem important to put before or after, so long as it was outside of the timer. If you prefer I change it, I'm happy to.
There was a problem hiding this comment.
Nah it's fine either way I think, just curious
|
I think I'll also double-invoke |
packages/shared/ReactFeatureFlags.js
Outdated
| // Helps identify side effects in begin-phase lifecycle hooks: | ||
| export const invokePrecommitLifecycleHooksTwice = false; | ||
| // Helps identify side effects in setState callback: | ||
| export const invokeSetStateCallbackTwice = false; |
There was a problem hiding this comment.
Is there a scenario where it's useful for these to be separate flags?
There was a problem hiding this comment.
Not really. Maybe it was mostly just a naming concern, which is kind of silly I guess. Happy to combine them. Any suggestion for a name that covers both?
There was a problem hiding this comment.
debugRenderPhaseSideEffects seems appropriately generic :D
|
Maybe we should put all these checks inside DEV blocks? |
Considered this. Unfortunately doing that means that we (internally at Facebook) would be much less likely to see them while dog fooding. |
|
Hmm good point. Should get stripped out regardless. |
…acebook#11587) Added `debugRenderPhaseSideEffects` feature flag to help detect unexpected side effects in pre-commit lifecycle hooks and `setState` reducers.
Avoiding side effects in the will-mount/will-update/etc lifecycle hooks becomes very important with async rendering. Unfortunately it's not always easy to detect. This PR adds 2 new feature flags (both disabled by default) to help with this. These flags invoke methods that are supposed to be side-effect-free a second time, roughly approximating what happens when React throws away incomplete work.
Before I started this, I planned to repeat the whole begin phase (eg: cWRP -> sCU -> cWU -> cWRP -> sCU -> cWU -> render) rather than just individual methods (eg cWRP -> cWRP -> sCU -> sCU -> cWU -> cWU -> render). That looks to be more complicated than I had assumed though, and I'm not convinced it's worth the extra effort and complexity. (Since these methods should not have side effects, it should be okay to call them in either sequence.) Happy to discuss this more in depth if others disagree though!
Once these flags are approved, my next step will be to connect them to a GK for the Facebook internal builds, similar to
enableAsyncSchedulingByDefaultInReactDOM.