Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Unify use and renderDidSuspendDelayIfPossible implementations #25922

Merged
merged 2 commits into from
Jan 4, 2023
Merged

Unify use and renderDidSuspendDelayIfPossible implementations #25922

merged 2 commits into from
Jan 4, 2023

Commits on Jan 4, 2023

  1. Move renderDidSuspend logic to throwException 

    This is part of a larger refactor I'm doing to simplify how this logic
works, since it's currently spread out and duplicated across several
different parts of the work loop.

When a Suspense boundary shows a fallback, and it wasn't already showing
a fallback, we mark the render as suspended. Knowing whether the
in-progress tree includes a new fallback is useful for several reasons:
we can choose to interrupt the current render and switch to a different
task, we can suspend the work loop until more data becomes available, we
can throttle the appearance of multiple fallback states, and so on.

Currently this logic happens in the complete phase, but because we use
renderDidSuspendWithDelay as a signal to interrupt the work loop, it's
best to do it early as we possibly can.

A subsequent step will move the logic even earlier, as soon as we
attempt to unwrap an unresolved promise, so that `use` can determine
whether it's OK to pause the entire work loop and wait for the promise.
There's some existing code that attempts to do this but it doesn't have
parity with how `renderDidSuspend` works, which is the main motivation
for this series of refactors.
    @acdlite
    acdlite committed Jan 4, 2023
    Configuration menu
    Copy the full SHA
    218d4c4 View commit details
    Browse the repository at this point in the history

  2. Unify use and renderDidSuspendWithDelay impl 

    When unwrapping a promise with `use`, we sometimes suspend the work loop
from rendering anything else until the data has resolved. This is
different from how Suspense works in the old throw-a-promise world,
where rather than suspend rendering midway through the render phase, we
prepare a fallback and block the commit at the end, if necessary;
however, the logic for determining whether it's OK to block is the same.
The implementation is only incidentally different because it happens in
two different parts of the code. This means for `use`, we end up doing
the same checks twice, which is wasteful in terms of computataion, but
also introduces a risk that the logic will accidentally diverage.

This unifies the implementation by moving it into the SuspenseContext
module. Most of the logic for deciding whether to suspend is already
performed in the begin phase of SuspenseComponent, so it makes sense to
store that information on the stack rather than recompute it on demand.

The way I've chosen to model this is to track whether the work loop is
rendering inside the "shell" of the tree. The shell is defined as the
part of the tree that's visible in the current UI. Once we enter a 
new Suspense boundary (or a hidden Offscreen boundary, which acts a
Suspense boundary), we're no longer in the shell. This is already how
Suspense behavior was modeled in terms of UX, so using this concept 
directly in the implementation turns out to result in less code
than before.

For the most part, this is purely an internal refactor, though it does
fix a bug in the `use` implementation related to nested Suspense
boundaries. I wouldn't be surprised if it happens to fix other bugs that
we haven't yet discovered, especially around Offscreen. I'll add more
tests as I think of them.
    @acdlite
    acdlite committed Jan 4, 2023
    Configuration menu
    Copy the full SHA
    7ee7066 View commit details
    Browse the repository at this point in the history