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

useMutableSource: Use StrictMode double render to detect render phase mutation #20698

Merged
merged 2 commits into from
Feb 1, 2021
Merged

useMutableSource: Use StrictMode double render to detect render phase mutation #20698

merged 2 commits into from
Feb 1, 2021

Commits on Jan 30, 2021

  1. Concurrent Mode test for uMS render mutation 

    Same test as the one added in facebook#20665, but for Concurrent Mode.
    @acdlite
    acdlite committed Jan 30, 2021
    Configuration menu
    Copy the full SHA
    28ab856 View commit details
    Browse the repository at this point in the history

  2. Use double render to detect render phase mutation 

    PR facebook#20665 added a mechanism to detect when a `useMutableSource` source
is mutated during the render phase. It relies on the fact that we double
invoke components that error during development using
`invokeGuardedCallback`. If the version in the double render doesn't
match the first, that indicates there must have been a mutation during
render.

At first I thought it worked by detecting inside the *other* double
render, the one we do for Strict Mode. It turns out that while it does
warn then, the warning is suppressed, because we suppress all console
methods that occur during the Strict Mode double render. So it's really
the `invokeGuardedCallback` one that makes it work.

Anyway, let's set that aside that issue for a second. I realized during
review that errors that occur during the Strict Mode double render
reveal a useful property: A pure component will never throw during the
double render, because if it were pure, it would have also thrown during
the first render... in which case it wouldn't have double rendered! This
is true of all such errors, not just the one thrown by
`useMutableSource`.

Given this, we can simplify the `useMutableSource` mutation detection
mechanism. Instead of tracking and comparing the source's version, we
can instead check if we're inside a double render when the error is
thrown.

To get around the console suppression issue, I changed the warning to an
error. It errors regardless, in both dev and prod, so it doesn't have
semantic implications.

However, because of the paradox I described above, we arguably
_shouldn't_ throw an error in development, since we know that error
won't happen in production, because prod doesn't double render. (It's
still a tearing bug, but that doesn't mean the component will actually
throw.) I considered that, but that requires a larger conversation about
how to handle errors that we know are only possible in development. I
think we should probably be suppressing *all* errors (with a warning)
that occur during a double render.
    @acdlite
    acdlite committed Jan 30, 2021
    Configuration menu
    Copy the full SHA
    c16fe14 View commit details
    Browse the repository at this point in the history