diff --git a/packages/react-debug-tools/src/ReactDebugHooks.js b/packages/react-debug-tools/src/ReactDebugHooks.js index 798cdec4380f8..5cf93c587de11 100644 --- a/packages/react-debug-tools/src/ReactDebugHooks.js +++ b/packages/react-debug-tools/src/ReactDebugHooks.js @@ -374,11 +374,8 @@ function useInsertionEffect( } function useEffect( - create: (() => (() => void) | void) | (() => {...} | void | null), - createDeps: Array | void | null, - update?: ((resource: {...} | void | null) => void) | void, - updateDeps?: Array | void | null, - destroy?: ((resource: {...} | void | null) => void) | void, + create: () => (() => void) | void, + deps: Array | void | null, ): void { nextHook(); hookLog.push({ diff --git a/packages/react-dom/src/__tests__/ReactDOMServerIntegrationHooks-test.js b/packages/react-dom/src/__tests__/ReactDOMServerIntegrationHooks-test.js index 840d6c5b15d5e..e1aeea70fd806 100644 --- a/packages/react-dom/src/__tests__/ReactDOMServerIntegrationHooks-test.js +++ b/packages/react-dom/src/__tests__/ReactDOMServerIntegrationHooks-test.js @@ -653,52 +653,6 @@ describe('ReactDOMServerHooks', () => { }); }); - describe('useEffect with CRUD overload', () => { - gate(flags => { - if (flags.enableUseEffectCRUDOverload) { - const yields = []; - itRenders( - 'should ignore resource effects on the server', - async render => { - function Counter(props) { - useEffect( - () => { - yieldValue('created on client'); - return {resource_counter: props.count}; - }, - [props.count], - resource => { - resource.resource_counter = props.count; - yieldValue('updated on client'); - }, - [props.count], - () => { - yieldValue('cleanup on client'); - }, - ); - return ; - } - - const domNode = await render(); - yields.push(clearLog()); - expect(domNode.tagName).toEqual('SPAN'); - expect(domNode.textContent).toEqual('Count: 0'); - }, - ); - - it('verifies yields in order', () => { - expect(yields).toEqual([ - ['Count: 0'], // server render - ['Count: 0'], // server stream - ['Count: 0', 'created on client'], // clean render - ['Count: 0', 'created on client'], // hydrated render - // nothing yielded for bad markup - ]); - }); - } - }); - }); - describe('useContext', () => { itThrowsWhenRendering( 'if used inside a class component', diff --git a/packages/react-reconciler/src/ReactFiberCallUserSpace.js b/packages/react-reconciler/src/ReactFiberCallUserSpace.js index a9b5590f387b7..b48933fe3c0dd 100644 --- a/packages/react-reconciler/src/ReactFiberCallUserSpace.js +++ b/packages/react-reconciler/src/ReactFiberCallUserSpace.js @@ -14,11 +14,6 @@ import type {CapturedValue} from './ReactCapturedValue'; import {isRendering, setIsRendering} from './ReactCurrentFiber'; import {captureCommitPhaseError} from './ReactFiberWorkLoop'; -import { - ResourceEffectIdentityKind, - ResourceEffectUpdateKind, -} from './ReactFiberHooks'; -import {enableUseEffectCRUDOverload} from 'shared/ReactFeatureFlags'; // These indirections exists so we can exclude its stack frame in DEV (and anything below it). // TODO: Consider marking the whole bundle instead of these boundaries. @@ -184,51 +179,11 @@ const callCreate = { 'react-stack-bottom-frame': function ( effect: Effect, ): (() => void) | {...} | void | null { - if (!enableUseEffectCRUDOverload) { - if (effect.resourceKind != null) { - if (__DEV__) { - console.error( - 'Expected only SimpleEffects when enableUseEffectCRUDOverload is disabled, ' + - 'got %s', - effect.resourceKind, - ); - } - } - const create = effect.create; - const inst = effect.inst; - // $FlowFixMe[not-a-function] (@poteto) - const destroy = create(); - // $FlowFixMe[incompatible-type] (@poteto) - inst.destroy = destroy; - return destroy; - } else { - if (effect.resourceKind == null) { - const create = effect.create; - const inst = effect.inst; - const destroy = create(); - inst.destroy = destroy; - return destroy; - } - switch (effect.resourceKind) { - case ResourceEffectIdentityKind: { - return effect.create(); - } - case ResourceEffectUpdateKind: { - if (typeof effect.update === 'function') { - effect.update(effect.inst.resource); - } - break; - } - default: { - if (__DEV__) { - console.error( - 'Unhandled Effect kind %s. This is a bug in React.', - effect.kind, - ); - } - } - } - } + const create = effect.create; + const inst = effect.inst; + const destroy = create(); + inst.destroy = destroy; + return destroy; }, }; diff --git a/packages/react-reconciler/src/ReactFiberCommitEffects.js b/packages/react-reconciler/src/ReactFiberCommitEffects.js index a0f6b54780a2d..cc393b58995d8 100644 --- a/packages/react-reconciler/src/ReactFiberCommitEffects.js +++ b/packages/react-reconciler/src/ReactFiberCommitEffects.js @@ -23,7 +23,6 @@ import { enableProfilerCommitHooks, enableProfilerNestedUpdatePhase, enableSchedulingProfiler, - enableUseEffectCRUDOverload, enableViewTransition, enableFragmentRefs, } from 'shared/ReactFeatureFlags'; @@ -62,7 +61,6 @@ import { Layout as HookLayout, Insertion as HookInsertion, Passive as HookPassive, - HasEffect as HookHasEffect, } from './ReactHookEffectTags'; import {didWarnAboutReassigningProps} from './ReactFiberBeginWork'; import { @@ -84,10 +82,6 @@ import { } from './ReactFiberCallUserSpace'; import {runWithFiberInDEV} from './ReactCurrentFiber'; -import { - ResourceEffectIdentityKind, - ResourceEffectUpdateKind, -} from './ReactFiberHooks'; function shouldProfile(current: Fiber): boolean { return ( @@ -164,91 +158,19 @@ export function commitHookEffectListMount( // Mount let destroy; - if (enableUseEffectCRUDOverload) { - if (effect.resourceKind === ResourceEffectIdentityKind) { - if (__DEV__) { - effect.inst.resource = runWithFiberInDEV( - finishedWork, - callCreateInDEV, - effect, - ); - if (effect.inst.resource == null) { - console.error( - 'useEffect must provide a callback which returns a resource. ' + - 'If a managed resource is not needed here, do not provide an updater or ' + - 'destroy callback. Received %s', - effect.inst.resource, - ); - } - } else { - effect.inst.resource = effect.create(); - } - destroy = effect.inst.destroy; - } - if (effect.resourceKind === ResourceEffectUpdateKind) { - if ( - // We don't want to fire updates on remount during Activity - (flags & HookHasEffect) > 0 && - typeof effect.update === 'function' && - effect.inst.resource != null - ) { - // TODO(@poteto) what about multiple updates? - if (__DEV__) { - runWithFiberInDEV(finishedWork, callCreateInDEV, effect); - } else { - effect.update(effect.inst.resource); - } - } - } - } if (__DEV__) { if ((flags & HookInsertion) !== NoHookEffect) { setIsRunningInsertionEffect(true); } - if (enableUseEffectCRUDOverload) { - if (effect.resourceKind == null) { - destroy = runWithFiberInDEV( - finishedWork, - callCreateInDEV, - effect, - ); - } - } else { - destroy = runWithFiberInDEV( - finishedWork, - callCreateInDEV, - effect, - ); - } + destroy = runWithFiberInDEV(finishedWork, callCreateInDEV, effect); if ((flags & HookInsertion) !== NoHookEffect) { setIsRunningInsertionEffect(false); } } else { - if (enableUseEffectCRUDOverload) { - if (effect.resourceKind == null) { - const create = effect.create; - const inst = effect.inst; - destroy = create(); - inst.destroy = destroy; - } - } else { - if (effect.resourceKind != null) { - if (__DEV__) { - console.error( - 'Expected only SimpleEffects when enableUseEffectCRUDOverload is disabled, ' + - 'got %s', - effect.resourceKind, - ); - } - } - const create = effect.create; - const inst = effect.inst; - // $FlowFixMe[incompatible-type] (@poteto) - // $FlowFixMe[not-a-function] (@poteto) - destroy = create(); - // $FlowFixMe[incompatible-type] (@poteto) - inst.destroy = destroy; - } + const create = effect.create; + const inst = effect.inst; + destroy = create(); + inst.destroy = destroy; } if (enableSchedulingProfiler) { @@ -338,13 +260,7 @@ export function commitHookEffectListUnmount( const inst = effect.inst; const destroy = inst.destroy; if (destroy !== undefined) { - if (enableUseEffectCRUDOverload) { - if (effect.resourceKind == null) { - inst.destroy = undefined; - } - } else { - inst.destroy = undefined; - } + inst.destroy = undefined; if (enableSchedulingProfiler) { if ((flags & HookPassive) !== NoHookEffect) { markComponentPassiveEffectUnmountStarted(finishedWork); @@ -358,41 +274,7 @@ export function commitHookEffectListUnmount( setIsRunningInsertionEffect(true); } } - if (enableUseEffectCRUDOverload) { - if ( - effect.resourceKind === ResourceEffectIdentityKind && - effect.inst.resource != null - ) { - safelyCallDestroy( - finishedWork, - nearestMountedAncestor, - destroy, - effect.inst.resource, - ); - if (effect.next.resourceKind === ResourceEffectUpdateKind) { - // $FlowFixMe[prop-missing] (@poteto) - effect.next.update = undefined; - } else { - if (__DEV__) { - console.error( - 'Expected a ResourceEffectUpdateKind to follow ResourceEffectIdentityKind, ' + - 'got %s. This is a bug in React.', - effect.next.resourceKind, - ); - } - } - effect.inst.resource = null; - } - if (effect.resourceKind == null) { - safelyCallDestroy( - finishedWork, - nearestMountedAncestor, - destroy, - ); - } - } else { - safelyCallDestroy(finishedWork, nearestMountedAncestor, destroy); - } + safelyCallDestroy(finishedWork, nearestMountedAncestor, destroy); if (__DEV__) { if ((flags & HookInsertion) !== NoHookEffect) { setIsRunningInsertionEffect(false); diff --git a/packages/react-reconciler/src/ReactFiberHooks.js b/packages/react-reconciler/src/ReactFiberHooks.js index 270cd87d4941c..30dc3f91384b7 100644 --- a/packages/react-reconciler/src/ReactFiberHooks.js +++ b/packages/react-reconciler/src/ReactFiberHooks.js @@ -43,7 +43,6 @@ import { enableSchedulingProfiler, enableTransitionTracing, enableUseEffectEventHook, - enableUseEffectCRUDOverload, enableLegacyCache, disableLegacyMode, enableNoCloningMemoCache, @@ -219,43 +218,16 @@ export type Hook = { // the additional memory and we can follow up with performance // optimizations later. type EffectInstance = { - resource: {...} | void | null, - destroy: void | (() => void) | ((resource: {...} | void | null) => void), + destroy: void | (() => void), }; -export const ResourceEffectIdentityKind: 0 = 0; -export const ResourceEffectUpdateKind: 1 = 1; -export type EffectKind = - | typeof ResourceEffectIdentityKind - | typeof ResourceEffectUpdateKind; -export type Effect = - | SimpleEffect - | ResourceEffectIdentity - | ResourceEffectUpdate; -export type SimpleEffect = { +export type Effect = { tag: HookFlags, inst: EffectInstance, create: () => (() => void) | void, deps: Array | void | null, next: Effect, }; -export type ResourceEffectIdentity = { - resourceKind: typeof ResourceEffectIdentityKind, - tag: HookFlags, - inst: EffectInstance, - create: () => {...} | void | null, - deps: Array | void | null, - next: Effect, -}; -export type ResourceEffectUpdate = { - resourceKind: typeof ResourceEffectUpdateKind, - tag: HookFlags, - inst: EffectInstance, - update: ((resource: {...} | void | null) => void) | void, - deps: Array | void | null, - next: Effect, - identity: ResourceEffectIdentity, -}; type StoreInstance = { value: T, @@ -377,23 +349,6 @@ function checkDepsAreArrayDev(deps: mixed): void { } } -function checkDepsAreNonEmptyArrayDev(deps: mixed): void { - if (__DEV__) { - if ( - deps !== undefined && - deps !== null && - isArray(deps) && - deps.length === 0 - ) { - console.error( - '%s received a dependency array with no dependencies. When ' + - 'specified, the dependency array must have at least one dependency.', - currentHookNameInDev, - ); - } - } -} - function warnOnHookMismatchInDev(currentHookName: HookType): void { if (__DEV__) { const componentName = getComponentNameFromFiber(currentlyRenderingFiber); @@ -2540,15 +2495,12 @@ function pushSimpleEffect( tag: HookFlags, inst: EffectInstance, create: () => (() => void) | void, - createDeps: Array | void | null, - update?: ((resource: {...} | void | null) => void) | void, - updateDeps?: Array | void | null, - destroy?: ((resource: {...} | void | null) => void) | void, + deps: Array | void | null, ): Effect { const effect: Effect = { tag, create, - deps: createDeps, + deps, inst, // Circular next: (null: any), @@ -2556,39 +2508,6 @@ function pushSimpleEffect( return pushEffectImpl(effect); } -function pushResourceEffect( - identityTag: HookFlags, - updateTag: HookFlags, - inst: EffectInstance, - create: () => {...} | void | null, - createDeps: Array | void | null, - update: ((resource: {...} | void | null) => void) | void, - updateDeps: Array | void | null, -): Effect { - const effectIdentity: ResourceEffectIdentity = { - resourceKind: ResourceEffectIdentityKind, - tag: identityTag, - create, - deps: createDeps, - inst, - // Circular - next: (null: any), - }; - pushEffectImpl(effectIdentity); - - const effectUpdate: ResourceEffectUpdate = { - resourceKind: ResourceEffectUpdateKind, - tag: updateTag, - update, - deps: updateDeps, - inst, - identity: effectIdentity, - // Circular - next: (null: any), - }; - return pushEffectImpl(effectUpdate); -} - function pushEffectImpl(effect: Effect): Effect { let componentUpdateQueue: null | FunctionComponentUpdateQueue = (currentlyRenderingFiber.updateQueue: any); @@ -2609,7 +2528,7 @@ function pushEffectImpl(effect: Effect): Effect { } function createEffectInstance(): EffectInstance { - return {destroy: undefined, resource: undefined}; + return {destroy: undefined}; } function mountRef(initialValue: T): {current: T} { @@ -2628,13 +2547,10 @@ function mountEffectImpl( fiberFlags: Flags, hookFlags: HookFlags, create: () => (() => void) | void, - createDeps: Array | void | null, - update?: ((resource: {...} | void | null) => void) | void, - updateDeps?: Array | void | null, - destroy?: ((resource: {...} | void | null) => void) | void, + deps: Array | void | null, ): void { const hook = mountWorkInProgressHook(); - const nextDeps = createDeps === undefined ? null : createDeps; + const nextDeps = deps === undefined ? null : deps; currentlyRenderingFiber.flags |= fiberFlags; hook.memoizedState = pushSimpleEffect( HookHasEffect | hookFlags, @@ -2685,223 +2601,35 @@ function updateEffectImpl( } function mountEffect( - create: (() => (() => void) | void) | (() => {...} | void | null), - createDeps: Array | void | null, - update?: ((resource: {...} | void | null) => void) | void, - updateDeps?: Array | void | null, - destroy?: ((resource: {...} | void | null) => void) | void, + create: () => (() => void) | void, + deps: Array | void | null, ): void { if ( __DEV__ && (currentlyRenderingFiber.mode & StrictEffectsMode) !== NoMode && (currentlyRenderingFiber.mode & NoStrictPassiveEffectsMode) === NoMode ) { - if ( - enableUseEffectCRUDOverload && - (typeof update === 'function' || typeof destroy === 'function') - ) { - mountResourceEffectImpl( - MountPassiveDevEffect | PassiveEffect | PassiveStaticEffect, - HookPassive, - create, - createDeps, - update, - updateDeps, - destroy, - ); - } else { - mountEffectImpl( - MountPassiveDevEffect | PassiveEffect | PassiveStaticEffect, - HookPassive, - // $FlowFixMe[incompatible-call] @poteto it's not possible to narrow `create` without calling it. - create, - createDeps, - ); - } - } else { - if ( - enableUseEffectCRUDOverload && - (typeof update === 'function' || typeof destroy === 'function') - ) { - mountResourceEffectImpl( - PassiveEffect | PassiveStaticEffect, - HookPassive, - create, - createDeps, - update, - updateDeps, - destroy, - ); - } else { - mountEffectImpl( - PassiveEffect | PassiveStaticEffect, - HookPassive, - // $FlowFixMe[incompatible-call] @poteto it's not possible to narrow `create` without calling it. - create, - createDeps, - ); - } - } -} - -function updateEffect( - create: (() => (() => void) | void) | (() => {...} | void | null), - createDeps: Array | void | null, - update?: ((resource: {...} | void | null) => void) | void, - updateDeps?: Array | void | null, - destroy?: ((resource: {...} | void | null) => void) | void, -): void { - if ( - enableUseEffectCRUDOverload && - (typeof update === 'function' || typeof destroy === 'function') - ) { - updateResourceEffectImpl( - PassiveEffect, + mountEffectImpl( + MountPassiveDevEffect | PassiveEffect | PassiveStaticEffect, HookPassive, create, - createDeps, - update, - updateDeps, - destroy, + deps, ); } else { - // $FlowFixMe[incompatible-call] @poteto it's not possible to narrow `create` without calling it. - updateEffectImpl(PassiveEffect, HookPassive, create, createDeps); - } -} - -function mountResourceEffect( - create: () => {...} | void | null, - createDeps: Array | void | null, - update: ((resource: {...} | void | null) => void) | void, - updateDeps: Array | void | null, - destroy: ((resource: {...} | void | null) => void) | void, -) { - if ( - __DEV__ && - (currentlyRenderingFiber.mode & StrictEffectsMode) !== NoMode && - (currentlyRenderingFiber.mode & NoStrictPassiveEffectsMode) === NoMode - ) { - } else { - mountResourceEffectImpl( + mountEffectImpl( PassiveEffect | PassiveStaticEffect, HookPassive, create, - createDeps, - update, - updateDeps, - destroy, + deps, ); } } -function mountResourceEffectImpl( - fiberFlags: Flags, - hookFlags: HookFlags, - create: () => {...} | void | null, - createDeps: Array | void | null, - update: ((resource: {...} | void | null) => void) | void, - updateDeps: Array | void | null, - destroy: ((resource: {...} | void | null) => void) | void, -) { - const hook = mountWorkInProgressHook(); - currentlyRenderingFiber.flags |= fiberFlags; - const inst = createEffectInstance(); - inst.destroy = destroy; - hook.memoizedState = pushResourceEffect( - HookHasEffect | hookFlags, - hookFlags, - inst, - create, - createDeps, - update, - updateDeps, - ); -} - -function updateResourceEffect( - create: () => {...} | void | null, - createDeps: Array | void | null, - update: ((resource: {...} | void | null) => void) | void, - updateDeps: Array | void | null, - destroy: ((resource: {...} | void | null) => void) | void, -) { - updateResourceEffectImpl( - PassiveEffect, - HookPassive, - create, - createDeps, - update, - updateDeps, - destroy, - ); -} - -function updateResourceEffectImpl( - fiberFlags: Flags, - hookFlags: HookFlags, - create: () => {...} | void | null, - createDeps: Array | void | null, - update: ((resource: {...} | void | null) => void) | void, - updateDeps: Array | void | null, - destroy: ((resource: {...} | void | null) => void) | void, -) { - const hook = updateWorkInProgressHook(); - const effect: Effect = hook.memoizedState; - const inst = effect.inst; - inst.destroy = destroy; - - const nextCreateDeps = createDeps === undefined ? null : createDeps; - const nextUpdateDeps = updateDeps === undefined ? null : updateDeps; - let isCreateDepsSame: boolean; - let isUpdateDepsSame: boolean; - - if (currentHook !== null) { - const prevEffect: Effect = currentHook.memoizedState; - if (nextCreateDeps !== null) { - let prevCreateDeps; - if ( - prevEffect.resourceKind != null && - prevEffect.resourceKind === ResourceEffectUpdateKind - ) { - prevCreateDeps = - prevEffect.identity.deps != null ? prevEffect.identity.deps : null; - } else { - 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) { - 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); - } - } - - if (!(isCreateDepsSame && isUpdateDepsSame)) { - currentlyRenderingFiber.flags |= fiberFlags; - } - - hook.memoizedState = pushResourceEffect( - isCreateDepsSame ? hookFlags : HookHasEffect | hookFlags, - isUpdateDepsSame ? hookFlags : HookHasEffect | hookFlags, - inst, - create, - nextCreateDeps, - update, - nextUpdateDeps, - ); +function updateEffect( + create: () => (() => void) | void, + deps: Array | void | null, +): void { + updateEffectImpl(PassiveEffect, HookPassive, create, deps); } function useEffectEventImpl) => Return>( @@ -4339,30 +4067,13 @@ if (__DEV__) { return readContext(context); }, useEffect( - create: (() => (() => void) | void) | (() => {...} | void | null), - createDeps: Array | void | null, - update?: ((resource: {...} | void | null) => void) | void, - updateDeps?: Array | void | null, - destroy?: ((resource: {...} | void | null) => void) | void, + create: () => (() => void) | void, + deps: Array | void | null, ): void { currentHookNameInDev = 'useEffect'; mountHookTypesDev(); - if ( - enableUseEffectCRUDOverload && - (typeof update === 'function' || typeof destroy === 'function') - ) { - checkDepsAreNonEmptyArrayDev(updateDeps); - return mountResourceEffect( - create, - createDeps, - update, - updateDeps, - destroy, - ); - } else { - checkDepsAreArrayDev(createDeps); - return mountEffect(create, createDeps); - } + checkDepsAreArrayDev(deps); + return mountEffect(create, deps); }, useImperativeHandle( ref: {current: T | null} | ((inst: T | null) => mixed) | null | void, @@ -4540,28 +4251,12 @@ if (__DEV__) { return readContext(context); }, useEffect( - create: (() => (() => void) | void) | (() => {...} | void | null), - createDeps: Array | void | null, - update?: ((resource: {...} | void | null) => void) | void, - updateDeps?: Array | void | null, - destroy?: ((resource: {...} | void | null) => void) | void, + create: () => (() => void) | void, + deps: Array | void | null, ): void { currentHookNameInDev = 'useEffect'; updateHookTypesDev(); - if ( - enableUseEffectCRUDOverload && - (typeof update === 'function' || typeof destroy === 'function') - ) { - return mountResourceEffect( - create, - createDeps, - update, - updateDeps, - destroy, - ); - } else { - return mountEffect(create, createDeps); - } + return mountEffect(create, deps); }, useImperativeHandle( ref: {current: T | null} | ((inst: T | null) => mixed) | null | void, @@ -4735,28 +4430,12 @@ if (__DEV__) { return readContext(context); }, useEffect( - create: (() => (() => void) | void) | (() => {...} | void | null), - createDeps: Array | void | null, - update?: ((resource: {...} | void | null) => void) | void, - updateDeps?: Array | void | null, - destroy?: ((resource: {...} | void | null) => void) | void, + create: () => (() => void) | void, + deps: Array | void | null, ): void { currentHookNameInDev = 'useEffect'; updateHookTypesDev(); - if ( - enableUseEffectCRUDOverload && - (typeof update === 'function' || typeof destroy === 'function') - ) { - return updateResourceEffect( - create, - createDeps, - update, - updateDeps, - destroy, - ); - } else { - return updateEffect(create, createDeps); - } + return updateEffect(create, deps); }, useImperativeHandle( ref: {current: T | null} | ((inst: T | null) => mixed) | null | void, @@ -4930,28 +4609,12 @@ if (__DEV__) { return readContext(context); }, useEffect( - create: (() => (() => void) | void) | (() => {...} | void | null), - createDeps: Array | void | null, - update?: ((resource: {...} | void | null) => void) | void, - updateDeps?: Array | void | null, - destroy?: ((resource: {...} | void | null) => void) | void, + create: () => (() => void) | void, + deps: Array | void | null, ): void { currentHookNameInDev = 'useEffect'; updateHookTypesDev(); - if ( - enableUseEffectCRUDOverload && - (typeof update === 'function' || typeof destroy === 'function') - ) { - return updateResourceEffect( - create, - createDeps, - update, - updateDeps, - destroy, - ); - } else { - return updateEffect(create, createDeps); - } + return updateEffect(create, deps); }, useImperativeHandle( ref: {current: T | null} | ((inst: T | null) => mixed) | null | void, @@ -5131,29 +4794,13 @@ if (__DEV__) { return readContext(context); }, useEffect( - create: (() => (() => void) | void) | (() => {...} | void | null), - createDeps: Array | void | null, - update?: ((resource: {...} | void | null) => void) | void, - updateDeps?: Array | void | null, - destroy?: ((resource: {...} | void | null) => void) | void, + create: () => (() => void) | void, + deps: Array | void | null, ): void { currentHookNameInDev = 'useEffect'; warnInvalidHookAccess(); mountHookTypesDev(); - if ( - enableUseEffectCRUDOverload && - (typeof update === 'function' || typeof destroy === 'function') - ) { - return mountResourceEffect( - create, - createDeps, - update, - updateDeps, - destroy, - ); - } else { - return mountEffect(create, createDeps); - } + return mountEffect(create, deps); }, useImperativeHandle( ref: {current: T | null} | ((inst: T | null) => mixed) | null | void, @@ -5352,29 +4999,13 @@ if (__DEV__) { return readContext(context); }, useEffect( - create: (() => (() => void) | void) | (() => {...} | void | null), - createDeps: Array | void | null, - update?: ((resource: {...} | void | null) => void) | void, - updateDeps?: Array | void | null, - destroy?: ((resource: {...} | void | null) => void) | void, + create: () => (() => void) | void, + deps: Array | void | null, ): void { currentHookNameInDev = 'useEffect'; warnInvalidHookAccess(); updateHookTypesDev(); - if ( - enableUseEffectCRUDOverload && - (typeof update === 'function' || typeof destroy === 'function') - ) { - return updateResourceEffect( - create, - createDeps, - update, - updateDeps, - destroy, - ); - } else { - return updateEffect(create, createDeps); - } + return updateEffect(create, deps); }, useImperativeHandle( ref: {current: T | null} | ((inst: T | null) => mixed) | null | void, @@ -5573,29 +5204,13 @@ if (__DEV__) { return readContext(context); }, useEffect( - create: (() => (() => void) | void) | (() => {...} | void | null), - createDeps: Array | void | null, - update?: ((resource: {...} | void | null) => void) | void, - updateDeps?: Array | void | null, - destroy?: ((resource: {...} | void | null) => void) | void, + create: () => (() => void) | void, + deps: Array | void | null, ): void { currentHookNameInDev = 'useEffect'; warnInvalidHookAccess(); updateHookTypesDev(); - if ( - enableUseEffectCRUDOverload && - (typeof update === 'function' || typeof destroy === 'function') - ) { - return updateResourceEffect( - create, - createDeps, - update, - updateDeps, - destroy, - ); - } else { - return updateEffect(create, createDeps); - } + return updateEffect(create, deps); }, useImperativeHandle( ref: {current: T | null} | ((inst: T | null) => mixed) | null | void, diff --git a/packages/react-reconciler/src/ReactInternalTypes.js b/packages/react-reconciler/src/ReactInternalTypes.js index 1452746089c2b..4de46988db14b 100644 --- a/packages/react-reconciler/src/ReactInternalTypes.js +++ b/packages/react-reconciler/src/ReactInternalTypes.js @@ -399,11 +399,8 @@ export type Dispatcher = { useContext(context: ReactContext): T, useRef(initialValue: T): {current: T}, useEffect( - create: (() => (() => void) | void) | (() => {...} | void | null), - createDeps: Array | void | null, - update?: ((resource: {...} | void | null) => void) | void, - updateDeps?: Array | void | null, - destroy?: ((resource: {...} | void | null) => void) | void, + create: () => (() => void) | void, + deps: Array | void | null, ): void, // TODO: Non-nullable once `enableUseEffectEventHook` is on everywhere. useEffectEvent?: ) => mixed>(callback: F) => F, diff --git a/packages/react-reconciler/src/__tests__/ReactHooksWithNoopRenderer-test.js b/packages/react-reconciler/src/__tests__/ReactHooksWithNoopRenderer-test.js index 47fd534e683a9..f72b738cf69b4 100644 --- a/packages/react-reconciler/src/__tests__/ReactHooksWithNoopRenderer-test.js +++ b/packages/react-reconciler/src/__tests__/ReactHooksWithNoopRenderer-test.js @@ -3309,760 +3309,6 @@ describe('ReactHooksWithNoopRenderer', () => { }); }); - // @gate enableUseEffectCRUDOverload - describe('useEffect CRUD overload', () => { - class Resource { - isDeleted: false; - id: string; - opts: mixed; - constructor(id, opts) { - this.id = id; - this.opts = opts; - } - update(opts) { - if (this.isDeleted) { - console.error('Cannot update deleted resource'); - return; - } - this.opts = opts; - } - destroy() { - this.isDeleted = true; - } - } - - // @gate !enableUseEffectCRUDOverload - it('is null when flag is disabled', async () => { - function App({id}) { - useEffect( - () => { - Scheduler.log(`create(${id})`); - return {}; - }, - [id], - () => { - Scheduler.log('update'); - }, - [], - ); - return null; - } - - await expect(async () => { - await act(() => { - ReactNoop.render(); - }); - }).rejects.toThrow( - 'useEffect CRUD overload is not enabled in this build of React.', - ); - }); - - // @gate enableUseEffectCRUDOverload - it('validates non-empty update deps', async () => { - function App({id}) { - useEffect( - () => { - Scheduler.log(`create(${id})`); - return {}; - }, - [id], - () => { - Scheduler.log('update'); - }, - [], - ); - return null; - } - - await act(() => { - ReactNoop.render(); - }); - assertConsoleErrorDev([ - 'useEffect received a dependency array with no dependencies. ' + - 'When specified, the dependency array must have at least one dependency.\n' + - ' in App (at **)', - ]); - }); - - // @gate enableUseEffectCRUDOverload - it('simple mount and update', async () => { - function App({id, username}) { - const opts = useMemo(() => { - return {username}; - }, [username]); - useEffect( - () => { - 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(['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(() => { - ReactNoop.render(); - }); - assertLog(['destroy(1, Jordan)', 'create(2, Jack)']); - - await act(() => { - ReactNoop.render(null); - }); - assertLog(['destroy(2, Jack)']); - }); - - // @gate enableUseEffectCRUDOverload - it('simple mount with no update', async () => { - function App({id, username}) { - const opts = useMemo(() => { - return {username}; - }, [username]); - useEffect( - () => { - 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(['create(1, Jack)']); - - await act(() => { - ReactNoop.render(null); - }); - assertLog(['destroy(1, Jack)']); - }); - - // @gate enableUseEffectCRUDOverload - it('calls update on every render if no deps are specified', async () => { - function App({id, username}) { - const opts = useMemo(() => { - return {username}; - }, [username]); - useEffect( - () => { - 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})`); - }, - ); - return null; - } - - await act(() => { - ReactNoop.render(); - }); - assertLog(['create(1, Jack)']); - - await act(() => { - ReactNoop.render(); - }); - assertLog(['update(1, Jack)']); - - await act(() => { - ReactNoop.render(); - }); - assertLog(['create(2, Jack)', 'update(2, Jack)']); - - await act(() => { - ReactNoop.render(); - }); - - assertLog(['update(2, Lauren)']); - }); - - // @gate enableUseEffectCRUDOverload - it('does not unmount previous useEffect between updates', async () => { - function App({id}) { - useEffect( - () => { - const resource = new Resource(id); - Scheduler.log(`create(${resource.id})`); - return resource; - }, - [], - resource => { - Scheduler.log(`update(${resource.id})`); - }, - undefined, - resource => { - Scheduler.log(`destroy(${resource.id})`); - resource.destroy(); - }, - ); - return ; - } - - await act(async () => { - ReactNoop.render(, () => Scheduler.log('Sync effect')); - await waitFor(['Id: 0', 'Sync effect']); - expect(ReactNoop).toMatchRenderedOutput(); - }); - - assertLog(['create(0)']); - - await act(async () => { - ReactNoop.render(, () => Scheduler.log('Sync effect')); - await waitFor(['Id: 1', 'Sync effect']); - expect(ReactNoop).toMatchRenderedOutput(); - }); - - assertLog(['update(0)']); - }); - - // @gate enableUseEffectCRUDOverload - it('unmounts only on deletion', async () => { - function App({id}) { - useEffect( - () => { - const resource = new Resource(id); - Scheduler.log(`create(${resource.id})`); - return resource; - }, - undefined, - resource => { - Scheduler.log(`update(${resource.id})`); - }, - undefined, - resource => { - Scheduler.log(`destroy(${resource.id})`); - resource.destroy(); - }, - ); - return ; - } - await act(async () => { - ReactNoop.render(, () => Scheduler.log('Sync effect')); - await waitFor(['Id: 0', 'Sync effect']); - expect(ReactNoop).toMatchRenderedOutput(); - }); - - assertLog(['create(0)']); - - ReactNoop.render(null); - await waitForAll(['destroy(0)']); - expect(ReactNoop).toMatchRenderedOutput(null); - }); - - // @gate enableUseEffectCRUDOverload - it('unmounts on deletion', async () => { - function Wrapper(props) { - return ; - } - function App({id, username}) { - const opts = useMemo(() => { - return {username}; - }, [username]); - useEffect( - () => { - 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 ; - } - - await act(async () => { - ReactNoop.render(, () => - Scheduler.log('Sync effect'), - ); - await waitFor(['Id: 0', 'Sync effect']); - expect(ReactNoop).toMatchRenderedOutput(); - }); - - assertLog(['create(0, Sathya)']); - - await act(async () => { - ReactNoop.render(, () => - Scheduler.log('Sync effect'), - ); - await waitFor(['Id: 0', 'Sync effect']); - expect(ReactNoop).toMatchRenderedOutput(); - }); - - assertLog(['update(0, Lauren)']); - - ReactNoop.render(null); - await waitForAll(['destroy(0, Lauren)']); - expect(ReactNoop).toMatchRenderedOutput(null); - }); - - // @gate enableUseEffectCRUDOverload - it('handles errors in create on mount', async () => { - function App({id}) { - useEffect( - () => { - Scheduler.log(`Mount A [${id}]`); - return {}; - }, - undefined, - undefined, - undefined, - resource => { - Scheduler.log(`Unmount A [${id}]`); - }, - ); - useEffect( - () => { - Scheduler.log('Oops!'); - throw new Error('Oops!'); - // eslint-disable-next-line no-unreachable - Scheduler.log(`Mount B [${id}]`); - return {}; - }, - undefined, - undefined, - undefined, - resource => { - Scheduler.log(`Unmount B [${id}]`); - }, - ); - return ; - } - await expect(async () => { - await act(async () => { - ReactNoop.render(, () => Scheduler.log('Sync effect')); - await waitFor(['Id: 0', 'Sync effect']); - expect(ReactNoop).toMatchRenderedOutput(); - }); - }).rejects.toThrow('Oops'); - - assertLog([ - 'Mount A [0]', - 'Oops!', - // Clean up effect A. There's no effect B to clean-up, because it - // never mounted. - 'Unmount A [0]', - ]); - expect(ReactNoop).toMatchRenderedOutput(null); - }); - - // @gate enableUseEffectCRUDOverload - it('handles errors in create on update', async () => { - function App({id}) { - useEffect( - () => { - Scheduler.log(`Mount A [${id}]`); - return {}; - }, - [], - () => { - if (id === 1) { - Scheduler.log('Oops!'); - throw new Error('Oops error!'); - } - Scheduler.log(`Update A [${id}]`); - }, - [id], - () => { - Scheduler.log(`Unmount A [${id}]`); - }, - ); - return ; - } - await act(async () => { - ReactNoop.render(, () => Scheduler.log('Sync effect')); - await waitFor(['Id: 0', 'Sync effect']); - expect(ReactNoop).toMatchRenderedOutput(); - ReactNoop.flushPassiveEffects(); - assertLog(['Mount A [0]']); - }); - - await expect(async () => { - await act(async () => { - // This update will trigger an error - ReactNoop.render(, () => Scheduler.log('Sync effect')); - await waitFor(['Id: 1', 'Sync effect']); - expect(ReactNoop).toMatchRenderedOutput(); - ReactNoop.flushPassiveEffects(); - assertLog(['Oops!', 'Unmount A [1]']); - expect(ReactNoop).toMatchRenderedOutput(null); - }); - }).rejects.toThrow('Oops error!'); - }); - - // @gate enableUseEffectCRUDOverload - it('handles errors in destroy on update', async () => { - function App({id, username}) { - const opts = useMemo(() => { - return {username}; - }, [username]); - useEffect( - () => { - const resource = new Resource(id, opts); - Scheduler.log(`Mount A [${id}, ${resource.opts.username}]`); - return resource; - }, - [id], - resource => { - resource.update(opts); - Scheduler.log(`Update A [${id}, ${resource.opts.username}]`); - }, - [opts], - resource => { - Scheduler.log(`Oops, ${resource.opts.username}!`); - if (id === 1) { - throw new Error(`Oops ${resource.opts.username} error!`); - } - Scheduler.log(`Unmount A [${id}, ${resource.opts.username}]`); - }, - ); - return ; - } - await act(async () => { - ReactNoop.render(, () => - Scheduler.log('Sync effect'), - ); - await waitFor(['Id: 0', 'Sync effect']); - expect(ReactNoop).toMatchRenderedOutput(); - ReactNoop.flushPassiveEffects(); - assertLog(['Mount A [0, Lauren]']); - }); - - await expect(async () => { - await act(async () => { - // This update will trigger an error during passive effect unmount - ReactNoop.render(, () => - Scheduler.log('Sync effect'), - ); - await waitFor(['Id: 1', 'Sync effect']); - expect(ReactNoop).toMatchRenderedOutput(); - ReactNoop.flushPassiveEffects(); - assertLog(['Oops, Lauren!', 'Mount A [1, Sathya]', 'Oops, Sathya!']); - }); - // TODO(lauren) more explicit assertions. this is weird because we - // destroy both the first and second resource - }).rejects.toThrow(); - - expect(ReactNoop).toMatchRenderedOutput(null); - }); - - // @gate enableUseEffectCRUDOverload && enableActivity - it('composes with activity', async () => { - function App({id, username}) { - const opts = useMemo(() => { - return {username}; - }, [username]); - useEffect( - () => { - 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; - } - - const root = ReactNoop.createRoot(); - await act(() => { - root.render( - - - , - ); - }); - assertLog([]); - - await act(() => { - root.render( - - - , - ); - }); - assertLog([]); - - await act(() => { - root.render( - - - , - ); - }); - assertLog(['create(0, Rick)']); - - await act(() => { - root.render( - - - , - ); - }); - assertLog(['update(0, Lauren)']); - - await act(() => { - root.render( - - - , - ); - }); - assertLog(['destroy(0, Lauren)']); - }); - - // @gate enableUseEffectCRUDOverload - it('composes with suspense', async () => { - function TextBox({text}) { - return ; - } - let setUsername_; - function App({id}) { - const [username, setUsername] = useState('Mofei'); - setUsername_ = setUsername; - const opts = useMemo(() => { - return {username}; - }, [username]); - useEffect( - () => { - 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 ( - <> - - }> - - - - ); - } - - await act(async () => { - ReactNoop.render(); - await waitFor([ - 'Sync: Mofei', - 'Suspend! [Mofei]', - 'Loading', - 'create(0, Mofei)', - ]); - expect(ReactNoop).toMatchRenderedOutput( - <> - - - , - ); - ReactNoop.flushPassiveEffects(); - assertLog([]); - - Scheduler.unstable_advanceTime(10); - await advanceTimers(10); - assertLog(['Promise resolved [Mofei]']); - }); - assertLog(['Mofei']); - expect(ReactNoop).toMatchRenderedOutput( - <> - - - , - ); - - await act(async () => { - ReactNoop.render(, () => Scheduler.log('Sync effect')); - await waitFor([ - 'Sync: Mofei', - 'Mofei', - 'Sync effect', - 'destroy(0, Mofei)', - 'create(1, Mofei)', - ]); - expect(ReactNoop).toMatchRenderedOutput( - <> - - - , - ); - ReactNoop.flushPassiveEffects(); - assertLog([]); - }); - - await act(async () => { - setUsername_('Lauren'); - await waitFor([ - 'Sync: Lauren', - 'Suspend! [Lauren]', - 'Loading', - 'update(1, Lauren)', - ]); - expect(ReactNoop).toMatchRenderedOutput( - <> - -