Proposed new Suspense layout effect semantics #21079
Conversation
facebook-github-bot
added
CLA Signed
React Core Team
|
Comparing: b48b38a...8c191e8 Critical size changesIncludes critical production bundles, as well as any change greater than 2%:
Significant size changesIncludes any change greater than 0.2%: Expand to show
|
|
This only touches layout effects (so kinda a good thing - considering they are less common in applications, so the impact is minimized). From the comments/tweets in the past I was under the impression that such a change is being discussed for all effects - was it always specifically about layout effects and I've just assumed the other intention? Or maybe passive effects got excluded from that in the process of experimenting with it? Is there any plan to experiment with similar semantics for passive effects? Are React-managed refs those that are passed to the ref prop? Or is there anything else to it? Would it make sense to add a dev-only warning about non-React write attempts to React-managed refs? From what I see there is no such warning. Doing something like this was always not the best idea and I haven't actually seen any code doing it but maybe worth adding something like this nevertheless. |
@Andarist: See this comment in the PR:
We plan to offer a way to cleanup passive effects as well. This is just an incremental step.
React-managed refs are when a |
Will this be controllable by the user for passive effects?
Gotcha, that's what I thought - thanks for clarifying 👍 |
|
@Andarist We'll provide more detail about this soon 😄 ("More to follow.") |
|
Ok, I'll practice being patient. It ain't easy though! 😅 |
| current !== null && | ||
| (current.flags & PassiveStaticEffect) !== | ||
| (workInProgress.flags & PassiveStaticEffect) | ||
| (current.flags & StaticMaskEffect) !== | ||
| (workInProgress.flags & StaticMaskEffect) | ||
| ) { |
There was a problem hiding this comment.
@acdlite mentioned a concern that we might not be resetting static flags correctly. This DEV check has been expanded to also cover the static layout and ref flags.
| if ( | ||
| enableSuspenseLayoutEffectSemantics && | ||
| isModernRoot && | ||
| offscreenSubtreeWasHidden && | ||
| !offscreenSubtreeIsHidden | ||
| ) { |
There was a problem hiding this comment.
I've verified in the built bundle that DCE works to correctly strip these conditional branches when enableSuspenseLayoutEffectSemantics is false.
bvaughn
force-pushed
the
suspense-layout-effects
branch
from
aaaa8e5 to
9563aad
Compare
|
The failing
|
bvaughn
force-pushed
the
suspense-layout-effects
branch
2 times, most recently
from
58a403b to
d9be7b5
Compare
|
I think everything passes now except the Suspense fuzz test, which caught a problem that I need to dig into: <React.Suspense
fallback="Loading..."
>
<React.Suspense>
<React.Suspense>
<Text
initialDelay={9683}
text="E"
updates={[]}
/>
</React.Suspense>
<Text
initialDelay={4053}
text="C"
updates={
[
{
"beginAfter": 1566,
"suspendFor": 4142,
},
{
"beginAfter": 9572,
"suspendFor": 4832,
},
]
}
/>
</React.Suspense>
</React.Suspense>
);The variant yields Looking closer at this, the difference is related to this semantic change. The legacy root output and effects match (because the new semantics only apply to modern roots) but the modern root varies: when a certain subtree is hidden (because it suspends in an update), its layout effects are destroyed and not recreated. In the legacy branch, layout effects are created an additional time. Not yet sure if this indicates a problem with my implementation or just an expected observable difference. |
bvaughn
force-pushed
the
suspense-layout-effects
branch
2 times, most recently
from
786c2c9 to
1ad6bb7
Compare
|
Digging into the above failing test case further. The behavior of both control group and sync legacy render match. (New semantics aren't enabled for legacy roots.) In both groups we only create layout effects once. In the final batch of |
bvaughn
changed the title
bvaughn
force-pushed
the
suspense-layout-effects
branch
5 times, most recently
from
ec2d1ff to
7e40b76
Compare
This PR contains a proposed change to layout effect semantics within Suspense subtrees: If a component mounts within a Suspense boundary and is later hidden (because of something else suspending) React will cleanup that component’s layout effects (including React-managed refs).
This change will hopefully fix existing bugs that occur because of things like reading layout in a hidden tree and will also enable a point at which to e.g. pause videos and hide user-managed portals. After the suspended boundary resolves, React will setup the component’s layout effects again (including React-managed refs).
The scenario described above is not common. The
useTransitionAPI should ensure that Suspense does not revert to its fallback state after being mounted.Note that these changes are primarily written in terms of the (as of yet internal) Offscreen API as we intend to provide similar effects semantics within recently shown/hidden Offscreen trees in the future. (More to follow.)
(Note that all changes in this PR are behind a new feature flag,
enableSuspenseLayoutEffectSemantics, which is disabled for now.)Breakdown
This PR is split into a few commits:
enableSuspenseLayoutEffectSemantics) enabled for variant tests only (__VARIANT__) and toggleable by a GK in Facebook builds.ReactSuspenseEffectsSemantics-test.js) covering the changes to layout effects and refs semantics.ReactSuspenseFuzz-test.internal.js) that fails b'c of the changed semantics. (The failure is expected. A comment was left explaining why.)Checklist
Audit feature flags to make sure DCE works(including for nested if/else statements). OSS build contains no references toRefStatic/LayoutStaticflags,enableSuspenseLayoutEffectSemanticsfeature flag, oroffscreenSubtree* stack variables.Final audit for naming and inline comments.Fix failingReactLazy-test.internal.jstest.Gated failingReactSuspenseFuzz-test.internal.jstest with an explanatory comment.