From c11c9510fa14bbd87053685c19bfdfec2f427f49 Mon Sep 17 00:00:00 2001 From: lauren Date: Wed, 20 Nov 2024 16:54:41 -0500 Subject: [PATCH] [crud] Fix deps comparison bug (#31599) Fixes a bug with the experimental `useResourceEffect` hook where we would compare the wrong deps when there happened to be another kind of effect preceding the ResourceEffect. To do this correctly we need to add a pointer to the ResourceEffect's identity on the update. I also unified the previously separate push effect impls for resource effects since they are always pushed together as a unit. --- .../react-reconciler/src/ReactFiberHooks.js | 80 +++++++++---------- .../ReactHooksWithNoopRenderer-test.js | 73 +++++++++++++++++ scripts/error-codes/codes.json | 4 +- 3 files changed, 115 insertions(+), 42 deletions(-) diff --git a/packages/react-reconciler/src/ReactFiberHooks.js b/packages/react-reconciler/src/ReactFiberHooks.js index d7f9d00e79bcf..514f93760981c 100644 --- a/packages/react-reconciler/src/ReactFiberHooks.js +++ b/packages/react-reconciler/src/ReactFiberHooks.js @@ -253,6 +253,7 @@ export type ResourceEffectUpdate = { update: ((resource: mixed) => void) | void, deps: Array | void | null, next: Effect, + identity: ResourceEffectIdentity, }; type StoreInstance = { @@ -2585,40 +2586,37 @@ function pushSimpleEffect( return pushEffectImpl(effect); } -function pushResourceEffectIdentity( - tag: HookFlags, +function pushResourceEffect( + identityTag: HookFlags, + updateTag: HookFlags, inst: EffectInstance, create: () => mixed, - deps: Array | void | null, + createDeps: Array | void | null, + update: ((resource: mixed) => void) | void, + updateDeps: Array | void | null, ): Effect { - const effect: ResourceEffectIdentity = { + const effectIdentity: ResourceEffectIdentity = { resourceKind: ResourceEffectIdentityKind, - tag, + tag: identityTag, create, - deps, + deps: createDeps, inst, // Circular next: (null: any), }; - return pushEffectImpl(effect); -} + pushEffectImpl(effectIdentity); -function pushResourceEffectUpdate( - tag: HookFlags, - inst: EffectInstance, - update: ((resource: mixed) => void) | void, - deps: Array | void | null, -): Effect { - const effect: ResourceEffectUpdate = { + const effectUpdate: ResourceEffectUpdate = { resourceKind: ResourceEffectUpdateKind, - tag, + tag: updateTag, update, - deps, + deps: updateDeps, inst, + identity: effectIdentity, // Circular next: (null: any), }; - return pushEffectImpl(effect); + return pushEffectImpl(effectUpdate); } function pushEffectImpl(effect: Effect): Effect { @@ -2792,15 +2790,12 @@ function mountResourceEffectImpl( currentlyRenderingFiber.flags |= fiberFlags; const inst = createEffectInstance(); inst.destroy = destroy; - hook.memoizedState = pushResourceEffectIdentity( + hook.memoizedState = pushResourceEffect( HookHasEffect | hookFlags, + hookFlags, inst, create, createDeps, - ); - hook.memoizedState = pushResourceEffectUpdate( - hookFlags, - inst, update, updateDeps, ); @@ -2847,25 +2842,31 @@ function updateResourceEffectImpl( const prevEffect: Effect = currentHook.memoizedState; if (nextCreateDeps !== null) { let prevCreateDeps; - // Seems sketchy but in practice we always push an Identity and an Update together. For safety - // we error in DEV if this does not hold true. - if (prevEffect.resourceKind === ResourceEffectUpdateKind) { + if ( + prevEffect.resourceKind != null && + prevEffect.resourceKind === ResourceEffectUpdateKind + ) { prevCreateDeps = - prevEffect.next.deps != null ? prevEffect.next.deps : null; + prevEffect.identity.deps != null ? prevEffect.identity.deps : null; } else { - if (__DEV__) { - console.error( - 'Expected a ResourceEffectUpdateKind to be pushed together with ' + - 'ResourceEffectIdentityKind, got %s. This is a bug in React.', - prevEffect.resourceKind, - ); - } - prevCreateDeps = prevEffect.deps != null ? prevEffect.deps : null; + throw new Error( + `Expected a ResourceEffectUpdate to be pushed together with ResourceEffectIdentity. This is a bug in React.`, + ); } isCreateDepsSame = areHookInputsEqual(nextCreateDeps, prevCreateDeps); } if (nextUpdateDeps !== null) { - const prevUpdateDeps = prevEffect.deps != null ? prevEffect.deps : null; + let prevUpdateDeps; + if ( + prevEffect.resourceKind != null && + prevEffect.resourceKind === ResourceEffectUpdateKind + ) { + prevUpdateDeps = prevEffect.deps != null ? prevEffect.deps : null; + } else { + throw new Error( + `Expected a ResourceEffectUpdate to be pushed together with ResourceEffectIdentity. This is a bug in React.`, + ); + } isUpdateDepsSame = areHookInputsEqual(nextUpdateDeps, prevUpdateDeps); } } @@ -2874,15 +2875,12 @@ function updateResourceEffectImpl( currentlyRenderingFiber.flags |= fiberFlags; } - hook.memoizedState = pushResourceEffectIdentity( + hook.memoizedState = pushResourceEffect( isCreateDepsSame ? hookFlags : HookHasEffect | hookFlags, + isUpdateDepsSame ? hookFlags : HookHasEffect | hookFlags, inst, create, nextCreateDeps, - ); - hook.memoizedState = pushResourceEffectUpdate( - isUpdateDepsSame ? hookFlags : HookHasEffect | hookFlags, - inst, update, nextUpdateDeps, ); diff --git a/packages/react-reconciler/src/__tests__/ReactHooksWithNoopRenderer-test.js b/packages/react-reconciler/src/__tests__/ReactHooksWithNoopRenderer-test.js index f9b382647ddea..a0e84b81d034f 100644 --- a/packages/react-reconciler/src/__tests__/ReactHooksWithNoopRenderer-test.js +++ b/packages/react-reconciler/src/__tests__/ReactHooksWithNoopRenderer-test.js @@ -3927,6 +3927,79 @@ describe('ReactHooksWithNoopRenderer', () => { , ); }); + + // @gate enableUseResourceEffectHook + it('composes with other kinds of effects', async () => { + let rerender; + function App({id, username}) { + const [count, rerender_] = useState(0); + rerender = rerender_; + const opts = useMemo(() => { + return {username}; + }, [username]); + useEffect(() => { + Scheduler.log(`useEffect(${count})`); + }, [count]); + useResourceEffect( + () => { + const resource = new Resource(id, opts); + Scheduler.log(`create(${resource.id}, ${resource.opts.username})`); + return resource; + }, + [id], + resource => { + resource.update(opts); + Scheduler.log(`update(${resource.id}, ${resource.opts.username})`); + }, + [opts], + resource => { + resource.destroy(); + Scheduler.log(`destroy(${resource.id}, ${resource.opts.username})`); + }, + ); + return null; + } + + await act(() => { + ReactNoop.render(); + }); + assertLog(['useEffect(0)', 'create(1, Jack)']); + + await act(() => { + ReactNoop.render(); + }); + assertLog(['update(1, Lauren)']); + + await act(() => { + ReactNoop.render(); + }); + assertLog([]); + + await act(() => { + ReactNoop.render(); + }); + assertLog(['update(1, Jordan)']); + + await act(() => { + rerender(n => n + 1); + }); + assertLog(['useEffect(1)']); + + await act(() => { + ReactNoop.render(); + }); + assertLog(['update(1, Mofei)']); + + await act(() => { + ReactNoop.render(); + }); + assertLog(['destroy(1, Mofei)', 'create(2, Jack)']); + + await act(() => { + ReactNoop.render(null); + }); + assertLog(['destroy(2, Jack)']); + }); }); describe('useCallback', () => { diff --git a/scripts/error-codes/codes.json b/scripts/error-codes/codes.json index 48b6ae91a1dc2..4c99d330f05ac 100644 --- a/scripts/error-codes/codes.json +++ b/scripts/error-codes/codes.json @@ -527,5 +527,7 @@ "539": "Binary RSC chunks cannot be encoded as strings. This is a bug in the wiring of the React streams.", "540": "String chunks need to be passed in their original shape. Not split into smaller string chunks. This is a bug in the wiring of the React streams.", "541": "Compared context values must be arrays", - "542": "Suspense Exception: This is not a real error! It's an implementation detail of `useActionState` to interrupt the current render. You must either rethrow it immediately, or move the `useActionState` call outside of the `try/catch` block. Capturing without rethrowing will lead to unexpected behavior.\n\nTo handle async errors, wrap your component in an error boundary." + "542": "Suspense Exception: This is not a real error! It's an implementation detail of `useActionState` to interrupt the current render. You must either rethrow it immediately, or move the `useActionState` call outside of the `try/catch` block. Capturing without rethrowing will lead to unexpected behavior.\n\nTo handle async errors, wrap your component in an error boundary.", + "543": "Expected a ResourceEffectUpdate to be pushed together with ResourceEffectIdentity. This is a bug in React." } +