[Fizz] Outline a Suspense Boundary if it has Suspensey CSS or Images #34552
Conversation
github-actions
bot
added
the
React Core Team
| <head> | ||
| <link rel="stylesheet" href="A" data-precedence="A" /> | ||
| <link rel="stylesheet" href="AA" data-precedence="AA" /> | ||
| <link rel="stylesheet" href="A" data-precedence="A" /> |
There was a problem hiding this comment.
The change in precedence here is concerning. It suggests an existing bug that if you suspend or not in a sibling (which causes outlining) can change the precedence.
Shouldn't these always be registered by when we first observe the precedence?
There was a problem hiding this comment.
They are but we don't flush all preceding precedences just to unblock a particular boundary. The way things worked before we should in theory always hoist stylesheets of an inner boundary to the outer if the outer hadn't revealed yet. For some reason that's not happening here and so the later precedences end up appearing earlier in the document. I'll have to look at it more because I can't yet understand why outlining would lead to this since you didn't change the part of the code that does the hoisting. But perhaps the outlining branch needs to explicitly hoist and it doesn't and now this test hits that path instead of the complete boundary path so it ends up skipping a hoist that it should not have.
Oh it's probably because the parent says it flushed and we're relying on the parent flushed to decide whether to hoist or not.
There was a problem hiding this comment.
re
Shouldn't these always be registered by when we first observe the precedence
we could emit dummy style tags with precedence to fill out the precedence order anytime we are about to flush a precedence that has other precedences that precede it. I didn't do this at first b/c I didn't think it was necessary but it seems like it is.
There was a problem hiding this comment.
I solved the immediate problem here by skipping outlining while were emitting partial boundaries. I.e. when a parent blocks a child.
When it comes to pure bytes it doesn't make sense to outline in a partial boundary since the resolution of the parent will come later.
When it comes to outlining because of client resources, it can still make sense to outline in case the parent resolved before the CSS or the image in the child has loaded. However, there's always a tradeoff to outlining so in this case maybe it's ok to skip it.
The consequence is that the parent can be blocked for longer if the CSS gets hoisted, or in the case that the image is blocking then the parent's animation can be blocked on the image. Delaying the parent. However, if the parent doesn't have an animation then it's instead possible that the image isn't blocking but that's only if you have a separate boundary with no animation above which is not resolved initially. Which is not really a case this PR is trying to optimize for. It's mainly when there's a full shell above and you're animating in from that.
There was a problem hiding this comment.
I do believe that there's a separate existing bug unrelated to this PR if a child boundary is also suspended but unsuspends before the parent unsuspends. That would trigger this same scenario. Since ultimately it's because the "complete" boundary instruction of the child would insert it while it's actually still in a hidden subtree.
It could either track all pending parent suspense boundaries to know if it's still in a hidden tree on the server but that suffers from the problem of hoisting something up and now blocking the parent. Alternatively it could track this on the client that a complete only inserts its CSS after the parent is complete.
…e size is not exceeded In this case stylesheets.
…ch might animate Since the client runtime doesn't implement suspensey image semantics unless we're animating, there's no point in outlining unless it's inside a Suspense boundary with an enter animation or a <ViewTransition> around the boundary.
We don't outline when we're emitting partially completed boundaries optimistically because it doesn't make sense to outline something if its parent is going to be blocked on something later in the stream anyway. We won't be able to show the parent anyway so we might as well inline. This is not completely true for when we outline for CSS or images because the reveal of the inner boundary could be still pending after the parent finishes but there's a trade off there.
sebmarkbage
force-pushed
the
fizzoutlinesuspenseyimage
branch
from
fd523ca to
a9f2324
Compare
…34552) We should favor outlining a boundary if it contains Suspensey CSS or Suspensey Images since then we can load that content separately and not block the main content. This also allows us to animate the reveal. For example this should be able to animate the reveal even though the actual HTML content isn't large in this case it's worth outlining so that the JS runtime can choose to animate this reveal. ```js <ViewTransition> <Suspense> <img src="..." /> </Suspense> </ViewTransition> ``` For Suspensey Images, in Fizz, we currently only implement the suspensey semantics when a View Transition is running. Therefore the outlining only applies if it appears inside a Suspense boundary which might animate. Otherwise there's no point in outlining. It is also only if the Suspense boundary itself might animate its appear and not just any ViewTransition. So the effect is very conservative. For CSS it applies even without ViewTransition though, since it can help unblock the main content faster. DiffTrain build for [6eb5d67](6eb5d67)
[diff facebook/react@e2332183...b0c1dc01](facebook/react@e233218...b0c1dc0) <details> <summary>React upstream changes</summary> - facebook/react#34601 - facebook/react#34552 - facebook/react#34524 - facebook/react#34587 - facebook/react#34589 - facebook/react#34585 - facebook/react#34586 </details>
…acebook#34552) We should favor outlining a boundary if it contains Suspensey CSS or Suspensey Images since then we can load that content separately and not block the main content. This also allows us to animate the reveal. For example this should be able to animate the reveal even though the actual HTML content isn't large in this case it's worth outlining so that the JS runtime can choose to animate this reveal. ```js <ViewTransition> <Suspense> <img src="..." /> </Suspense> </ViewTransition> ``` For Suspensey Images, in Fizz, we currently only implement the suspensey semantics when a View Transition is running. Therefore the outlining only applies if it appears inside a Suspense boundary which might animate. Otherwise there's no point in outlining. It is also only if the Suspense boundary itself might animate its appear and not just any ViewTransition. So the effect is very conservative. For CSS it applies even without ViewTransition though, since it can help unblock the main content faster.
We should favor outlining a boundary if it contains Suspensey CSS or Suspensey Images since then we can load that content separately and not block the main content. This also allows us to animate the reveal.
For example this should be able to animate the reveal even though the actual HTML content isn't large in this case it's worth outlining so that the JS runtime can choose to animate this reveal.
For Suspensey Images, in Fizz, we currently only implement the suspensey semantics when a View Transition is running. Therefore the outlining only applies if it appears inside a Suspense boundary which might animate. Otherwise there's no point in outlining. It is also only if the Suspense boundary itself might animate its appear and not just any ViewTransition. So the effect is very conservative.
For CSS it applies even without ViewTransition though, since it can help unblock the main content faster.