useMemoCache implementation #25143
useMemoCache implementation #25143
Conversation
josephsavona
requested review from
kassens,
poteto,
acdlite,
Huxpro and
mofeiZ
|
Comparing: 0556bab...32cd35e Critical size changesIncludes critical production bundles, as well as any change greater than 2%:
Significant size changesIncludes any change greater than 0.2%: Expand to show
|
|
Chatted with @acdlite, i had misunderstood and thought both versions of the fiber tree could be updated at the same time. Since that can't happen we don't need the indirection for current/previous, i'll remove and add handling for multiple useMemoCache calls. |
josephsavona
force-pushed
the
use-memo-cache-impl
branch
from
500767b
to
0cc4b5e
Compare
| test('render component using cache', async () => { | ||
| function Component(props) { | ||
| const cache = useMemoCache(1); | ||
| expect(cache).toBeInstanceOf(Array); |
There was a problem hiding this comment.
minor nit: Array.isArray(cache) might be more accurate?
| console.warn( | ||
| 'Expected each useMemoCache to receive a consistent size argument, found different sizes', | ||
| ); |
There was a problem hiding this comment.
Should this error instead of warn? If this ever happens it seems like something might have gone horribly wrong
There was a problem hiding this comment.
Yeah, fair enough that this should basically never happen with correctly compiled code so we might as well error.
There was a problem hiding this comment.
We also don’t use warn() for anything except deprecated methods
| @@ -579,7 +611,7 @@ export function bailoutHooks( | |||
| current.lanes = removeLanes(current.lanes, lanes); | |||
| } | |||
|
|
|||
| export function resetHooksAfterThrow(): void { | |||
| export function resetHooksAfterThrow(erroredWork: Fiber | null): void { | |||
There was a problem hiding this comment.
Isn't erroredWork the same as currentlyRenderingFiber at this point which is also used elsewhere in this reset function. for consistency maybe just use the module global?
There was a problem hiding this comment.
Good catch, let me try that. It's hard to tell when i should use the global and when not to (lots of functions explicitly pass around a Fiber, including some of the callers of this function)
There was a problem hiding this comment.
I confirmed that currentlyRenderingFiber can be null here. @acdlite is there some global for "current fiber" that I can rely on here? Or is the approach of passing in the fiber from the caller ok?
|
@poteto I actually haven't seen that fire since we ironed out the first bigger issues. We can keep patching it in for our in progress debugging, but I don't see it as a risk later. (These are fairly bad compiler bugs if they happen) |
| let previousMemoCache = null; | ||
| if (memoCache !== null) { | ||
| previousMemoCache = memoCache.data.map(array => array.slice()); | ||
| if (workInProgress.alternate != null) { |
There was a problem hiding this comment.
Nit: Use the current variable. We try to minimize the number of references to .alternate so it's easier to refactor later. Also it happens to minify slightly better if we minimize property access.
| if (memoCache !== null) { | ||
| // Setting state during render could leave the cache in an inconsistent state, | ||
| // reset to the previous state before re-rendering. | ||
| if (previousMemoCache !== null) { |
There was a problem hiding this comment.
I don't understand this comment about setState during render. If we're cloning the current cache, copy-on-write style, we shouldn't have to do anything on unwind. The interrupted cache will get dropped.
There was a problem hiding this comment.
AFAICT workInProgress isn't fully reset when we retry rendering after a setState: the only thing that is reset is what is explicitly done here in this function. So at the beginning we set wIP.memoCache to a clone of the previous value. The render will then mutate that fresh clone, and if there is a setstate that clone will be corrupt. We have to explicitly reset it to a fresh clone of the original value.
There was a problem hiding this comment.
If that's true, that suggests there's a bug somewhere because we access it here as if it's non-null:
Are you sure it was null at the beginning of this function and not the end? The purpose of resetHooksAfterError is to reset all the module-level variables, including currentlyRenderingFiber.
Could you show what scenario you found where it was null at the beginning of this function?
Aside from confirming there's no existing bug, though, passing it from the caller is also fine
| previousMemoCache = memoCache.data.map(array => array.slice()); | ||
| if (workInProgress.alternate != null) { | ||
| // Backup to the alternate fiber in case the component throws | ||
| workInProgress.alternate.memoCache = { |
There was a problem hiding this comment.
We shouldn't need to mutate the current fiber at all. My understanding is we're starting with a copy-on-write approach, same as we do for fibers and hooks: inside useMemoCache, copy the cache from the previous current and put it on the work-in-progress fiber. If something errors, or the render is interrupted, the work-in-progress fiber will be dropped. So you don't need to do any resetting of the current cache or fiber.
| @@ -851,6 +853,7 @@ export function assignFiberPropertiesInDEV( | |||
| target.pendingProps = source.pendingProps; | |||
| target.memoizedProps = source.memoizedProps; | |||
| target.updateQueue = source.updateQueue; | |||
| target.memoCache = source.memoCache; | |||
There was a problem hiding this comment.
Since this field is only relevant to functional components, I suggest putting this field on a different object that only exists for hooks.
I think the best place is the FunctionalComponentUpdateQueue object. The existing fields on that object are conceptually similar because they, too, are copy-on-write. For example, this is how we track usages of useSyncExternalStore:
react/packages/react-reconciler/src/ReactFiberHooks.new.js
Lines 1502 to 1513 in aa80a30
There was a problem hiding this comment.
It's possible we may eventually want to move this to a fiber-level field if we end up using the Forget cache for non-user components, too, but since that's only theoretical for now I think it's best to move it to reduce the memory impact.
There was a problem hiding this comment.
You had suggested that but the updateQueue property is set to null prior to calling the render function, so it isn't clear to me how we'd make the cache value available to useMemoCache(). Do you mean to add a new global that we set/unset as we enter/exit each component?
There was a problem hiding this comment.
You can read the previous values from the current fiber: current.updateQueue
|
Make sure you wrap all the code in the feature flag. To confirm, |
There was a problem hiding this comment.
Thanks all for the comments! Outstanding items that i'm not sure how to address:
- @acdlite See questions about how to store memoCache on the updateQueue, given that the latter is nulled our prior to rendering
- @acdlite During error recovery i'm not sure of the best way to access the fiber that was rendered and failed, see comments
- @gaearon Is throwing reasonable instead of console.warn? Or should I just console.error?
Thanks in advance!
| @@ -579,7 +611,7 @@ export function bailoutHooks( | |||
| current.lanes = removeLanes(current.lanes, lanes); | |||
| } | |||
|
|
|||
| export function resetHooksAfterThrow(): void { | |||
| export function resetHooksAfterThrow(erroredWork: Fiber | null): void { | |||
There was a problem hiding this comment.
Good catch, let me try that. It's hard to tell when i should use the global and when not to (lots of functions explicitly pass around a Fiber, including some of the callers of this function)
| @@ -851,6 +853,7 @@ export function assignFiberPropertiesInDEV( | |||
| target.pendingProps = source.pendingProps; | |||
| target.memoizedProps = source.memoizedProps; | |||
| target.updateQueue = source.updateQueue; | |||
| target.memoCache = source.memoCache; | |||
There was a problem hiding this comment.
You had suggested that but the updateQueue property is set to null prior to calling the render function, so it isn't clear to me how we'd make the cache value available to useMemoCache(). Do you mean to add a new global that we set/unset as we enter/exit each component?
| if (memoCache !== null) { | ||
| // Setting state during render could leave the cache in an inconsistent state, | ||
| // reset to the previous state before re-rendering. | ||
| if (previousMemoCache !== null) { |
There was a problem hiding this comment.
AFAICT workInProgress isn't fully reset when we retry rendering after a setState: the only thing that is reset is what is explicitly done here in this function. So at the beginning we set wIP.memoCache to a clone of the previous value. The render will then mutate that fresh clone, and if there is a setstate that clone will be corrupt. We have to explicitly reset it to a fresh clone of the original value.
| @@ -579,7 +611,7 @@ export function bailoutHooks( | |||
| current.lanes = removeLanes(current.lanes, lanes); | |||
| } | |||
|
|
|||
| export function resetHooksAfterThrow(): void { | |||
| export function resetHooksAfterThrow(erroredWork: Fiber | null): void { | |||
There was a problem hiding this comment.
I confirmed that currentlyRenderingFiber can be null here. @acdlite is there some global for "current fiber" that I can rely on here? Or is the approach of passing in the fiber from the caller ok?
| workInProgress.memoCache = memoCache; | ||
| } | ||
| } | ||
|
|
||
| workInProgress.memoizedState = null; | ||
| workInProgress.updateQueue = null; |
There was a problem hiding this comment.
@acdlite updateQueue is set to null here, so i'm not sure how to pass the memo cache via that object.
There was a problem hiding this comment.
It's on the current fiber: current.updateQueue. This is a really common pattern in the reconciler — conceptually the whole render phase is a copy-on-write of the "current" tree, where "work-in-progress" is the copy.
There was a problem hiding this comment.
current isn't in scope in this file, how do i access this? you mentioned elsewhere that we try not to access .alternate, so it seems like i shouldn't do currentlyRenderingFiber.alternate.
Why do you need to reset after a setState, though? Unlike errors and suspends, we didn't throw / interrupt the function call, so the cache should be consistent even in the current Forget implementation. |
|
To clarify, it's really the combo of setState + early return that's problematic. Simplifying from the test cases that fail w/o this reset: const c_x = x !== $[0];
$[0] = x;
if (cond) {
setState(...);
return;
}
let y;
if (c_x) {
y = $[1] = ...;
} else {
y = $[1];If you hit the setState and early return, then upon the re-render |
|
Turns out switching to store the memoCache on updateQueue makes the question of resetting on setState+render (and error) mute, since both of those cases already reset |
josephsavona
force-pushed
the
use-memo-cache-impl
branch
2 times, most recently
from
0719b66
to
45fb607
Compare
| let updateQueue: FunctionComponentUpdateQueue | null = (currentlyRenderingFiber.updateQueue: any); | ||
| if (updateQueue !== null) { | ||
| memoCache = updateQueue.memoCache; | ||
| } | ||
| // Otherwise clone from the current fiber | ||
| // TODO: not sure how to access the current fiber here other than going through | ||
| // currentlyRenderingFiber.alternate | ||
| if (memoCache === null) { | ||
| const current: Fiber | null = currentlyRenderingFiber.alternate; | ||
| if (current !== null) { | ||
| const currentUpdateQueue: FunctionComponentUpdateQueue | null = (current.updateQueue: any); | ||
| if (currentUpdateQueue !== null) { | ||
| const currentMemoCache: MemoCache | null = currentUpdateQueue.memoCache; | ||
| if (currentMemoCache !== null) { | ||
| memoCache = { | ||
| data: currentMemoCache.data.map(array => array.slice()), | ||
| index: 0, | ||
| }; | ||
| } | ||
| } | ||
| } | ||
| } | ||
| // Finally fall back to allocating a fresh instance | ||
| if (memoCache === null) { | ||
| memoCache = { | ||
| data: [], | ||
| index: 0, | ||
| }; | ||
| } | ||
| if (updateQueue === null) { | ||
| updateQueue = createFunctionComponentUpdateQueue(); | ||
| currentlyRenderingFiber.updateQueue = updateQueue; | ||
| } | ||
| updateQueue.memoCache = memoCache; |
There was a problem hiding this comment.
this whole thing is so tedious without nullish coalescing. is there a reason we don't use that?
let memoCache = currentlyRenderFiber.updateQueue ?? null;
if (memoCache === null) {
const currentMemoCache = currentlyRenderingFiber.alternate?.updateQueue?.memoCache ?? null;
if (currentMemoCache !== null) {
memoCache = cow(currentMemoCache);
}
}
...and similarThere was a problem hiding this comment.
Mostly that we're paranoid about compiler output and it encourages people to reduce redundant type checks by combining branches
There was a problem hiding this comment.
It's good if code that becomes long after compilation also looks long in the source. So that we see when we're blowing up the code size. We generally avoid features that generate disproportionate bundler output. It also forces us to be very explicit about checks. (E.g. why is so much stuff nullable? Is this modeled correctly? — Not talking about your code per se but in general.)
There was a problem hiding this comment.
Fair, though I wonder if there are other ways to achieve those ends. We have a bot that alerts us to unexpected code size increases, and the type system (if used more) would help us with data modeling. A lot of types are any or mixed right now and so you have to code more defensively than may be necessary.
josephsavona
force-pushed
the
use-memo-cache-impl
branch
from
45fb607
to
bebca2f
Compare
| memoCache = updateQueue.memoCache; | ||
| } | ||
| // Otherwise clone from the current fiber | ||
| // TODO: not sure how to access the current fiber here other than going through |
There was a problem hiding this comment.
This is fine, we don't access it that often in this module so probably not worth stashing in a module-level variable
| if (data === undefined) { | ||
| data = memoCache.data[memoCache.index] = new Array(size); | ||
| } else if (data.length !== size) { | ||
| // TODO: consider warning or throwing here |
There was a problem hiding this comment.
console.error is usually what we use for internal invariants because it doesn't add runtime overhead in prod
| } else if (data.length !== size) { | ||
| // TODO: consider warning or throwing here | ||
| } | ||
| memoCache.index++; |
There was a problem hiding this comment.
Do you need this index? Looks like it's always data.length
There was a problem hiding this comment.
Whenever we initialize a cache we set the index to 0, and each call to useMemoCache increments it.
| } | ||
| } | ||
| // Finally fall back to allocating a fresh instance | ||
| if (memoCache === null) { |
There was a problem hiding this comment.
One thing you can do is split this into separate mountMemoCache and updateMemoCache. That's how most of the other hooks are implemented. Then mountMemoCache would always create a new one, and updateMemoCache would always copy.
There was a problem hiding this comment.
I might do this in a followup. To confirm, in mount there is no current (ie currentlyRenderingFiber.alternate == null), and in update there is an alternate?
| let useMemoCache; | ||
| let ErrorBoundary; | ||
|
|
||
| describe('ReactCache', () => { |
There was a problem hiding this comment.
Nit: "cache" is a bit overloaded at this point :D Maybe ReactForgetCache or something
There was a problem hiding this comment.
oh lol this is just copypasta from the ReactCache test, i'll update
| ErrorBoundary = _ErrorBoundary; | ||
| }); | ||
|
|
||
| // @gate experimental || www |
There was a problem hiding this comment.
Nit: I usually prefer to gate on the feature flag, enableUseMemoCacheHook. That way you could change which channels a feature is available in without updating all the gate conditions.
josephsavona
force-pushed
the
use-memo-cache-impl
branch
from
bebca2f
to
90dd08e
Compare
Summary: This sync includes the following changes: - **[0cac4d54c](facebook/react@0cac4d54c )**: Double invoked effects on suspended children ([#25307](facebook/react#25307)) //<Samuel Susla>// - **[3d615fc14](facebook/react@3d615fc14 )**: Grammar. Removed doubles of the word "the". ([#25295](facebook/react#25295)) //<Victoria Graf>// - **[6e3bc8a2e](facebook/react@6e3bc8a2e )**: [DevTools] Check if Proxy exists before creating DispatcherProxy ([#25278](facebook/react#25278)) //<Tianyu Yao>// - **[e7fc04b29](facebook/react@e7fc04b29 )**: [react-dom] Reorganize react-dom internals to match react ([#25277](facebook/react#25277)) //<Josh Story>// - **[0b54e0047](facebook/react@0b54e0047 )**: Handle rejections to avoid uncaught rejections ([#25272](facebook/react#25272)) //<Sebastian Markbåge>// - **[c5d06fdc5](facebook/react@c5d06fdc5 )**: [Flight] Fix Webpack Chunk Loading ([#25271](facebook/react#25271)) //<Sebastian Markbåge>// - **[975b64464](facebook/react@975b64464 )**: [Flight] response.readRoot() -> use(response) ([#25267](facebook/react#25267)) //<Sebastian Markbåge>// - **[60fbb7b14](facebook/react@60fbb7b14 )**: [Flight] Implement FlightClient in terms of Thenable/Promises instead of throwing Promises ([#25260](facebook/react#25260)) //<Sebastian Markbåge>// - **[c91a1e03b](facebook/react@c91a1e03b )**: experimental_useEvent ([#25229](facebook/react#25229)) //<Lauren Tan>// - **[346c7d4c4](facebook/react@346c7d4c4 )**: straightford explicit types ([#25253](facebook/react#25253)) //<Jan Kassens>// - **[3401e9200](facebook/react@3401e9200 )**: useMemoCache implementation ([#25143](facebook/react#25143)) //<Joseph Savona>// - **[0556bab32](facebook/react@0556bab32 )**: [Transition Tracing] More Accurate End Time ([#25105](facebook/react#25105)) //<Luna Ruan>// - **[5fdcd23aa](facebook/react@5fdcd23aa )**: Flow: upgrade to 0.140 ([#25252](facebook/react#25252)) //<Jan Kassens>// - **[5c43c6f02](facebook/react@5c43c6f02 )**: Unwind the current workInProgress if it's suspended ([#25247](facebook/react#25247)) //<Sebastian Markbåge>// - **[e52fa4c57](facebook/react@e52fa4c57 )**: Add early exit to strict mode ([#25235](facebook/react#25235)) //<Samuel Susla>// - **[6aa38e74c](facebook/react@6aa38e74c )**: Flow: enable unsafe-addition error ([#25242](facebook/react#25242)) //<Jan Kassens>// - **[ba7b6f418](facebook/react@ba7b6f418 )**: Flow: upgrade to 0.132 ([#25244](facebook/react#25244)) //<Jan Kassens>// - **[9328988c0](facebook/react@9328988c0 )**: Flow: fix Fiber typed as any ([#25241](facebook/react#25241)) //<Jan Kassens>// - **[c739cef2f](facebook/react@c739cef2f )**: Flow: ReactFiberHotReloading recursive type ([#25225](facebook/react#25225)) //<Jan Kassens>// - **[c156ecd48](facebook/react@c156ecd48 )**: Add some test coverage for some error cases ([#25240](facebook/react#25240)) //<Sebastian Markbåge>// - **[3613284dc](facebook/react@3613284dc )**: experimental_use(context) for server components and ssr ([#25226](facebook/react#25226)) //<mofeiZ>// - **[269c4e975](facebook/react@269c4e975 )**: Prevent infinite re-renders in StrictMode + Offscreen ([#25203](facebook/react#25203)) //<Samuel Susla>// - **[8003ab9cf](facebook/react@8003ab9cf )**: Flow: remove explicit object syntax ([#25223](facebook/react#25223)) //<Jan Kassens>// - **[492c6e29e](facebook/react@492c6e29e )**: Flow: upgrade to 0.127 ([#25221](facebook/react#25221)) //<Jan Kassens>// - **[8a9e7b6ce](facebook/react@8a9e7b6ce )**: Flow: implicit-inexact-object=error ([#25210](facebook/react#25210)) //<Jan Kassens>// - **[37cc6bf12](facebook/react@37cc6bf12 )**: Remove useDeferredValue and useTransition from Flight subset ([#25215](facebook/react#25215)) //<Sebastian Markbåge>// Changelog: [General][Changed] - React Native sync for revisions c28f313...0cac4d5 jest_e2e[run_all_tests] Reviewed By: rickhanlonii Differential Revision: D39696377 fbshipit-source-id: 113878d22d6244b8555b5fb86db1da5d43f7cfd9
* useMemoCache impl * test for multiple calls in a component (from custom hook) * Use array of arrays for multiple calls; use alternate/local as the backup * code cleanup * fix internal test * oops we do not support nullable property access * Simplify implementation, still have questions on some of the PR feedback though * Gate all code based on the feature flag * refactor to use updateQueue * address feedback * Try to eliminate size increase in prod bundle * update to retrigger ci
Summary: This sync includes the following changes: - **[0cac4d54c](facebook/react@0cac4d54c )**: Double invoked effects on suspended children ([facebook#25307](facebook/react#25307)) //<Samuel Susla>// - **[3d615fc14](facebook/react@3d615fc14 )**: Grammar. Removed doubles of the word "the". ([facebook#25295](facebook/react#25295)) //<Victoria Graf>// - **[6e3bc8a2e](facebook/react@6e3bc8a2e )**: [DevTools] Check if Proxy exists before creating DispatcherProxy ([facebook#25278](facebook/react#25278)) //<Tianyu Yao>// - **[e7fc04b29](facebook/react@e7fc04b29 )**: [react-dom] Reorganize react-dom internals to match react ([facebook#25277](facebook/react#25277)) //<Josh Story>// - **[0b54e0047](facebook/react@0b54e0047 )**: Handle rejections to avoid uncaught rejections ([facebook#25272](facebook/react#25272)) //<Sebastian Markbåge>// - **[c5d06fdc5](facebook/react@c5d06fdc5 )**: [Flight] Fix Webpack Chunk Loading ([facebook#25271](facebook/react#25271)) //<Sebastian Markbåge>// - **[975b64464](facebook/react@975b64464 )**: [Flight] response.readRoot() -> use(response) ([facebook#25267](facebook/react#25267)) //<Sebastian Markbåge>// - **[60fbb7b14](facebook/react@60fbb7b14 )**: [Flight] Implement FlightClient in terms of Thenable/Promises instead of throwing Promises ([facebook#25260](facebook/react#25260)) //<Sebastian Markbåge>// - **[c91a1e03b](facebook/react@c91a1e03b )**: experimental_useEvent ([facebook#25229](facebook/react#25229)) //<Lauren Tan>// - **[346c7d4c4](facebook/react@346c7d4c4 )**: straightford explicit types ([facebook#25253](facebook/react#25253)) //<Jan Kassens>// - **[3401e9200](facebook/react@3401e9200 )**: useMemoCache implementation ([facebook#25143](facebook/react#25143)) //<Joseph Savona>// - **[0556bab32](facebook/react@0556bab32 )**: [Transition Tracing] More Accurate End Time ([facebook#25105](facebook/react#25105)) //<Luna Ruan>// - **[5fdcd23aa](facebook/react@5fdcd23aa )**: Flow: upgrade to 0.140 ([facebook#25252](facebook/react#25252)) //<Jan Kassens>// - **[5c43c6f02](facebook/react@5c43c6f02 )**: Unwind the current workInProgress if it's suspended ([facebook#25247](facebook/react#25247)) //<Sebastian Markbåge>// - **[e52fa4c57](facebook/react@e52fa4c57 )**: Add early exit to strict mode ([facebook#25235](facebook/react#25235)) //<Samuel Susla>// - **[6aa38e74c](facebook/react@6aa38e74c )**: Flow: enable unsafe-addition error ([facebook#25242](facebook/react#25242)) //<Jan Kassens>// - **[ba7b6f418](facebook/react@ba7b6f418 )**: Flow: upgrade to 0.132 ([facebook#25244](facebook/react#25244)) //<Jan Kassens>// - **[9328988c0](facebook/react@9328988c0 )**: Flow: fix Fiber typed as any ([facebook#25241](facebook/react#25241)) //<Jan Kassens>// - **[c739cef2f](facebook/react@c739cef2f )**: Flow: ReactFiberHotReloading recursive type ([facebook#25225](facebook/react#25225)) //<Jan Kassens>// - **[c156ecd48](facebook/react@c156ecd48 )**: Add some test coverage for some error cases ([facebook#25240](facebook/react#25240)) //<Sebastian Markbåge>// - **[3613284dc](facebook/react@3613284dc )**: experimental_use(context) for server components and ssr ([facebook#25226](facebook/react#25226)) //<mofeiZ>// - **[269c4e975](facebook/react@269c4e975 )**: Prevent infinite re-renders in StrictMode + Offscreen ([facebook#25203](facebook/react#25203)) //<Samuel Susla>// - **[8003ab9cf](facebook/react@8003ab9cf )**: Flow: remove explicit object syntax ([facebook#25223](facebook/react#25223)) //<Jan Kassens>// - **[492c6e29e](facebook/react@492c6e29e )**: Flow: upgrade to 0.127 ([facebook#25221](facebook/react#25221)) //<Jan Kassens>// - **[8a9e7b6ce](facebook/react@8a9e7b6ce )**: Flow: implicit-inexact-object=error ([facebook#25210](facebook/react#25210)) //<Jan Kassens>// - **[37cc6bf12](facebook/react@37cc6bf12 )**: Remove useDeferredValue and useTransition from Flight subset ([facebook#25215](facebook/react#25215)) //<Sebastian Markbåge>// Changelog: [General][Changed] - React Native sync for revisions c28f313...0cac4d5 jest_e2e[run_all_tests] Reviewed By: rickhanlonii Differential Revision: D39696377 fbshipit-source-id: 113878d22d6244b8555b5fb86db1da5d43f7cfd9
Summary
context: I'm still a noob to this codebase so this started as a hacky implementation to get feedback.
Initial implementation of useMemoCache that aims to at least get the basics correct, even if it doesn't handle all cases and/or there are better ways to implement it (feedback please!). The approach adds a new
memoCache: MemoCache | nullproperty on Fiber. MemoCache contains a current version of the cache and an optional previous version: the previous version is used to restore in the case of a setState during render and/or an error during render. In both cases the cache could become invalid (only partially updated), so restoring in these cases ensures the cache is always in a consistent state. To avoid corrupting the "previous" cache, it is cloned prior to rendering.I wasn't sure if it would be safe to use the alternate fiber's memoCache as the previous instance — both the main and alternate fiber could be rendered concurrently (right?) in which case they could fail and need to restore independently. Let me know if this assumption is incorrect.
How did you test this change?
New unit test.