A passive effect's cleanup function may throw after an unmount. Prior to this commit, such an error would be ignored. (React would not notify any error boundaries.)
After this commit, React will skip any unmounted boundaries and look for a still-mounted boundary. If one is found, it will call getDerivedStateFromError and/or componentDidCatch (depending on the type of boundary). Unmounted boundaries will be ignored, but as they have been unmounted– this seems appropriate.
## Motivation
An *interleaved* update is one that is scheduled while a render is
already in progress, typically from a concurrent user input event.
We have to take care not to process these updates during the current
render, because a multiple interleaved updates may have been scheduled
across many components; to avoid tearing, we cannot render some of
those updates without rendering all of them.
## Old approach
What we currently do when we detect an interleaved update is assign a
lane that is not part of the current render.
This has some unfortunate drawbacks. For example, we will eventually run
out of lanes at a given priority level. When this happens, our last
resort is to interrupt the current render and start over from scratch.
If this happens enough, it can lead to starvation.
More concerning, there are a suprising number of places that must
separately account for this case, often in subtle ways. The maintenance
complexity has led to a number of tearing bugs.
## New approach
I added a new field to the update queue, `interleaved`. It's a linked
list, just like the `pending` field. When an interleaved update is
scheduled, we add it to the `interleaved` list instead of `pending`.
Then we push the entire queue object onto a global array. When the
current render exits, we iterate through the array of interleaved queues
and transfer the `interleaved` list to the `pending` list.
So, until the current render has exited (whether due to a commit or an
interruption), it's impossible to process an interleaved update, because
they have not yet been enqueued.
In this new approach, we don't need to resort to clever lanes tricks to
avoid inconsistencies. This should allow us to simplify a lot of the
logic that's currently in ReactFiberWorkLoop and ReactFiberLane,
especially `findUpdateLane` and `getNextLanes`. All the logic for
interleaved updates is isolated to one place.
This combines changes originally made in #19523, #20028, and #20415 but with slightly different semantics: "strict effects" mode is enabled only for the experimental root APIs (never for legacy render, regardless of <StrictMode> usage). These semantics may change slightly in the future.
DevTools was built with a fork of an early idea for how Suspense cache might work. This idea is incompatible with newer APIs like `useTransition` which unfortunately prevented me from making certain UX improvements. This PR swaps out the primary usage of this cache (there are a few) in favor of the newer `unstable_getCacheForType` and `unstable_useCacheRefresh` APIs. We can go back and update the others in follow up PRs.
### Messaging changes
I've refactored the way the frontend loads component props/state/etc to hopefully make it better match the Suspense+cache model. Doing this gave up some of the small optimizations I'd added but hopefully the actual performance impact of that is minor and the overall ergonomic improvements of working with the cache API make this worth it.
The backend no longer remembers inspected paths. Instead, the frontend sends them every time and the backend sends a response with those paths. I've also added a new "force" parameter that the frontend can use to tell the backend to send a response even if the component hasn't rendered since the last time it asked. (This is used to get data for newly inspected paths.)
_Initial inspection..._
```
front | | back
| -- "inspect" (id:1, paths:[], force:true) ---------> |
| <------------------------ "inspected" (full-data) -- |
```
_1 second passes with no updates..._
```
| -- "inspect" (id:1, paths:[], force:false) --------> |
| <------------------------ "inspected" (no-change) -- |
```
_User clicks to expand a path, aka hydrate..._
```
| -- "inspect" (id:1, paths:['foo'], force:true) ----> |
| <------------------------ "inspected" (full-data) -- |
```
_1 second passes during which there is an update..._
```
| -- "inspect" (id:1, paths:['foo'], force:false) ---> |
| <----------------- "inspectedElement" (full-data) -- |
```
### Clear errors/warnings transition
Previously this meant there would be a delay after clicking the "clear" button. The UX after this change is much improved.
### Hydrating paths transition
I also added a transition to hydration (expanding "dehyrated" paths).
### Better error boundaries
I also added a lower-level error boundary in case the new suspense operation ever failed. It provides a better "retry" mechanism (select a new element) so DevTools doesn't become entirely useful. Here I'm intentionally causing an error every time I select an element.
### Improved snapshot tests
I also migrated several of the existing snapshot tests to use inline snapshots and added a new serializer for dehydrated props. Inline snapshots are easier to verify and maintain and the new serializer means dehydrated props will be formatted in a way that makes sense rather than being empty (in external snapshots) or super verbose (default inline snapshot format).
We need to call `detachFiberAfterEffects` on the fiber's alternate to
clean it up. We're currently not, because the `alternate` field is
cleared during `detachFiberMutation`. So I deferred detaching the
`alternate` until the passive phase. Only the `return` pointer needs to
be detached for semantic purposes.
I don't think there's any good way to test this without using
reflection. It's not even observable using out our "supported"
reflection APIs (`findDOMNode`), or at least not that I can think of.
Which is a good thing, in a way.
It's not really a memory leak, either, because the only reference to the
alternate fiber is from the parent's alternate. Which will be
disconnected the next time the parent is updated or deleted.
It's really only observable if you mess around with internals in ways
you're not supposed to — I found it because a product test in www that
uses Enzyme was doing just that.
In lieu of a new unit test, I confirmed this patch fixes the broken
product test.
Passive flags are a new concept that is tricky to get right. We've
already found two bugs related to PassiveStatic. Let's remove this
optimization for now, and add it back once the main part of the effects
refactor lands.
As per Seb's comment in #20465, we need to do the same thing in React Native as we do in Relay.
When `parseModel` suspends because of missing dependencies, it will exit and retry to parse later. However, in the relay implementation, the model is an object that we modify in place when we parse it, so when we we retry, part of the model might be parsed already into React elements, which will error because the parsing code expect a Flight model. This diff clones instead of mutating the original model, which fixes this error.
When `parseModel` suspends because of missing dependencies, it will exit and retry to parse later. However, in the relay implementation, the model is an object that we modify in place when we parse it, so when we we retry, part of the model might be parsed already into React elements, which will error because the parsing code expect a Flight model. This diff clones instead of mutating the original model, which fixes this error.
@sebmarkbage reminded me that the complete phase of SuspenseList
will sometimes enter the begin phase of the children without calling
`createWorkInProgress` again, instead calling `resetWorkInProgress`.
This was raised in the context of considering whether #20398 might
have accidentally caused a SuspenseList bug. (I did look at this code
at the time, but considering how complicated SuspenseList is it's not
hard to imagine that I made a mistake.)
Anyway, I think that PR is fine; however, reviewing it again did lead me
to find a different bug. This new bug is actually a variant of the bug
fixed by #20398.
`resetWorkInProgress` clears a fiber's static flags. That's wrong, since
static flags -- like PassiveStatic -- are meant to last the lifetime of
the component.
In more practical terms, what happens is that if a direct child of
SuspenseList contains a `useEffect`, then SuspenseList will cause the
child to "forget" that it contains passive effects. When the child
is deleted, its unmount effects are not fired :O
This is the second of this type of bug I've found, which indicates to me
that it's too easy to accidentally clear static flags.
Maybe we should only update the `flags` field using helper functions,
like we do with `lanes`.
Or perhaps we add an internal warning somewhere that detects when a
fiber has different static flags than its alternate.
We replay errors so you can break on paused exceptions. This is done in
the second pass so that the first pass can ignore suspense.
Originally this threw the original error. For suppression purposes
we copied the flag onto the original error.
https://github.com/facebook/react/blob/f1dc626b29b8bf0f14c75a8525e8650b7ea94a47/packages/react-reconciler/src/ReactFiberScheduler.old.js#L367-L369
During this refactor it changed to just throw the retried error:
https://github.com/facebook/react/pull/15151
We're not sure exactly why but it was likely just an optimization or
clean up.
So we can go back to throwing the original error. That helps in the case
where a memoized function is naively not rethrowing each time such as
in Node's module system.
Unfortunately this doesn't fix the problem fully.
Because invokeGuardedCallback captures the error and logs it to the browser.
So you still end up seeing the wrong message in the logs.
This just fixes so that the error boundary sees the first one.
Sebastian Markbåge
Brian Vaughn
Berton Zhu
Andrew Clark
Dima Tisnek
Ricky
ChenLei
Christian Ruigrok
Jaiwanth
Sebastian Silbermann
Luna Ruan
Oliver Lassen
Dan Abramov