From 841ea33a7a7b8c9b4e043f68a7c90765f429275b Mon Sep 17 00:00:00 2001 From: Joe Savona Date: Thu, 25 Aug 2022 16:25:17 +0100 Subject: [PATCH 01/12] useMemoCache impl --- .../react-reconciler/src/ReactFiber.new.js | 2 + .../src/ReactFiberHooks.new.js | 49 ++- .../src/ReactFiberWorkLoop.new.js | 4 +- .../src/ReactInternalTypes.js | 8 + .../src/__tests__/useMemoCache-test.js | 306 ++++++++++++++++++ 5 files changed, 365 insertions(+), 4 deletions(-) create mode 100644 packages/react-reconciler/src/__tests__/useMemoCache-test.js diff --git a/packages/react-reconciler/src/ReactFiber.new.js b/packages/react-reconciler/src/ReactFiber.new.js index d6a6c8f7adc19..a34d0fd2cff06 100644 --- a/packages/react-reconciler/src/ReactFiber.new.js +++ b/packages/react-reconciler/src/ReactFiber.new.js @@ -144,6 +144,7 @@ function FiberNode( this.updateQueue = null; this.memoizedState = null; this.dependencies = null; + this.memoCache = null; this.mode = mode; @@ -310,6 +311,7 @@ export function createWorkInProgress(current: Fiber, pendingProps: any): Fiber { workInProgress.memoizedProps = current.memoizedProps; workInProgress.memoizedState = current.memoizedState; workInProgress.updateQueue = current.updateQueue; + workInProgress.memoCache = current.memoCache; // Clone the dependencies object. This is mutated during the render phase, so // it cannot be shared with the current fiber. diff --git a/packages/react-reconciler/src/ReactFiberHooks.new.js b/packages/react-reconciler/src/ReactFiberHooks.new.js index 3f45ec61434a9..eccbd27262bbe 100644 --- a/packages/react-reconciler/src/ReactFiberHooks.new.js +++ b/packages/react-reconciler/src/ReactFiberHooks.new.js @@ -399,6 +399,13 @@ export function renderWithHooks( current !== null && current.type !== workInProgress.type; } + const memoCache = workInProgress.memoCache; + if (memoCache !== null) { + // Clone memo cache prior to rendering to avoid corruption in case of error + memoCache.previous = memoCache.current; + memoCache.current = [...memoCache.current]; + } + workInProgress.memoizedState = null; workInProgress.updateQueue = null; workInProgress.lanes = NoLanes; @@ -470,6 +477,20 @@ export function renderWithHooks( workInProgress.updateQueue = null; + if (memoCache !== null) { + // Re-clone the cache, setting state in the middle of rendering can leave the cache + // in an invalid state + const previous = memoCache.previous; + if (previous !== null) { + memoCache.current = [...previous]; + } else { + console.warn( + 'Expected a previous memo cache instance to have been cached prior to render. This is a bug in React.', + ); + memoCache.current = new Array(memoCache.current.length); + } + } + if (__DEV__) { // Also validate hook order for cascading updates. hookTypesUpdateIndexDev = -1; @@ -583,7 +604,7 @@ export function bailoutHooks( current.lanes = removeLanes(current.lanes, lanes); } -export function resetHooksAfterThrow(): void { +export function resetHooksAfterThrow(erroredWork: Fiber | null): void { // We can assume the previous dispatcher is always this one, since we set it // at the beginning of the render phase and there's no re-entrance. ReactCurrentDispatcher.current = ContextOnlyDispatcher; @@ -608,6 +629,23 @@ export function resetHooksAfterThrow(): void { didScheduleRenderPhaseUpdate = false; } + // The current memo cache may be in an inconsistent state, reset to the previous + // version of the cache. + if (erroredWork != null) { + const memoCache = erroredWork.memoCache; + if (memoCache !== null) { + if (memoCache.previous === null) { + console.warn( + 'Expected a previous useMemoCache to be present if rendering of a memoized component errored. This is likely a bug in React', + ); + } + memoCache.current = + memoCache.previous !== null + ? [...memoCache.previous] + : new Array(memoCache.current.length); + } + } + renderLanes = NoLanes; currentlyRenderingFiber = (null: any); @@ -787,7 +825,14 @@ function use(usable: Usable): T { } function useMemoCache(size: number): Array { - throw new Error('Not implemented.'); + let memoCache = currentlyRenderingFiber.memoCache; + if (memoCache === null) { + memoCache = currentlyRenderingFiber.memoCache = { + current: new Array(size), + previous: null, + }; + } + return memoCache.current; } function basicStateReducer(state: S, action: BasicStateAction): S { diff --git a/packages/react-reconciler/src/ReactFiberWorkLoop.new.js b/packages/react-reconciler/src/ReactFiberWorkLoop.new.js index ca6bbcae278ab..9227c2cbded78 100644 --- a/packages/react-reconciler/src/ReactFiberWorkLoop.new.js +++ b/packages/react-reconciler/src/ReactFiberWorkLoop.new.js @@ -1696,7 +1696,7 @@ function prepareFreshStack(root: FiberRoot, lanes: Lanes): Fiber { function handleThrow(root, thrownValue): void { // Reset module-level state that was set during the render phase. resetContextDependencies(); - resetHooksAfterThrow(); + resetHooksAfterThrow(workInProgress); resetCurrentDebugFiberInDEV(); // TODO: I found and added this missing line while investigating a // separate issue. Write a regression test using string refs. @@ -3347,7 +3347,7 @@ if (__DEV__ && replayFailedUnitOfWorkWithInvokeGuardedCallback) { // Keep this code in sync with handleThrow; any changes here must have // corresponding changes there. resetContextDependencies(); - resetHooksAfterThrow(); + resetHooksAfterThrow(current); // Don't reset current debug fiber, since we're about to work on the // same fiber again. diff --git a/packages/react-reconciler/src/ReactInternalTypes.js b/packages/react-reconciler/src/ReactInternalTypes.js index df7398c7963d0..76838182cbfa3 100644 --- a/packages/react-reconciler/src/ReactInternalTypes.js +++ b/packages/react-reconciler/src/ReactInternalTypes.js @@ -68,6 +68,11 @@ export type Dependencies = { ... }; +export type MemoCache = { + current: Array, + previous: Array | null, +}; + // A Fiber is work on a Component that needs to be done or was done. There can // be more than one per component. export type Fiber = { @@ -128,6 +133,9 @@ export type Fiber = { // A queue of state updates and callbacks. updateQueue: mixed, + // useMemoCache storage + memoCache: MemoCache | null, + // The state used to create the output memoizedState: any, diff --git a/packages/react-reconciler/src/__tests__/useMemoCache-test.js b/packages/react-reconciler/src/__tests__/useMemoCache-test.js new file mode 100644 index 0000000000000..1566a7d093c27 --- /dev/null +++ b/packages/react-reconciler/src/__tests__/useMemoCache-test.js @@ -0,0 +1,306 @@ +let React; +let ReactNoop; +let Cache; +let getCacheSignal; +let getCacheForType; +let Scheduler; +let act; +let Suspense; +let Offscreen; +let useCacheRefresh; +let startTransition; +let useState; +let useMemoCache; +let ErrorBoundary; + +describe('ReactCache', () => { + beforeEach(() => { + jest.resetModules(); + + React = require('react'); + ReactNoop = require('react-noop-renderer'); + Cache = React.unstable_Cache; + Scheduler = require('scheduler'); + act = require('jest-react').act; + Suspense = React.Suspense; + Offscreen = React.unstable_Offscreen; + getCacheSignal = React.unstable_getCacheSignal; + getCacheForType = React.unstable_getCacheForType; + useCacheRefresh = React.unstable_useCacheRefresh; + startTransition = React.startTransition; + useState = React.useState; + useMemoCache = React.unstable_useMemoCache; + + class _ErrorBoundary extends React.Component { + constructor(props) { + super(props); + this.state = {hasError: false}; + } + + static getDerivedStateFromError(error) { + // Update state so the next render will show the fallback UI. + return {hasError: true}; + } + + componentDidCatch(error, errorInfo) {} + + render() { + if (this.state.hasError) { + // You can render any custom fallback UI + return

Something went wrong.

; + } + + return this.props.children; + } + } + ErrorBoundary = _ErrorBoundary; + }); + + // @gate experimental || www + test('render component using cache', async () => { + function Component(props) { + const cache = useMemoCache(1); + expect(cache).toBeInstanceOf(Array); + expect(cache.length).toBe(1); + expect(cache[0]).toBe(undefined); + return 'Ok'; + } + const root = ReactNoop.createRoot(); + await act(async () => { + root.render(); + }); + expect(root).toMatchRenderedOutput('Ok'); + }); + + // @gate experimental || www + test('update component using cache', async () => { + let setX; + let forceUpdate; + function Component(props) { + const cache = useMemoCache(4); + + // x is used to produce a `data` object passed to the child + const [x, _setX] = useState(0); + setX = _setX; + const c_x = x !== cache[0]; + cache[0] = x; + + // n is passed as-is to the child as a cache breaker + const [n, setN] = useState(0); + forceUpdate = () => setN(n => n + 1); + const c_n = n !== cache[1]; + cache[1] = n; + + let data; + if (c_x) { + data = cache[2] = {text: `Count ${x}`}; + } else { + data = cache[2]; + } + if (c_x || c_n) { + return (cache[3] = ); + } else { + return cache[3]; + } + } + let data; + const Text = jest.fn(function Text(props) { + data = props.data; + return data.text; + }); + + const root = ReactNoop.createRoot(); + await act(async () => { + root.render(); + }); + expect(root).toMatchRenderedOutput('Count 0'); + expect(Text).toBeCalledTimes(1); + let data0 = data; + + // Changing x should reset the data object + await act(async () => { + setX(1); + }); + expect(root).toMatchRenderedOutput('Count 1'); + expect(Text).toBeCalledTimes(2); + expect(data).not.toBe(data0); + const data1 = data; + + // Forcing an unrelated update shouldn't recreate the + // data object. + await act(async () => { + forceUpdate(); + }); + expect(root).toMatchRenderedOutput('Count 1'); + expect(Text).toBeCalledTimes(3); + expect(data).toBe(data1); // confirm that the cache persisted across renders + }); + + // @gate experimental || www + test('update component using cache with setstate during render', async () => { + let setX; + let setN; + function Component(props) { + const cache = useMemoCache(4); + + // x is used to produce a `data` object passed to the child + const [x, _setX] = useState(0); + setX = _setX; + const c_x = x !== cache[0]; + cache[0] = x; + + // n is passed as-is to the child as a cache breaker + const [n, _setN] = useState(0); + setN = _setN; + const c_n = n !== cache[1]; + cache[1] = n; + + // NOTE: setstate and early return here means that x will update + // without the data value being updated. Subsequent renders could + // therefore think that c_x = false (hasn't changed) and skip updating + // data. + // The memoizing compiler will have to handle this case, but the runtime + // can help by falling back to resetting the cache if a setstate occurs + // during render (this mirrors what we do for useMemo and friends) + if (n === 1) { + setN(2); + return; + } + + let data; + if (c_x) { + data = cache[2] = {text: `Count ${x}`}; + } else { + data = cache[2]; + } + if (c_x || c_n) { + return (cache[3] = ); + } else { + return cache[3]; + } + } + let data; + const Text = jest.fn(function Text(props) { + data = props.data; + return data.text; + }); + + const root = ReactNoop.createRoot(); + await act(async () => { + root.render(); + }); + expect(root).toMatchRenderedOutput('Count 0'); + expect(Text).toBeCalledTimes(1); + let data0 = data; + + // Simultaneously trigger an update to x (should create a new data value) + // and trigger the setState+early return. The runtime should reset the cache + // to avoid an inconsistency + await act(async () => { + setX(1); + setN(1); + }); + expect(root).toMatchRenderedOutput('Count 1'); + expect(Text).toBeCalledTimes(2); + expect(data).not.toBe(data0); + const data1 = data; + + // Forcing an unrelated update shouldn't recreate the + // data object. + await act(async () => { + setN(3); + }); + expect(root).toMatchRenderedOutput('Count 1'); + expect(Text).toBeCalledTimes(3); + expect(data).toBe(data1); // confirm that the cache persisted across renders + }); + + // @gate experimental || www + test('update component using cache with throw during render', async () => { + let setX; + let setN; + let shouldFail = true; + function Component(props) { + const cache = useMemoCache(4); + + // x is used to produce a `data` object passed to the child + const [x, _setX] = useState(0); + setX = _setX; + const c_x = x !== cache[0]; + cache[0] = x; + + // n is passed as-is to the child as a cache breaker + const [n, _setN] = useState(0); + setN = _setN; + const c_n = n !== cache[1]; + cache[1] = n; + + // NOTE the initial failure will trigger a re-render, after which the function + // will early return. This validates that the runtime resets the cache on error: + // if it doesn't the cache will be corrupt, with the cached version of data + // out of data from the cached version of x. + if (n === 1) { + if (shouldFail) { + shouldFail = false; + throw new Error('failed'); + } + setN(2); + return; + } + + let data; + if (c_x) { + data = cache[2] = {text: `Count ${x}`}; + } else { + data = cache[2]; + } + if (c_x || c_n) { + return (cache[3] = ); + } else { + return cache[3]; + } + } + let data; + const Text = jest.fn(function Text(props) { + data = props.data; + return data.text; + }); + + spyOnDev(console, 'error'); + + const root = ReactNoop.createRoot(); + await act(async () => { + root.render( + + + , + ); + }); + expect(root).toMatchRenderedOutput('Count 0'); + expect(Text).toBeCalledTimes(1); + let data0 = data; + + // Simultaneously trigger an update to x (should create a new data value) + // and trigger the setState+early return. The runtime should reset the cache + // to avoid an inconsistency + await act(async () => { + // this update bumps the count + setX(1); + // this triggers a throw. + setN(1); + }); + expect(root).toMatchRenderedOutput('Count 1'); + expect(Text).toBeCalledTimes(2); + expect(data).not.toBe(data0); + const data1 = data; + + // Forcing an unrelated update shouldn't recreate the + // data object. + await act(async () => { + setN(3); + }); + expect(root).toMatchRenderedOutput('Count 1'); + expect(Text).toBeCalledTimes(3); + expect(data).toBe(data1); // confirm that the cache persisted across renders + }); +}); From 91112dcd11e4e1c7da2bb98e0e22cabdb79bfaf5 Mon Sep 17 00:00:00 2001 From: Joe Savona Date: Fri, 26 Aug 2022 15:01:39 +0100 Subject: [PATCH 02/12] test for multiple calls in a component (from custom hook) --- .../src/__tests__/useMemoCache-test.js | 77 +++++++++++++++++++ 1 file changed, 77 insertions(+) diff --git a/packages/react-reconciler/src/__tests__/useMemoCache-test.js b/packages/react-reconciler/src/__tests__/useMemoCache-test.js index 1566a7d093c27..2b9c229d27f41 100644 --- a/packages/react-reconciler/src/__tests__/useMemoCache-test.js +++ b/packages/react-reconciler/src/__tests__/useMemoCache-test.js @@ -303,4 +303,81 @@ describe('ReactCache', () => { expect(Text).toBeCalledTimes(3); expect(data).toBe(data1); // confirm that the cache persisted across renders }); + + // @gate experimental || www + test('update component and custom hook with caches', async () => { + let setX; + let forceUpdate; + function Component(props) { + const cache = useMemoCache(4); + + // x is used to produce a `data` object passed to the child + const [x, _setX] = useState(0); + setX = _setX; + const c_x = x !== cache[0]; + cache[0] = x; + + // n is passed as-is to the child as a cache breaker + const [n, setN] = useState(0); + forceUpdate = () => setN(n => n + 1); + const c_n = n !== cache[1]; + cache[1] = n; + + let _data; + if (c_x) { + _data = cache[2] = {text: `Count ${x}`}; + } else { + _data = cache[2]; + } + const data = useData(_data); + if (c_x || c_n) { + return (cache[3] = ); + } else { + return cache[3]; + } + } + function useData(data) { + const cache = useMemoCache(2); + const c_data = data !== cache[0]; + cache[0] = data; + let nextData; + if (c_data) { + nextData = cache[1] = {text: data.text.toLowerCase()}; + } else { + nextData = cache[1]; + } + return cache[1]; + } + let data; + const Text = jest.fn(function Text(props) { + data = props.data; + return data.text; + }); + + const root = ReactNoop.createRoot(); + await act(async () => { + root.render(); + }); + expect(root).toMatchRenderedOutput('count 0'); + expect(Text).toBeCalledTimes(1); + let data0 = data; + + // Changing x should reset the data object + await act(async () => { + setX(1); + }); + expect(root).toMatchRenderedOutput('count 1'); + expect(Text).toBeCalledTimes(2); + expect(data).not.toBe(data0); + const data1 = data; + + // Forcing an unrelated update shouldn't recreate the + // data object. + await act(async () => { + forceUpdate(); + }); + expect(root).toMatchRenderedOutput('count 1'); + expect(Text).toBeCalledTimes(3); + expect(data).toBe(data1); // confirm that the cache persisted across renders + }); }); From 11081d10bb6b11524d608a36f7e9fbbe96231642 Mon Sep 17 00:00:00 2001 From: Joe Savona Date: Mon, 29 Aug 2022 13:52:52 -0700 Subject: [PATCH 03/12] Use array of arrays for multiple calls; use alternate/local as the backup --- .../react-reconciler/src/ReactFiber.new.js | 11 ++- .../react-reconciler/src/ReactFiber.old.js | 11 +++ .../src/ReactFiberHooks.new.js | 67 +++++++++++------- .../src/ReactFiberHooks.old.js | 68 ++++++++++++++++++- .../src/ReactFiberWorkLoop.new.js | 2 +- .../src/ReactFiberWorkLoop.old.js | 4 +- .../src/ReactInternalTypes.js | 4 +- .../src/__tests__/useMemoCache-test.js | 30 ++------ 8 files changed, 142 insertions(+), 55 deletions(-) diff --git a/packages/react-reconciler/src/ReactFiber.new.js b/packages/react-reconciler/src/ReactFiber.new.js index a34d0fd2cff06..e9daf4bdf245d 100644 --- a/packages/react-reconciler/src/ReactFiber.new.js +++ b/packages/react-reconciler/src/ReactFiber.new.js @@ -311,7 +311,16 @@ export function createWorkInProgress(current: Fiber, pendingProps: any): Fiber { workInProgress.memoizedProps = current.memoizedProps; workInProgress.memoizedState = current.memoizedState; workInProgress.updateQueue = current.updateQueue; - workInProgress.memoCache = current.memoCache; + + const memoCache = current.memoCache; + if (memoCache !== null) { + workInProgress.memoCache = { + data: memoCache.data.map(data => data.slice()), + index: 0, + }; + } else { + workInProgress.memoCache = null; + } // Clone the dependencies object. This is mutated during the render phase, so // it cannot be shared with the current fiber. diff --git a/packages/react-reconciler/src/ReactFiber.old.js b/packages/react-reconciler/src/ReactFiber.old.js index d8f1ef424bd13..5fe41257c6d90 100644 --- a/packages/react-reconciler/src/ReactFiber.old.js +++ b/packages/react-reconciler/src/ReactFiber.old.js @@ -144,6 +144,7 @@ function FiberNode( this.updateQueue = null; this.memoizedState = null; this.dependencies = null; + this.memoCache = null; this.mode = mode; @@ -311,6 +312,16 @@ export function createWorkInProgress(current: Fiber, pendingProps: any): Fiber { workInProgress.memoizedState = current.memoizedState; workInProgress.updateQueue = current.updateQueue; + const memoCache = current.memoCache; + if (memoCache !== null) { + workInProgress.memoCache = { + data: memoCache.data.map(data => data.slice()), + index: 0, + }; + } else { + workInProgress.memoCache = null; + } + // Clone the dependencies object. This is mutated during the render phase, so // it cannot be shared with the current fiber. const currentDependencies = current.dependencies; diff --git a/packages/react-reconciler/src/ReactFiberHooks.new.js b/packages/react-reconciler/src/ReactFiberHooks.new.js index eccbd27262bbe..7a28a750aea00 100644 --- a/packages/react-reconciler/src/ReactFiberHooks.new.js +++ b/packages/react-reconciler/src/ReactFiberHooks.new.js @@ -400,10 +400,17 @@ export function renderWithHooks( } const memoCache = workInProgress.memoCache; + let previousMemoCache = null; if (memoCache !== null) { - // Clone memo cache prior to rendering to avoid corruption in case of error - memoCache.previous = memoCache.current; - memoCache.current = [...memoCache.current]; + // Clone the cache to allow resetting to the most recent version in case of a setstate during render + previousMemoCache = memoCache.data.map(data => data.slice()); + if (workInProgress.alternate != null) { + workInProgress.alternate.memoCache = { + data: previousMemoCache, + index: 0, + }; + } + memoCache.index = 0; } workInProgress.memoizedState = null; @@ -478,17 +485,20 @@ export function renderWithHooks( workInProgress.updateQueue = null; if (memoCache !== null) { - // Re-clone the cache, setting state in the middle of rendering can leave the cache - // in an invalid state - const previous = memoCache.previous; - if (previous !== null) { - memoCache.current = [...previous]; + // Setting state during render could leave the cache in a partially filled, + // and therefore inconsistent, state. Reset to the previous value to ensure + // the cache is consistent. + if (previousMemoCache !== null) { + memoCache.data = previousMemoCache.map(data => data.slice()); } else { - console.warn( - 'Expected a previous memo cache instance to have been cached prior to render. This is a bug in React.', - ); - memoCache.current = new Array(memoCache.current.length); + if (__DEV__) { + console.warn( + 'Expected a previous memo cache to exist if the fiber previously called useMemoCache(). This is a bug in React.', + ); + } + memoCache.data.length = 0; } + memoCache.index = 0; } if (__DEV__) { @@ -630,19 +640,17 @@ export function resetHooksAfterThrow(erroredWork: Fiber | null): void { } // The current memo cache may be in an inconsistent state, reset to the previous - // version of the cache. + // version of the cache from the alternate. if (erroredWork != null) { const memoCache = erroredWork.memoCache; if (memoCache !== null) { - if (memoCache.previous === null) { - console.warn( - 'Expected a previous useMemoCache to be present if rendering of a memoized component errored. This is likely a bug in React', - ); + const alternateMemoCache = erroredWork.alternate?.memoCache ?? null; + if (alternateMemoCache !== null) { + memoCache.data = alternateMemoCache.data.map(data => data.slice()); + } else { + memoCache.data.length = 0; } - memoCache.current = - memoCache.previous !== null - ? [...memoCache.previous] - : new Array(memoCache.current.length); + memoCache.index = 0; } } @@ -828,11 +836,22 @@ function useMemoCache(size: number): Array { let memoCache = currentlyRenderingFiber.memoCache; if (memoCache === null) { memoCache = currentlyRenderingFiber.memoCache = { - current: new Array(size), - previous: null, + data: [], + index: 0, }; } - return memoCache.current; + let data = memoCache.data[memoCache.index]; + if (data === undefined) { + data = memoCache.data[memoCache.index] = new Array(size); + } else if (data.length !== size) { + if (__DEV__) { + console.warn( + 'Expected each useMemoCache to receive a consistent size argument, found different sizes', + ); + } + } + memoCache.index++; + return data; } function basicStateReducer(state: S, action: BasicStateAction): S { diff --git a/packages/react-reconciler/src/ReactFiberHooks.old.js b/packages/react-reconciler/src/ReactFiberHooks.old.js index 5495a2992b787..b0334040d23a2 100644 --- a/packages/react-reconciler/src/ReactFiberHooks.old.js +++ b/packages/react-reconciler/src/ReactFiberHooks.old.js @@ -399,6 +399,20 @@ export function renderWithHooks( current !== null && current.type !== workInProgress.type; } + const memoCache = workInProgress.memoCache; + let previousMemoCache = null; + if (memoCache !== null) { + // Clone the cache to allow resetting to the most recent version in case of a setstate during render + previousMemoCache = memoCache.data.map(data => data.slice()); + if (workInProgress.alternate != null) { + workInProgress.alternate.memoCache = { + data: previousMemoCache, + index: 0, + }; + } + memoCache.index = 0; + } + workInProgress.memoizedState = null; workInProgress.updateQueue = null; workInProgress.lanes = NoLanes; @@ -470,6 +484,23 @@ export function renderWithHooks( workInProgress.updateQueue = null; + if (memoCache !== null) { + // Setting state during render could leave the cache in a partially filled, + // and therefore inconsistent, state. Reset to the previous value to ensure + // the cache is consistent. + if (previousMemoCache !== null) { + memoCache.data = previousMemoCache.map(data => data.slice()); + } else { + if (__DEV__) { + console.warn( + 'Expected a previous memo cache to exist if the fiber previously called useMemoCache(). This is a bug in React.', + ); + } + memoCache.data.length = 0; + } + memoCache.index = 0; + } + if (__DEV__) { // Also validate hook order for cascading updates. hookTypesUpdateIndexDev = -1; @@ -583,7 +614,7 @@ export function bailoutHooks( current.lanes = removeLanes(current.lanes, lanes); } -export function resetHooksAfterThrow(): void { +export function resetHooksAfterThrow(erroredWork: Fiber | null): void { // We can assume the previous dispatcher is always this one, since we set it // at the beginning of the render phase and there's no re-entrance. ReactCurrentDispatcher.current = ContextOnlyDispatcher; @@ -608,6 +639,21 @@ export function resetHooksAfterThrow(): void { didScheduleRenderPhaseUpdate = false; } + // The current memo cache may be in an inconsistent state, reset to the previous + // version of the cache from the alternate. + if (erroredWork != null) { + const memoCache = erroredWork.memoCache; + if (memoCache !== null) { + const alternateMemoCache = erroredWork.alternate?.memoCache ?? null; + if (alternateMemoCache !== null) { + memoCache.data = alternateMemoCache.data.map(data => data.slice()); + } else { + memoCache.data.length = 0; + } + memoCache.index = 0; + } + } + renderLanes = NoLanes; currentlyRenderingFiber = (null: any); @@ -787,7 +833,25 @@ function use(usable: Usable): T { } function useMemoCache(size: number): Array { - throw new Error('Not implemented.'); + let memoCache = currentlyRenderingFiber.memoCache; + if (memoCache === null) { + memoCache = currentlyRenderingFiber.memoCache = { + data: [], + index: 0, + }; + } + let data = memoCache.data[memoCache.index]; + if (data === undefined) { + data = memoCache.data[memoCache.index] = new Array(size); + } else if (data.length !== size) { + if (__DEV__) { + console.warn( + 'Expected each useMemoCache to receive a consistent size argument, found different sizes', + ); + } + } + memoCache.index++; + return data; } function basicStateReducer(state: S, action: BasicStateAction): S { diff --git a/packages/react-reconciler/src/ReactFiberWorkLoop.new.js b/packages/react-reconciler/src/ReactFiberWorkLoop.new.js index 9227c2cbded78..612265ab75cb3 100644 --- a/packages/react-reconciler/src/ReactFiberWorkLoop.new.js +++ b/packages/react-reconciler/src/ReactFiberWorkLoop.new.js @@ -3347,7 +3347,7 @@ if (__DEV__ && replayFailedUnitOfWorkWithInvokeGuardedCallback) { // Keep this code in sync with handleThrow; any changes here must have // corresponding changes there. resetContextDependencies(); - resetHooksAfterThrow(current); + resetHooksAfterThrow(unitOfWork); // Don't reset current debug fiber, since we're about to work on the // same fiber again. diff --git a/packages/react-reconciler/src/ReactFiberWorkLoop.old.js b/packages/react-reconciler/src/ReactFiberWorkLoop.old.js index 4715d3fdf4df9..8bb6ef30453b6 100644 --- a/packages/react-reconciler/src/ReactFiberWorkLoop.old.js +++ b/packages/react-reconciler/src/ReactFiberWorkLoop.old.js @@ -1696,7 +1696,7 @@ function prepareFreshStack(root: FiberRoot, lanes: Lanes): Fiber { function handleThrow(root, thrownValue): void { // Reset module-level state that was set during the render phase. resetContextDependencies(); - resetHooksAfterThrow(); + resetHooksAfterThrow(workInProgress); resetCurrentDebugFiberInDEV(); // TODO: I found and added this missing line while investigating a // separate issue. Write a regression test using string refs. @@ -3347,7 +3347,7 @@ if (__DEV__ && replayFailedUnitOfWorkWithInvokeGuardedCallback) { // Keep this code in sync with handleThrow; any changes here must have // corresponding changes there. resetContextDependencies(); - resetHooksAfterThrow(); + resetHooksAfterThrow(unitOfWork); // Don't reset current debug fiber, since we're about to work on the // same fiber again. diff --git a/packages/react-reconciler/src/ReactInternalTypes.js b/packages/react-reconciler/src/ReactInternalTypes.js index 76838182cbfa3..77d640feb84f3 100644 --- a/packages/react-reconciler/src/ReactInternalTypes.js +++ b/packages/react-reconciler/src/ReactInternalTypes.js @@ -69,8 +69,8 @@ export type Dependencies = { }; export type MemoCache = { - current: Array, - previous: Array | null, + data: Array>, + index: number, }; // A Fiber is work on a Component that needs to be done or was done. There can diff --git a/packages/react-reconciler/src/__tests__/useMemoCache-test.js b/packages/react-reconciler/src/__tests__/useMemoCache-test.js index 2b9c229d27f41..6c57d4ad6f215 100644 --- a/packages/react-reconciler/src/__tests__/useMemoCache-test.js +++ b/packages/react-reconciler/src/__tests__/useMemoCache-test.js @@ -1,14 +1,6 @@ let React; let ReactNoop; -let Cache; -let getCacheSignal; -let getCacheForType; -let Scheduler; let act; -let Suspense; -let Offscreen; -let useCacheRefresh; -let startTransition; let useState; let useMemoCache; let ErrorBoundary; @@ -19,15 +11,7 @@ describe('ReactCache', () => { React = require('react'); ReactNoop = require('react-noop-renderer'); - Cache = React.unstable_Cache; - Scheduler = require('scheduler'); act = require('jest-react').act; - Suspense = React.Suspense; - Offscreen = React.unstable_Offscreen; - getCacheSignal = React.unstable_getCacheSignal; - getCacheForType = React.unstable_getCacheForType; - useCacheRefresh = React.unstable_useCacheRefresh; - startTransition = React.startTransition; useState = React.useState; useMemoCache = React.unstable_useMemoCache; @@ -87,7 +71,7 @@ describe('ReactCache', () => { // n is passed as-is to the child as a cache breaker const [n, setN] = useState(0); - forceUpdate = () => setN(n => n + 1); + forceUpdate = () => setN(a => a + 1); const c_n = n !== cache[1]; cache[1] = n; @@ -115,7 +99,7 @@ describe('ReactCache', () => { }); expect(root).toMatchRenderedOutput('Count 0'); expect(Text).toBeCalledTimes(1); - let data0 = data; + const data0 = data; // Changing x should reset the data object await act(async () => { @@ -191,7 +175,7 @@ describe('ReactCache', () => { }); expect(root).toMatchRenderedOutput('Count 0'); expect(Text).toBeCalledTimes(1); - let data0 = data; + const data0 = data; // Simultaneously trigger an update to x (should create a new data value) // and trigger the setState+early return. The runtime should reset the cache @@ -278,7 +262,7 @@ describe('ReactCache', () => { }); expect(root).toMatchRenderedOutput('Count 0'); expect(Text).toBeCalledTimes(1); - let data0 = data; + const data0 = data; // Simultaneously trigger an update to x (should create a new data value) // and trigger the setState+early return. The runtime should reset the cache @@ -319,7 +303,7 @@ describe('ReactCache', () => { // n is passed as-is to the child as a cache breaker const [n, setN] = useState(0); - forceUpdate = () => setN(n => n + 1); + forceUpdate = () => setN(a => a + 1); const c_n = n !== cache[1]; cache[1] = n; @@ -346,7 +330,7 @@ describe('ReactCache', () => { } else { nextData = cache[1]; } - return cache[1]; + return nextData; } let data; const Text = jest.fn(function Text(props) { @@ -360,7 +344,7 @@ describe('ReactCache', () => { }); expect(root).toMatchRenderedOutput('count 0'); expect(Text).toBeCalledTimes(1); - let data0 = data; + const data0 = data; // Changing x should reset the data object await act(async () => { From 120973c0682b06d11ead65a30019c3796454ab5e Mon Sep 17 00:00:00 2001 From: Joe Savona Date: Mon, 29 Aug 2022 14:05:52 -0700 Subject: [PATCH 04/12] code cleanup --- packages/react-reconciler/src/ReactFiber.new.js | 11 +---------- packages/react-reconciler/src/ReactFiber.old.js | 11 +---------- .../react-reconciler/src/ReactFiberHooks.new.js | 16 ++++++++++------ .../react-reconciler/src/ReactFiberHooks.old.js | 16 ++++++++++------ 4 files changed, 22 insertions(+), 32 deletions(-) diff --git a/packages/react-reconciler/src/ReactFiber.new.js b/packages/react-reconciler/src/ReactFiber.new.js index e9daf4bdf245d..a34d0fd2cff06 100644 --- a/packages/react-reconciler/src/ReactFiber.new.js +++ b/packages/react-reconciler/src/ReactFiber.new.js @@ -311,16 +311,7 @@ export function createWorkInProgress(current: Fiber, pendingProps: any): Fiber { workInProgress.memoizedProps = current.memoizedProps; workInProgress.memoizedState = current.memoizedState; workInProgress.updateQueue = current.updateQueue; - - const memoCache = current.memoCache; - if (memoCache !== null) { - workInProgress.memoCache = { - data: memoCache.data.map(data => data.slice()), - index: 0, - }; - } else { - workInProgress.memoCache = null; - } + workInProgress.memoCache = current.memoCache; // Clone the dependencies object. This is mutated during the render phase, so // it cannot be shared with the current fiber. diff --git a/packages/react-reconciler/src/ReactFiber.old.js b/packages/react-reconciler/src/ReactFiber.old.js index 5fe41257c6d90..d4f49333b194d 100644 --- a/packages/react-reconciler/src/ReactFiber.old.js +++ b/packages/react-reconciler/src/ReactFiber.old.js @@ -311,16 +311,7 @@ export function createWorkInProgress(current: Fiber, pendingProps: any): Fiber { workInProgress.memoizedProps = current.memoizedProps; workInProgress.memoizedState = current.memoizedState; workInProgress.updateQueue = current.updateQueue; - - const memoCache = current.memoCache; - if (memoCache !== null) { - workInProgress.memoCache = { - data: memoCache.data.map(data => data.slice()), - index: 0, - }; - } else { - workInProgress.memoCache = null; - } + workInProgress.memoCache = current.memoCache; // Clone the dependencies object. This is mutated during the render phase, so // it cannot be shared with the current fiber. diff --git a/packages/react-reconciler/src/ReactFiberHooks.new.js b/packages/react-reconciler/src/ReactFiberHooks.new.js index 7a28a750aea00..1a915223b396b 100644 --- a/packages/react-reconciler/src/ReactFiberHooks.new.js +++ b/packages/react-reconciler/src/ReactFiberHooks.new.js @@ -399,12 +399,14 @@ export function renderWithHooks( current !== null && current.type !== workInProgress.type; } + // Reset the memoCache index and create a backup copy in case of a setState during render + // or error, either of which can leave the cache in an inconsistent state const memoCache = workInProgress.memoCache; let previousMemoCache = null; if (memoCache !== null) { - // Clone the cache to allow resetting to the most recent version in case of a setstate during render - previousMemoCache = memoCache.data.map(data => data.slice()); + previousMemoCache = memoCache.data.map(array => array.slice()); if (workInProgress.alternate != null) { + // Backup to the alternate fiber in case the component throws workInProgress.alternate.memoCache = { data: previousMemoCache, index: 0, @@ -485,9 +487,8 @@ export function renderWithHooks( workInProgress.updateQueue = null; if (memoCache !== null) { - // Setting state during render could leave the cache in a partially filled, - // and therefore inconsistent, state. Reset to the previous value to ensure - // the cache is consistent. + // Setting state during render could leave the cache in an inconsistent state, + // reset to the previous state before re-rendering. if (previousMemoCache !== null) { memoCache.data = previousMemoCache.map(data => data.slice()); } else { @@ -644,10 +645,13 @@ export function resetHooksAfterThrow(erroredWork: Fiber | null): void { if (erroredWork != null) { const memoCache = erroredWork.memoCache; if (memoCache !== null) { + // Unless this is the first render of a component, the alternate will have a + // consistent view of the memo cache that we can restore to const alternateMemoCache = erroredWork.alternate?.memoCache ?? null; if (alternateMemoCache !== null) { - memoCache.data = alternateMemoCache.data.map(data => data.slice()); + memoCache.data = alternateMemoCache.data.map(array => array.slice()); } else { + // Just in case, fall back to clearing the memo cache memoCache.data.length = 0; } memoCache.index = 0; diff --git a/packages/react-reconciler/src/ReactFiberHooks.old.js b/packages/react-reconciler/src/ReactFiberHooks.old.js index b0334040d23a2..09d0ee50ac0ad 100644 --- a/packages/react-reconciler/src/ReactFiberHooks.old.js +++ b/packages/react-reconciler/src/ReactFiberHooks.old.js @@ -399,12 +399,14 @@ export function renderWithHooks( current !== null && current.type !== workInProgress.type; } + // Reset the memoCache index and create a backup copy in case of a setState during render + // or error, either of which can leave the cache in an inconsistent state const memoCache = workInProgress.memoCache; let previousMemoCache = null; if (memoCache !== null) { - // Clone the cache to allow resetting to the most recent version in case of a setstate during render - previousMemoCache = memoCache.data.map(data => data.slice()); + previousMemoCache = memoCache.data.map(array => array.slice()); if (workInProgress.alternate != null) { + // Backup to the alternate fiber in case the component throws workInProgress.alternate.memoCache = { data: previousMemoCache, index: 0, @@ -485,9 +487,8 @@ export function renderWithHooks( workInProgress.updateQueue = null; if (memoCache !== null) { - // Setting state during render could leave the cache in a partially filled, - // and therefore inconsistent, state. Reset to the previous value to ensure - // the cache is consistent. + // Setting state during render could leave the cache in an inconsistent state, + // reset to the previous state before re-rendering. if (previousMemoCache !== null) { memoCache.data = previousMemoCache.map(data => data.slice()); } else { @@ -644,10 +645,13 @@ export function resetHooksAfterThrow(erroredWork: Fiber | null): void { if (erroredWork != null) { const memoCache = erroredWork.memoCache; if (memoCache !== null) { + // Unless this is the first render of a component, the alternate will have a + // consistent view of the memo cache that we can restore to const alternateMemoCache = erroredWork.alternate?.memoCache ?? null; if (alternateMemoCache !== null) { - memoCache.data = alternateMemoCache.data.map(data => data.slice()); + memoCache.data = alternateMemoCache.data.map(array => array.slice()); } else { + // Just in case, fall back to clearing the memo cache memoCache.data.length = 0; } memoCache.index = 0; From 202208e3d71eea40dcf1d59637a7c5c45460ffc4 Mon Sep 17 00:00:00 2001 From: Joe Savona Date: Mon, 29 Aug 2022 15:15:33 -0700 Subject: [PATCH 05/12] fix internal test --- packages/react-reconciler/src/ReactFiber.new.js | 1 + packages/react-reconciler/src/ReactFiber.old.js | 1 + 2 files changed, 2 insertions(+) diff --git a/packages/react-reconciler/src/ReactFiber.new.js b/packages/react-reconciler/src/ReactFiber.new.js index a34d0fd2cff06..1db9edeaa1307 100644 --- a/packages/react-reconciler/src/ReactFiber.new.js +++ b/packages/react-reconciler/src/ReactFiber.new.js @@ -853,6 +853,7 @@ export function assignFiberPropertiesInDEV( target.pendingProps = source.pendingProps; target.memoizedProps = source.memoizedProps; target.updateQueue = source.updateQueue; + target.memoCache = source.memoCache; target.memoizedState = source.memoizedState; target.dependencies = source.dependencies; target.mode = source.mode; diff --git a/packages/react-reconciler/src/ReactFiber.old.js b/packages/react-reconciler/src/ReactFiber.old.js index d4f49333b194d..4a367635a02b0 100644 --- a/packages/react-reconciler/src/ReactFiber.old.js +++ b/packages/react-reconciler/src/ReactFiber.old.js @@ -853,6 +853,7 @@ export function assignFiberPropertiesInDEV( target.pendingProps = source.pendingProps; target.memoizedProps = source.memoizedProps; target.updateQueue = source.updateQueue; + target.memoCache = source.memoCache; target.memoizedState = source.memoizedState; target.dependencies = source.dependencies; target.mode = source.mode; From 8bcf523ecc667775a700e03851a6c5d2b379648b Mon Sep 17 00:00:00 2001 From: Joe Savona Date: Mon, 29 Aug 2022 15:23:56 -0700 Subject: [PATCH 06/12] oops we do not support nullable property access --- packages/react-reconciler/src/ReactFiberHooks.new.js | 3 ++- packages/react-reconciler/src/ReactFiberHooks.old.js | 3 ++- 2 files changed, 4 insertions(+), 2 deletions(-) diff --git a/packages/react-reconciler/src/ReactFiberHooks.new.js b/packages/react-reconciler/src/ReactFiberHooks.new.js index 1a915223b396b..162125762c071 100644 --- a/packages/react-reconciler/src/ReactFiberHooks.new.js +++ b/packages/react-reconciler/src/ReactFiberHooks.new.js @@ -647,7 +647,8 @@ export function resetHooksAfterThrow(erroredWork: Fiber | null): void { if (memoCache !== null) { // Unless this is the first render of a component, the alternate will have a // consistent view of the memo cache that we can restore to - const alternateMemoCache = erroredWork.alternate?.memoCache ?? null; + const alternate = erroredWork.alternate; + const alternateMemoCache = alternate != null ? alternate.memoCache : null; if (alternateMemoCache !== null) { memoCache.data = alternateMemoCache.data.map(array => array.slice()); } else { diff --git a/packages/react-reconciler/src/ReactFiberHooks.old.js b/packages/react-reconciler/src/ReactFiberHooks.old.js index 09d0ee50ac0ad..519486bd3ec05 100644 --- a/packages/react-reconciler/src/ReactFiberHooks.old.js +++ b/packages/react-reconciler/src/ReactFiberHooks.old.js @@ -647,7 +647,8 @@ export function resetHooksAfterThrow(erroredWork: Fiber | null): void { if (memoCache !== null) { // Unless this is the first render of a component, the alternate will have a // consistent view of the memo cache that we can restore to - const alternateMemoCache = erroredWork.alternate?.memoCache ?? null; + const alternate = erroredWork.alternate; + const alternateMemoCache = alternate != null ? alternate.memoCache : null; if (alternateMemoCache !== null) { memoCache.data = alternateMemoCache.data.map(array => array.slice()); } else { From 2900e4dd8f9fcf8ab491feaa2b7d2ebebd01bf5a Mon Sep 17 00:00:00 2001 From: Joe Savona Date: Wed, 31 Aug 2022 16:20:09 -0700 Subject: [PATCH 07/12] Simplify implementation, still have questions on some of the PR feedback though --- .../src/ReactFiberHooks.new.js | 56 +++++++------------ .../src/ReactFiberHooks.old.js | 56 +++++++------------ 2 files changed, 42 insertions(+), 70 deletions(-) diff --git a/packages/react-reconciler/src/ReactFiberHooks.new.js b/packages/react-reconciler/src/ReactFiberHooks.new.js index 162125762c071..0472a7088c0dc 100644 --- a/packages/react-reconciler/src/ReactFiberHooks.new.js +++ b/packages/react-reconciler/src/ReactFiberHooks.new.js @@ -401,18 +401,16 @@ export function renderWithHooks( // Reset the memoCache index and create a backup copy in case of a setState during render // or error, either of which can leave the cache in an inconsistent state - const memoCache = workInProgress.memoCache; let previousMemoCache = null; - if (memoCache !== null) { - previousMemoCache = memoCache.data.map(array => array.slice()); - if (workInProgress.alternate != null) { - // Backup to the alternate fiber in case the component throws - workInProgress.alternate.memoCache = { - data: previousMemoCache, + if (enableUseMemoCacheHook) { + previousMemoCache = workInProgress.memoCache; + if (previousMemoCache !== null) { + const memoCache = { + data: previousMemoCache.data.map(array => array.slice()), index: 0, }; + workInProgress.memoCache = memoCache; } - memoCache.index = 0; } workInProgress.memoizedState = null; @@ -486,20 +484,15 @@ export function renderWithHooks( workInProgress.updateQueue = null; - if (memoCache !== null) { - // Setting state during render could leave the cache in an inconsistent state, - // reset to the previous state before re-rendering. + if (enableUseMemoCacheHook) { if (previousMemoCache !== null) { - memoCache.data = previousMemoCache.map(data => data.slice()); - } else { - if (__DEV__) { - console.warn( - 'Expected a previous memo cache to exist if the fiber previously called useMemoCache(). This is a bug in React.', - ); - } - memoCache.data.length = 0; + // Setting state during render could leave the cache in an inconsistent state, + // reset to the previous state before re-rendering. + workInProgress.memoCache = { + data: previousMemoCache.data.map(array => array.slice()), + index: 0, + }; } - memoCache.index = 0; } if (__DEV__) { @@ -640,22 +633,15 @@ export function resetHooksAfterThrow(erroredWork: Fiber | null): void { didScheduleRenderPhaseUpdate = false; } - // The current memo cache may be in an inconsistent state, reset to the previous - // version of the cache from the alternate. - if (erroredWork != null) { - const memoCache = erroredWork.memoCache; - if (memoCache !== null) { - // Unless this is the first render of a component, the alternate will have a - // consistent view of the memo cache that we can restore to - const alternate = erroredWork.alternate; - const alternateMemoCache = alternate != null ? alternate.memoCache : null; - if (alternateMemoCache !== null) { - memoCache.data = alternateMemoCache.data.map(array => array.slice()); - } else { - // Just in case, fall back to clearing the memo cache - memoCache.data.length = 0; + if (enableUseMemoCacheHook) { + if (erroredWork != null) { + const memoCache = erroredWork.memoCache; + if (memoCache !== null) { + // The memo cache may be in an inconsistent state, reset to the version from + // the alternate if available, or clear the cache completely. + const alternate = erroredWork.alternate; + erroredWork.memoCache = alternate != null ? alternate.memoCache : null; } - memoCache.index = 0; } } diff --git a/packages/react-reconciler/src/ReactFiberHooks.old.js b/packages/react-reconciler/src/ReactFiberHooks.old.js index 519486bd3ec05..a9f88c468916d 100644 --- a/packages/react-reconciler/src/ReactFiberHooks.old.js +++ b/packages/react-reconciler/src/ReactFiberHooks.old.js @@ -401,18 +401,16 @@ export function renderWithHooks( // Reset the memoCache index and create a backup copy in case of a setState during render // or error, either of which can leave the cache in an inconsistent state - const memoCache = workInProgress.memoCache; let previousMemoCache = null; - if (memoCache !== null) { - previousMemoCache = memoCache.data.map(array => array.slice()); - if (workInProgress.alternate != null) { - // Backup to the alternate fiber in case the component throws - workInProgress.alternate.memoCache = { - data: previousMemoCache, + if (enableUseMemoCacheHook) { + previousMemoCache = workInProgress.memoCache; + if (previousMemoCache !== null) { + const memoCache = { + data: previousMemoCache.data.map(array => array.slice()), index: 0, }; + workInProgress.memoCache = memoCache; } - memoCache.index = 0; } workInProgress.memoizedState = null; @@ -486,20 +484,15 @@ export function renderWithHooks( workInProgress.updateQueue = null; - if (memoCache !== null) { - // Setting state during render could leave the cache in an inconsistent state, - // reset to the previous state before re-rendering. + if (enableUseMemoCacheHook) { if (previousMemoCache !== null) { - memoCache.data = previousMemoCache.map(data => data.slice()); - } else { - if (__DEV__) { - console.warn( - 'Expected a previous memo cache to exist if the fiber previously called useMemoCache(). This is a bug in React.', - ); - } - memoCache.data.length = 0; + // Setting state during render could leave the cache in an inconsistent state, + // reset to the previous state before re-rendering. + workInProgress.memoCache = { + data: previousMemoCache.data.map(array => array.slice()), + index: 0, + }; } - memoCache.index = 0; } if (__DEV__) { @@ -640,22 +633,15 @@ export function resetHooksAfterThrow(erroredWork: Fiber | null): void { didScheduleRenderPhaseUpdate = false; } - // The current memo cache may be in an inconsistent state, reset to the previous - // version of the cache from the alternate. - if (erroredWork != null) { - const memoCache = erroredWork.memoCache; - if (memoCache !== null) { - // Unless this is the first render of a component, the alternate will have a - // consistent view of the memo cache that we can restore to - const alternate = erroredWork.alternate; - const alternateMemoCache = alternate != null ? alternate.memoCache : null; - if (alternateMemoCache !== null) { - memoCache.data = alternateMemoCache.data.map(array => array.slice()); - } else { - // Just in case, fall back to clearing the memo cache - memoCache.data.length = 0; + if (enableUseMemoCacheHook) { + if (erroredWork != null) { + const memoCache = erroredWork.memoCache; + if (memoCache !== null) { + // The memo cache may be in an inconsistent state, reset to the version from + // the alternate if available, or clear the cache completely. + const alternate = erroredWork.alternate; + erroredWork.memoCache = alternate != null ? alternate.memoCache : null; } - memoCache.index = 0; } } From c20c3a3509cba27646c079d60c817e9dda49270e Mon Sep 17 00:00:00 2001 From: Joe Savona Date: Wed, 31 Aug 2022 16:27:00 -0700 Subject: [PATCH 08/12] Gate all code based on the feature flag --- packages/react-reconciler/src/ReactFiber.new.js | 13 ++++++++++--- packages/react-reconciler/src/ReactFiber.old.js | 13 ++++++++++--- 2 files changed, 20 insertions(+), 6 deletions(-) diff --git a/packages/react-reconciler/src/ReactFiber.new.js b/packages/react-reconciler/src/ReactFiber.new.js index 1db9edeaa1307..08046b3c384ac 100644 --- a/packages/react-reconciler/src/ReactFiber.new.js +++ b/packages/react-reconciler/src/ReactFiber.new.js @@ -32,6 +32,7 @@ import { allowConcurrentByDefault, enableTransitionTracing, enableDebugTracing, + enableUseMemoCacheHook, } from 'shared/ReactFeatureFlags'; import {NoFlags, Placement, StaticMask} from './ReactFiberFlags'; import {ConcurrentRoot} from './ReactRootTags'; @@ -144,7 +145,9 @@ function FiberNode( this.updateQueue = null; this.memoizedState = null; this.dependencies = null; - this.memoCache = null; + if (enableUseMemoCacheHook) { + this.memoCache = null; + } this.mode = mode; @@ -311,7 +314,9 @@ export function createWorkInProgress(current: Fiber, pendingProps: any): Fiber { workInProgress.memoizedProps = current.memoizedProps; workInProgress.memoizedState = current.memoizedState; workInProgress.updateQueue = current.updateQueue; - workInProgress.memoCache = current.memoCache; + if (enableUseMemoCacheHook) { + workInProgress.memoCache = current.memoCache; + } // Clone the dependencies object. This is mutated during the render phase, so // it cannot be shared with the current fiber. @@ -853,7 +858,9 @@ export function assignFiberPropertiesInDEV( target.pendingProps = source.pendingProps; target.memoizedProps = source.memoizedProps; target.updateQueue = source.updateQueue; - target.memoCache = source.memoCache; + if (enableUseMemoCacheHook) { + target.memoCache = source.memoCache; + } target.memoizedState = source.memoizedState; target.dependencies = source.dependencies; target.mode = source.mode; diff --git a/packages/react-reconciler/src/ReactFiber.old.js b/packages/react-reconciler/src/ReactFiber.old.js index 4a367635a02b0..ce8322f3cb5f1 100644 --- a/packages/react-reconciler/src/ReactFiber.old.js +++ b/packages/react-reconciler/src/ReactFiber.old.js @@ -32,6 +32,7 @@ import { allowConcurrentByDefault, enableTransitionTracing, enableDebugTracing, + enableUseMemoCacheHook, } from 'shared/ReactFeatureFlags'; import {NoFlags, Placement, StaticMask} from './ReactFiberFlags'; import {ConcurrentRoot} from './ReactRootTags'; @@ -144,7 +145,9 @@ function FiberNode( this.updateQueue = null; this.memoizedState = null; this.dependencies = null; - this.memoCache = null; + if (enableUseMemoCacheHook) { + this.memoCache = null; + } this.mode = mode; @@ -311,7 +314,9 @@ export function createWorkInProgress(current: Fiber, pendingProps: any): Fiber { workInProgress.memoizedProps = current.memoizedProps; workInProgress.memoizedState = current.memoizedState; workInProgress.updateQueue = current.updateQueue; - workInProgress.memoCache = current.memoCache; + if (enableUseMemoCacheHook) { + workInProgress.memoCache = current.memoCache; + } // Clone the dependencies object. This is mutated during the render phase, so // it cannot be shared with the current fiber. @@ -853,7 +858,9 @@ export function assignFiberPropertiesInDEV( target.pendingProps = source.pendingProps; target.memoizedProps = source.memoizedProps; target.updateQueue = source.updateQueue; - target.memoCache = source.memoCache; + if (enableUseMemoCacheHook) { + target.memoCache = source.memoCache; + } target.memoizedState = source.memoizedState; target.dependencies = source.dependencies; target.mode = source.mode; From 90dd08eb20a94219025c40f3dd1fcfbd0d13e204 Mon Sep 17 00:00:00 2001 From: Joe Savona Date: Thu, 8 Sep 2022 16:49:37 -0700 Subject: [PATCH 09/12] refactor to use updateQueue --- .../react-reconciler/src/ReactFiber.new.js | 10 --- .../react-reconciler/src/ReactFiber.old.js | 10 --- .../src/ReactFiberHooks.new.js | 88 +++++++++---------- .../src/ReactFiberHooks.old.js | 88 +++++++++---------- .../src/ReactFiberWorkLoop.new.js | 4 +- .../src/ReactFiberWorkLoop.old.js | 4 +- .../src/ReactInternalTypes.js | 3 - .../src/__tests__/useMemoCache-test.js | 2 +- 8 files changed, 89 insertions(+), 120 deletions(-) diff --git a/packages/react-reconciler/src/ReactFiber.new.js b/packages/react-reconciler/src/ReactFiber.new.js index 08046b3c384ac..d6a6c8f7adc19 100644 --- a/packages/react-reconciler/src/ReactFiber.new.js +++ b/packages/react-reconciler/src/ReactFiber.new.js @@ -32,7 +32,6 @@ import { allowConcurrentByDefault, enableTransitionTracing, enableDebugTracing, - enableUseMemoCacheHook, } from 'shared/ReactFeatureFlags'; import {NoFlags, Placement, StaticMask} from './ReactFiberFlags'; import {ConcurrentRoot} from './ReactRootTags'; @@ -145,9 +144,6 @@ function FiberNode( this.updateQueue = null; this.memoizedState = null; this.dependencies = null; - if (enableUseMemoCacheHook) { - this.memoCache = null; - } this.mode = mode; @@ -314,9 +310,6 @@ export function createWorkInProgress(current: Fiber, pendingProps: any): Fiber { workInProgress.memoizedProps = current.memoizedProps; workInProgress.memoizedState = current.memoizedState; workInProgress.updateQueue = current.updateQueue; - if (enableUseMemoCacheHook) { - workInProgress.memoCache = current.memoCache; - } // Clone the dependencies object. This is mutated during the render phase, so // it cannot be shared with the current fiber. @@ -858,9 +851,6 @@ export function assignFiberPropertiesInDEV( target.pendingProps = source.pendingProps; target.memoizedProps = source.memoizedProps; target.updateQueue = source.updateQueue; - if (enableUseMemoCacheHook) { - target.memoCache = source.memoCache; - } target.memoizedState = source.memoizedState; target.dependencies = source.dependencies; target.mode = source.mode; diff --git a/packages/react-reconciler/src/ReactFiber.old.js b/packages/react-reconciler/src/ReactFiber.old.js index ce8322f3cb5f1..d8f1ef424bd13 100644 --- a/packages/react-reconciler/src/ReactFiber.old.js +++ b/packages/react-reconciler/src/ReactFiber.old.js @@ -32,7 +32,6 @@ import { allowConcurrentByDefault, enableTransitionTracing, enableDebugTracing, - enableUseMemoCacheHook, } from 'shared/ReactFeatureFlags'; import {NoFlags, Placement, StaticMask} from './ReactFiberFlags'; import {ConcurrentRoot} from './ReactRootTags'; @@ -145,9 +144,6 @@ function FiberNode( this.updateQueue = null; this.memoizedState = null; this.dependencies = null; - if (enableUseMemoCacheHook) { - this.memoCache = null; - } this.mode = mode; @@ -314,9 +310,6 @@ export function createWorkInProgress(current: Fiber, pendingProps: any): Fiber { workInProgress.memoizedProps = current.memoizedProps; workInProgress.memoizedState = current.memoizedState; workInProgress.updateQueue = current.updateQueue; - if (enableUseMemoCacheHook) { - workInProgress.memoCache = current.memoCache; - } // Clone the dependencies object. This is mutated during the render phase, so // it cannot be shared with the current fiber. @@ -858,9 +851,6 @@ export function assignFiberPropertiesInDEV( target.pendingProps = source.pendingProps; target.memoizedProps = source.memoizedProps; target.updateQueue = source.updateQueue; - if (enableUseMemoCacheHook) { - target.memoCache = source.memoCache; - } target.memoizedState = source.memoizedState; target.dependencies = source.dependencies; target.mode = source.mode; diff --git a/packages/react-reconciler/src/ReactFiberHooks.new.js b/packages/react-reconciler/src/ReactFiberHooks.new.js index 0472a7088c0dc..292323bdfc1aa 100644 --- a/packages/react-reconciler/src/ReactFiberHooks.new.js +++ b/packages/react-reconciler/src/ReactFiberHooks.new.js @@ -16,7 +16,12 @@ import type { Usable, Thenable, } from 'shared/ReactTypes'; -import type {Fiber, Dispatcher, HookType} from './ReactInternalTypes'; +import type { + Fiber, + Dispatcher, + HookType, + MemoCache, +} from './ReactInternalTypes'; import type {Lanes, Lane} from './ReactFiberLane.new'; import type {HookFlags} from './ReactHookEffectTags'; import type {FiberRoot} from './ReactInternalTypes'; @@ -177,6 +182,7 @@ type StoreConsistencyCheck = { export type FunctionComponentUpdateQueue = { lastEffect: Effect | null, stores: Array> | null, + memoCache: MemoCache | null, }; type BasicStateAction = (S => S) | S; @@ -399,20 +405,6 @@ export function renderWithHooks( current !== null && current.type !== workInProgress.type; } - // Reset the memoCache index and create a backup copy in case of a setState during render - // or error, either of which can leave the cache in an inconsistent state - let previousMemoCache = null; - if (enableUseMemoCacheHook) { - previousMemoCache = workInProgress.memoCache; - if (previousMemoCache !== null) { - const memoCache = { - data: previousMemoCache.data.map(array => array.slice()), - index: 0, - }; - workInProgress.memoCache = memoCache; - } - } - workInProgress.memoizedState = null; workInProgress.updateQueue = null; workInProgress.lanes = NoLanes; @@ -484,17 +476,6 @@ export function renderWithHooks( workInProgress.updateQueue = null; - if (enableUseMemoCacheHook) { - if (previousMemoCache !== null) { - // Setting state during render could leave the cache in an inconsistent state, - // reset to the previous state before re-rendering. - workInProgress.memoCache = { - data: previousMemoCache.data.map(array => array.slice()), - index: 0, - }; - } - } - if (__DEV__) { // Also validate hook order for cascading updates. hookTypesUpdateIndexDev = -1; @@ -608,7 +589,7 @@ export function bailoutHooks( current.lanes = removeLanes(current.lanes, lanes); } -export function resetHooksAfterThrow(erroredWork: Fiber | null): void { +export function resetHooksAfterThrow(): void { // We can assume the previous dispatcher is always this one, since we set it // at the beginning of the render phase and there's no re-entrance. ReactCurrentDispatcher.current = ContextOnlyDispatcher; @@ -633,18 +614,6 @@ export function resetHooksAfterThrow(erroredWork: Fiber | null): void { didScheduleRenderPhaseUpdate = false; } - if (enableUseMemoCacheHook) { - if (erroredWork != null) { - const memoCache = erroredWork.memoCache; - if (memoCache !== null) { - // The memo cache may be in an inconsistent state, reset to the version from - // the alternate if available, or clear the cache completely. - const alternate = erroredWork.alternate; - erroredWork.memoCache = alternate != null ? alternate.memoCache : null; - } - } - } - renderLanes = NoLanes; currentlyRenderingFiber = (null: any); @@ -751,6 +720,7 @@ function createFunctionComponentUpdateQueue(): FunctionComponentUpdateQueue { return { lastEffect: null, stores: null, + memoCache: null, }; } @@ -824,22 +794,48 @@ function use(usable: Usable): T { } function useMemoCache(size: number): Array { - let memoCache = currentlyRenderingFiber.memoCache; + let memoCache = null; + // Fast-path, load memo cache from wip fiber if already prepared + 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 = currentlyRenderingFiber.memoCache = { + memoCache = { data: [], index: 0, }; } + if (updateQueue === null) { + updateQueue = createFunctionComponentUpdateQueue(); + currentlyRenderingFiber.updateQueue = updateQueue; + } + updateQueue.memoCache = memoCache; + let data = memoCache.data[memoCache.index]; if (data === undefined) { data = memoCache.data[memoCache.index] = new Array(size); } else if (data.length !== size) { - if (__DEV__) { - console.warn( - 'Expected each useMemoCache to receive a consistent size argument, found different sizes', - ); - } + // TODO: consider warning or throwing here } memoCache.index++; return data; diff --git a/packages/react-reconciler/src/ReactFiberHooks.old.js b/packages/react-reconciler/src/ReactFiberHooks.old.js index a9f88c468916d..acb9c9542bf33 100644 --- a/packages/react-reconciler/src/ReactFiberHooks.old.js +++ b/packages/react-reconciler/src/ReactFiberHooks.old.js @@ -16,7 +16,12 @@ import type { Usable, Thenable, } from 'shared/ReactTypes'; -import type {Fiber, Dispatcher, HookType} from './ReactInternalTypes'; +import type { + Fiber, + Dispatcher, + HookType, + MemoCache, +} from './ReactInternalTypes'; import type {Lanes, Lane} from './ReactFiberLane.old'; import type {HookFlags} from './ReactHookEffectTags'; import type {FiberRoot} from './ReactInternalTypes'; @@ -177,6 +182,7 @@ type StoreConsistencyCheck = { export type FunctionComponentUpdateQueue = { lastEffect: Effect | null, stores: Array> | null, + memoCache: MemoCache | null, }; type BasicStateAction = (S => S) | S; @@ -399,20 +405,6 @@ export function renderWithHooks( current !== null && current.type !== workInProgress.type; } - // Reset the memoCache index and create a backup copy in case of a setState during render - // or error, either of which can leave the cache in an inconsistent state - let previousMemoCache = null; - if (enableUseMemoCacheHook) { - previousMemoCache = workInProgress.memoCache; - if (previousMemoCache !== null) { - const memoCache = { - data: previousMemoCache.data.map(array => array.slice()), - index: 0, - }; - workInProgress.memoCache = memoCache; - } - } - workInProgress.memoizedState = null; workInProgress.updateQueue = null; workInProgress.lanes = NoLanes; @@ -484,17 +476,6 @@ export function renderWithHooks( workInProgress.updateQueue = null; - if (enableUseMemoCacheHook) { - if (previousMemoCache !== null) { - // Setting state during render could leave the cache in an inconsistent state, - // reset to the previous state before re-rendering. - workInProgress.memoCache = { - data: previousMemoCache.data.map(array => array.slice()), - index: 0, - }; - } - } - if (__DEV__) { // Also validate hook order for cascading updates. hookTypesUpdateIndexDev = -1; @@ -608,7 +589,7 @@ export function bailoutHooks( current.lanes = removeLanes(current.lanes, lanes); } -export function resetHooksAfterThrow(erroredWork: Fiber | null): void { +export function resetHooksAfterThrow(): void { // We can assume the previous dispatcher is always this one, since we set it // at the beginning of the render phase and there's no re-entrance. ReactCurrentDispatcher.current = ContextOnlyDispatcher; @@ -633,18 +614,6 @@ export function resetHooksAfterThrow(erroredWork: Fiber | null): void { didScheduleRenderPhaseUpdate = false; } - if (enableUseMemoCacheHook) { - if (erroredWork != null) { - const memoCache = erroredWork.memoCache; - if (memoCache !== null) { - // The memo cache may be in an inconsistent state, reset to the version from - // the alternate if available, or clear the cache completely. - const alternate = erroredWork.alternate; - erroredWork.memoCache = alternate != null ? alternate.memoCache : null; - } - } - } - renderLanes = NoLanes; currentlyRenderingFiber = (null: any); @@ -751,6 +720,7 @@ function createFunctionComponentUpdateQueue(): FunctionComponentUpdateQueue { return { lastEffect: null, stores: null, + memoCache: null, }; } @@ -824,22 +794,48 @@ function use(usable: Usable): T { } function useMemoCache(size: number): Array { - let memoCache = currentlyRenderingFiber.memoCache; + let memoCache = null; + // Fast-path, load memo cache from wip fiber if already prepared + 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 = currentlyRenderingFiber.memoCache = { + memoCache = { data: [], index: 0, }; } + if (updateQueue === null) { + updateQueue = createFunctionComponentUpdateQueue(); + currentlyRenderingFiber.updateQueue = updateQueue; + } + updateQueue.memoCache = memoCache; + let data = memoCache.data[memoCache.index]; if (data === undefined) { data = memoCache.data[memoCache.index] = new Array(size); } else if (data.length !== size) { - if (__DEV__) { - console.warn( - 'Expected each useMemoCache to receive a consistent size argument, found different sizes', - ); - } + // TODO: consider warning or throwing here } memoCache.index++; return data; diff --git a/packages/react-reconciler/src/ReactFiberWorkLoop.new.js b/packages/react-reconciler/src/ReactFiberWorkLoop.new.js index 612265ab75cb3..ca6bbcae278ab 100644 --- a/packages/react-reconciler/src/ReactFiberWorkLoop.new.js +++ b/packages/react-reconciler/src/ReactFiberWorkLoop.new.js @@ -1696,7 +1696,7 @@ function prepareFreshStack(root: FiberRoot, lanes: Lanes): Fiber { function handleThrow(root, thrownValue): void { // Reset module-level state that was set during the render phase. resetContextDependencies(); - resetHooksAfterThrow(workInProgress); + resetHooksAfterThrow(); resetCurrentDebugFiberInDEV(); // TODO: I found and added this missing line while investigating a // separate issue. Write a regression test using string refs. @@ -3347,7 +3347,7 @@ if (__DEV__ && replayFailedUnitOfWorkWithInvokeGuardedCallback) { // Keep this code in sync with handleThrow; any changes here must have // corresponding changes there. resetContextDependencies(); - resetHooksAfterThrow(unitOfWork); + resetHooksAfterThrow(); // Don't reset current debug fiber, since we're about to work on the // same fiber again. diff --git a/packages/react-reconciler/src/ReactFiberWorkLoop.old.js b/packages/react-reconciler/src/ReactFiberWorkLoop.old.js index 8bb6ef30453b6..4715d3fdf4df9 100644 --- a/packages/react-reconciler/src/ReactFiberWorkLoop.old.js +++ b/packages/react-reconciler/src/ReactFiberWorkLoop.old.js @@ -1696,7 +1696,7 @@ function prepareFreshStack(root: FiberRoot, lanes: Lanes): Fiber { function handleThrow(root, thrownValue): void { // Reset module-level state that was set during the render phase. resetContextDependencies(); - resetHooksAfterThrow(workInProgress); + resetHooksAfterThrow(); resetCurrentDebugFiberInDEV(); // TODO: I found and added this missing line while investigating a // separate issue. Write a regression test using string refs. @@ -3347,7 +3347,7 @@ if (__DEV__ && replayFailedUnitOfWorkWithInvokeGuardedCallback) { // Keep this code in sync with handleThrow; any changes here must have // corresponding changes there. resetContextDependencies(); - resetHooksAfterThrow(unitOfWork); + resetHooksAfterThrow(); // Don't reset current debug fiber, since we're about to work on the // same fiber again. diff --git a/packages/react-reconciler/src/ReactInternalTypes.js b/packages/react-reconciler/src/ReactInternalTypes.js index 77d640feb84f3..004aa17e56ad8 100644 --- a/packages/react-reconciler/src/ReactInternalTypes.js +++ b/packages/react-reconciler/src/ReactInternalTypes.js @@ -133,9 +133,6 @@ export type Fiber = { // A queue of state updates and callbacks. updateQueue: mixed, - // useMemoCache storage - memoCache: MemoCache | null, - // The state used to create the output memoizedState: any, diff --git a/packages/react-reconciler/src/__tests__/useMemoCache-test.js b/packages/react-reconciler/src/__tests__/useMemoCache-test.js index 6c57d4ad6f215..f683b571d179e 100644 --- a/packages/react-reconciler/src/__tests__/useMemoCache-test.js +++ b/packages/react-reconciler/src/__tests__/useMemoCache-test.js @@ -44,7 +44,7 @@ describe('ReactCache', () => { test('render component using cache', async () => { function Component(props) { const cache = useMemoCache(1); - expect(cache).toBeInstanceOf(Array); + expect(Array.isArray(cache)).toBe(true); expect(cache.length).toBe(1); expect(cache[0]).toBe(undefined); return 'Ok'; From fcefa968ac1bb7fea37311dd84bfedfb67194cf6 Mon Sep 17 00:00:00 2001 From: Joe Savona Date: Tue, 13 Sep 2022 13:35:50 -0700 Subject: [PATCH 10/12] address feedback --- packages/react-reconciler/src/ReactFiberHooks.new.js | 8 ++++++++ packages/react-reconciler/src/ReactFiberHooks.old.js | 8 ++++++++ .../src/__tests__/useMemoCache-test.js | 12 ++++++------ 3 files changed, 22 insertions(+), 6 deletions(-) diff --git a/packages/react-reconciler/src/ReactFiberHooks.new.js b/packages/react-reconciler/src/ReactFiberHooks.new.js index 292323bdfc1aa..189b76bc71b97 100644 --- a/packages/react-reconciler/src/ReactFiberHooks.new.js +++ b/packages/react-reconciler/src/ReactFiberHooks.new.js @@ -836,6 +836,14 @@ function useMemoCache(size: number): Array { data = memoCache.data[memoCache.index] = new Array(size); } else if (data.length !== size) { // TODO: consider warning or throwing here + if (__DEV__) { +console.error( + 'Expected a constant size argument for each invocation of useMemoCache. ' + + 'The previous cache was allocated with size %s but size %s was requested.', + data.length, + size, + ) +}; } memoCache.index++; return data; diff --git a/packages/react-reconciler/src/ReactFiberHooks.old.js b/packages/react-reconciler/src/ReactFiberHooks.old.js index acb9c9542bf33..0e412bb04aaeb 100644 --- a/packages/react-reconciler/src/ReactFiberHooks.old.js +++ b/packages/react-reconciler/src/ReactFiberHooks.old.js @@ -836,6 +836,14 @@ function useMemoCache(size: number): Array { data = memoCache.data[memoCache.index] = new Array(size); } else if (data.length !== size) { // TODO: consider warning or throwing here + if (__DEV__) { +console.error( + 'Expected a constant size argument for each invocation of useMemoCache. ' + + 'The previous cache was allocated with size %s but size %s was requested.', + data.length, + size, + ) +}; } memoCache.index++; return data; diff --git a/packages/react-reconciler/src/__tests__/useMemoCache-test.js b/packages/react-reconciler/src/__tests__/useMemoCache-test.js index f683b571d179e..94684ec8c00b3 100644 --- a/packages/react-reconciler/src/__tests__/useMemoCache-test.js +++ b/packages/react-reconciler/src/__tests__/useMemoCache-test.js @@ -5,7 +5,7 @@ let useState; let useMemoCache; let ErrorBoundary; -describe('ReactCache', () => { +describe('useMemoCache()', () => { beforeEach(() => { jest.resetModules(); @@ -40,7 +40,7 @@ describe('ReactCache', () => { ErrorBoundary = _ErrorBoundary; }); - // @gate experimental || www + // @gate enableUseMemoCacheHook test('render component using cache', async () => { function Component(props) { const cache = useMemoCache(1); @@ -56,7 +56,7 @@ describe('ReactCache', () => { expect(root).toMatchRenderedOutput('Ok'); }); - // @gate experimental || www + // @gate enableUseMemoCacheHook test('update component using cache', async () => { let setX; let forceUpdate; @@ -120,7 +120,7 @@ describe('ReactCache', () => { expect(data).toBe(data1); // confirm that the cache persisted across renders }); - // @gate experimental || www + // @gate enableUseMemoCacheHook test('update component using cache with setstate during render', async () => { let setX; let setN; @@ -199,7 +199,7 @@ describe('ReactCache', () => { expect(data).toBe(data1); // confirm that the cache persisted across renders }); - // @gate experimental || www + // @gate enableUseMemoCacheHook test('update component using cache with throw during render', async () => { let setX; let setN; @@ -288,7 +288,7 @@ describe('ReactCache', () => { expect(data).toBe(data1); // confirm that the cache persisted across renders }); - // @gate experimental || www + // @gate enableUseMemoCacheHook test('update component and custom hook with caches', async () => { let setX; let forceUpdate; From e63e02b9882f82b2906f46d0302e8b3500366593 Mon Sep 17 00:00:00 2001 From: Joe Savona Date: Tue, 13 Sep 2022 14:03:21 -0700 Subject: [PATCH 11/12] Try to eliminate size increase in prod bundle --- .../src/ReactFiberHooks.new.js | 47 ++++++++++++------- .../src/ReactFiberHooks.old.js | 47 ++++++++++++------- 2 files changed, 60 insertions(+), 34 deletions(-) diff --git a/packages/react-reconciler/src/ReactFiberHooks.new.js b/packages/react-reconciler/src/ReactFiberHooks.new.js index 189b76bc71b97..d598fd12950aa 100644 --- a/packages/react-reconciler/src/ReactFiberHooks.new.js +++ b/packages/react-reconciler/src/ReactFiberHooks.new.js @@ -182,7 +182,8 @@ type StoreConsistencyCheck = { export type FunctionComponentUpdateQueue = { lastEffect: Effect | null, stores: Array> | null, - memoCache: MemoCache | null, + // NOTE: optional, only set when enableUseMemoCacheHook is enabled + memoCache?: MemoCache | null, }; type BasicStateAction = (S => S) | S; @@ -716,11 +717,23 @@ function updateWorkInProgressHook(): Hook { return workInProgressHook; } -function createFunctionComponentUpdateQueue(): FunctionComponentUpdateQueue { - return { - lastEffect: null, - stores: null, - memoCache: null, +// NOTE: defining two versions of this function to avoid size impact when this feature is disabled. +// Previously this function was inlined, the additional `memoCache` property makes it not inlined. +let createFunctionComponentUpdateQueue: () => FunctionComponentUpdateQueue; +if (enableUseMemoCacheHook) { + createFunctionComponentUpdateQueue = () => { + return { + lastEffect: null, + stores: null, + memoCache: null, + }; + }; +} else { + createFunctionComponentUpdateQueue = () => { + return { + lastEffect: null, + stores: null, + }; }; } @@ -803,13 +816,13 @@ function useMemoCache(size: number): Array { // 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) { + 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) { + const currentMemoCache: ?MemoCache = currentUpdateQueue.memoCache; + if (currentMemoCache != null) { memoCache = { data: currentMemoCache.data.map(array => array.slice()), index: 0, @@ -819,7 +832,7 @@ function useMemoCache(size: number): Array { } } // Finally fall back to allocating a fresh instance - if (memoCache === null) { + if (memoCache == null) { memoCache = { data: [], index: 0, @@ -837,13 +850,13 @@ function useMemoCache(size: number): Array { } else if (data.length !== size) { // TODO: consider warning or throwing here if (__DEV__) { -console.error( - 'Expected a constant size argument for each invocation of useMemoCache. ' + - 'The previous cache was allocated with size %s but size %s was requested.', - data.length, - size, - ) -}; + console.error( + 'Expected a constant size argument for each invocation of useMemoCache. ' + + 'The previous cache was allocated with size %s but size %s was requested.', + data.length, + size, + ); + } } memoCache.index++; return data; diff --git a/packages/react-reconciler/src/ReactFiberHooks.old.js b/packages/react-reconciler/src/ReactFiberHooks.old.js index 0e412bb04aaeb..dcd0f9b5ecdd2 100644 --- a/packages/react-reconciler/src/ReactFiberHooks.old.js +++ b/packages/react-reconciler/src/ReactFiberHooks.old.js @@ -182,7 +182,8 @@ type StoreConsistencyCheck = { export type FunctionComponentUpdateQueue = { lastEffect: Effect | null, stores: Array> | null, - memoCache: MemoCache | null, + // NOTE: optional, only set when enableUseMemoCacheHook is enabled + memoCache?: MemoCache | null, }; type BasicStateAction = (S => S) | S; @@ -716,11 +717,23 @@ function updateWorkInProgressHook(): Hook { return workInProgressHook; } -function createFunctionComponentUpdateQueue(): FunctionComponentUpdateQueue { - return { - lastEffect: null, - stores: null, - memoCache: null, +// NOTE: defining two versions of this function to avoid size impact when this feature is disabled. +// Previously this function was inlined, the additional `memoCache` property makes it not inlined. +let createFunctionComponentUpdateQueue: () => FunctionComponentUpdateQueue; +if (enableUseMemoCacheHook) { + createFunctionComponentUpdateQueue = () => { + return { + lastEffect: null, + stores: null, + memoCache: null, + }; + }; +} else { + createFunctionComponentUpdateQueue = () => { + return { + lastEffect: null, + stores: null, + }; }; } @@ -803,13 +816,13 @@ function useMemoCache(size: number): Array { // 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) { + 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) { + const currentMemoCache: ?MemoCache = currentUpdateQueue.memoCache; + if (currentMemoCache != null) { memoCache = { data: currentMemoCache.data.map(array => array.slice()), index: 0, @@ -819,7 +832,7 @@ function useMemoCache(size: number): Array { } } // Finally fall back to allocating a fresh instance - if (memoCache === null) { + if (memoCache == null) { memoCache = { data: [], index: 0, @@ -837,13 +850,13 @@ function useMemoCache(size: number): Array { } else if (data.length !== size) { // TODO: consider warning or throwing here if (__DEV__) { -console.error( - 'Expected a constant size argument for each invocation of useMemoCache. ' + - 'The previous cache was allocated with size %s but size %s was requested.', - data.length, - size, - ) -}; + console.error( + 'Expected a constant size argument for each invocation of useMemoCache. ' + + 'The previous cache was allocated with size %s but size %s was requested.', + data.length, + size, + ); + } } memoCache.index++; return data; From 32cd35e9834124c7706ab9bdb5c8710d562b9dfd Mon Sep 17 00:00:00 2001 From: Joe Savona Date: Tue, 13 Sep 2022 14:16:32 -0700 Subject: [PATCH 12/12] update to retrigger ci --- packages/react-reconciler/src/ReactFiberHooks.new.js | 2 +- packages/react-reconciler/src/ReactFiberHooks.old.js | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-) diff --git a/packages/react-reconciler/src/ReactFiberHooks.new.js b/packages/react-reconciler/src/ReactFiberHooks.new.js index d598fd12950aa..602f855c76d85 100644 --- a/packages/react-reconciler/src/ReactFiberHooks.new.js +++ b/packages/react-reconciler/src/ReactFiberHooks.new.js @@ -831,7 +831,7 @@ function useMemoCache(size: number): Array { } } } - // Finally fall back to allocating a fresh instance + // Finally fall back to allocating a fresh instance of the cache if (memoCache == null) { memoCache = { data: [], diff --git a/packages/react-reconciler/src/ReactFiberHooks.old.js b/packages/react-reconciler/src/ReactFiberHooks.old.js index dcd0f9b5ecdd2..f2b024bee943d 100644 --- a/packages/react-reconciler/src/ReactFiberHooks.old.js +++ b/packages/react-reconciler/src/ReactFiberHooks.old.js @@ -831,7 +831,7 @@ function useMemoCache(size: number): Array { } } } - // Finally fall back to allocating a fresh instance + // Finally fall back to allocating a fresh instance of the cache if (memoCache == null) { memoCache = { data: [],