Strict mode fix offscreen edge case #25322
Conversation
facebook-github-bot
added
CLA Signed
React Core Team
|
Comparing: cb5084d...ec54d7e Critical size changesIncludes critical production bundles, as well as any change greater than 2%:
Significant size changesIncludes any change greater than 0.2%: (No significant changes) |
sammy-SC
force-pushed
the
strict-mode-fix-offscreen-edge-case
branch
9 times, most recently
from
9a30e80 to
b76fd66
Compare
We need to keep track of fibers that have had effects invoked twice. To achieve this, this commit repurposes flag PlacementDEV to `NeedsDoubleInvokedEffectsDEV`, which is unset by StrictMode. Previous approach with Offscreen was not enough. When previously visible Offscreen is hidden, it can have descendants added. Once Offscreen becomes visible, only newly added components have to have their effects double invoked. Not the previously existing components. Co-authored-by: Robert Balicki <robertbalicki@fb.com>
sammy-SC
force-pushed
the
strict-mode-fix-offscreen-edge-case
branch
from
b76fd66 to
ec54d7e
Compare
There was a problem hiding this comment.
I don't think the behavior described by the test is worth the additional implementation complexity.
A hidden/detached tree is meant to behave identically to an unmounted tree, with the only difference being that the state is reused if it's later made visible again. So it's perfectly consistent to me that if a tree is hidden and then revealed, its effects would be double invoked. More to the point, though, it shouldn't matter either way.
I know you opened this PR because there's a Relay test that depended on the old behavior, but in what way? Is it an actual bug or did the test just happen to assert on a particular behavior?
Any code that is not resilient to double invoking a second time is almost certainly not resilient to actual production Offscreen behavior. Could we fix the Relay code instead? Or maybe the Relay code is fine, but the test is too coupled to implementation details?
|
You're right, hiding/showing Offscreen semantically should be the same as unmounting/mounting three. I thought it would be easier to fix this from our end to unblock the sync. Let me go back to Relay team and see what we can do from their end. It seems they depend on this behaviour in several places, here is one if them. |
|
I think I recall @rbalicki2 said that code was only needed to prevent a duplicate refresh, but there's another strategy Relay could use to account for that instead |
|
If we do need to unblock the sync, it might be worth reverting the Strict Mode refactor (including the ref fix) until we're able to unblock Relay. But I suspect it might be easier to fix or skip those Relay tests instead, as long as it doesn't cause an issue in production. |
|
To unblock the sync, we've decided to put new implementation of strict mode behind a feature flag: #25365 |
We need to keep track of fibers that have had effects invoked twice.
To achieve this, this commit repurposes flag
PlacementDEVtoNeedsDoubleInvokedEffectsDEV, which is unset by StrictMode.Previous approach with Offscreen was not enough. When previously visible
Offscreen is hidden, it can have descendants added. Once Offscreen
becomes visible, only newly added components have to have their effects
double invoked. Not the previously existing components.