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

Bugfix: Nested useOpaqueIdentifier references #22553

Merged
merged 2 commits into from
Oct 13, 2021
Merged

Bugfix: Nested useOpaqueIdentifier references #22553

merged 2 commits into from
Oct 13, 2021

Commits on Oct 13, 2021

  1. Handle render phase updates explicitly 

    We fire a warning in development if a component is updated during
the render phase (with the exception of local hook updates, which have
their own defined behavior).

Because it's not a supported React pattern, we don't have that many
tests that trigger this path. But it is meant to have reasonable
semantics when it does happen, so that if it accidentally ships to
production, the app doesn't crash unnecessarily. The behavior is not
super well-defined, though.

There are also some _internal_ React implementation details that
intentionally to rely on this behavior. Most prominently, selective
hydration and useOpaqueIdentifier.

I need to tweak the behavior of render phase updates slightly as part
of a fix for useOpaqueIdentifier. This shouldn't cause a user-facing
change in behavior outside of useOpaqueIdentifier, but it does require
that we explicitly model render phase updates.
    @acdlite
    acdlite committed Oct 13, 2021
    Configuration menu
    Copy the full SHA
    c71d3ab View commit details
    Browse the repository at this point in the history

  2. Bugfix: Nested useOpaqueIdentifier calls 

    Fixes an issue where multiple useOpaqueIdentifier hooks are upgraded
to client ids within the same render.

The way the upgrade works is that useOpaqueIdentifier schedules a
render phase update then throws an error to trigger React's error
recovery mechanism.

The normal error recovery mechanism is designed for errors that occur
as a result of interleaved mutations, so we usually only retry a single
time, synchronously, before giving up.

useOpaqueIdentifier is different because the error its throws when
upgrading is not caused by an interleaved mutation. Rather, it happens
when an ID is referenced for the first time inside a client-rendered
tree (i.e. sommething that wasn't part of the initial server render).
The fact that it relies on the error recovery mechanism is an
implementation detail. And a single recovery attempt may be
insufficient. For example, if a parent and a child component may
reference different ids, and both are mounted as a result of the same
client update, that will trigger two separate error recovery attempts.

Because render phase updates are not allowed when triggered from
userspace — we log a warning in developement to prevent them —
we can assume that if something does update during the render phase, it
is one of our "legit" implementation details like useOpaqueIdentifier.
So we can keep retrying until we succeed — up to a limit, to protect
against inifite loops. I chose 50 since that's the limit we use for
commit phase updates.
    @acdlite
    acdlite committed Oct 13, 2021
    Configuration menu
    Copy the full SHA
    aca9278 View commit details
    Browse the repository at this point in the history