From 7bee9fbdd49aa5b9365a94b0ddf6db04bc1bf51c Mon Sep 17 00:00:00 2001 From: Andrew Clark Date: Wed, 5 Sep 2018 11:29:08 -0700 Subject: [PATCH 01/19] Initial hooks implementation Includes: - useState - useContext - useEffect - useRef - useReducer - useCallback - useMemo - useAPI --- .../src/ReactFiberBeginWork.js | 31 +- .../src/ReactFiberCommitWork.js | 170 +++ .../src/ReactFiberDispatcher.js | 16 + .../react-reconciler/src/ReactFiberHooks.js | 691 +++++++++++ .../src/ReactFiberScheduler.js | 5 + .../src/__tests__/ReactHooks-test.internal.js | 1064 +++++++++++++++++ .../ReactNewContext-test.internal.js | 27 +- packages/react/src/React.js | 19 + packages/react/src/ReactHooks.js | 83 ++ 9 files changed, 2075 insertions(+), 31 deletions(-) create mode 100644 packages/react-reconciler/src/ReactFiberHooks.js create mode 100644 packages/react-reconciler/src/__tests__/ReactHooks-test.internal.js create mode 100644 packages/react/src/ReactHooks.js diff --git a/packages/react-reconciler/src/ReactFiberBeginWork.js b/packages/react-reconciler/src/ReactFiberBeginWork.js index 5358a69310020..051729a0d8328 100644 --- a/packages/react-reconciler/src/ReactFiberBeginWork.js +++ b/packages/react-reconciler/src/ReactFiberBeginWork.js @@ -79,6 +79,7 @@ import { prepareToReadContext, calculateChangedBits, } from './ReactFiberNewContext'; +import {prepareToUseHooks, finishHooks, resetHooks} from './ReactFiberHooks'; import {stopProfilerTimerIfRunning} from './ReactProfilerTimer'; import { getMaskedContext, @@ -193,27 +194,17 @@ function forceUnmountCurrentAndReconcile( function updateForwardRef( current: Fiber | null, workInProgress: Fiber, - type: any, + Component: any, nextProps: any, renderExpirationTime: ExpirationTime, ) { - const render = type.render; + const render = Component.render; const ref = workInProgress.ref; - if (hasLegacyContextChanged()) { - // Normally we can bail out on props equality but if context has changed - // we don't do the bailout and we have to reuse existing props instead. - } else if (workInProgress.memoizedProps === nextProps) { - const currentRef = current !== null ? current.ref : null; - if (ref === currentRef) { - return bailoutOnAlreadyFinishedWork( - current, - workInProgress, - renderExpirationTime, - ); - } - } + // The rest is a fork of updateFunctionComponent let nextChildren; + prepareToReadContext(workInProgress, renderExpirationTime); + prepareToUseHooks(current, workInProgress, renderExpirationTime); if (__DEV__) { ReactCurrentOwner.current = workInProgress; ReactCurrentFiber.setCurrentPhase('render'); @@ -222,7 +213,10 @@ function updateForwardRef( } else { nextChildren = render(nextProps, ref); } + nextChildren = finishHooks(render, nextProps, nextChildren, ref); + // React DevTools reads this flag. + workInProgress.effectTag |= PerformedWork; reconcileChildren( current, workInProgress, @@ -406,6 +400,7 @@ function updateFunctionComponent( let nextChildren; prepareToReadContext(workInProgress, renderExpirationTime); + prepareToUseHooks(current, workInProgress, renderExpirationTime); if (__DEV__) { ReactCurrentOwner.current = workInProgress; ReactCurrentFiber.setCurrentPhase('render'); @@ -414,6 +409,7 @@ function updateFunctionComponent( } else { nextChildren = Component(nextProps, context); } + nextChildren = finishHooks(Component, nextProps, nextChildren, context); // React DevTools reads this flag. workInProgress.effectTag |= PerformedWork; @@ -921,6 +917,7 @@ function mountIndeterminateComponent( const context = getMaskedContext(workInProgress, unmaskedContext); prepareToReadContext(workInProgress, renderExpirationTime); + prepareToUseHooks(null, workInProgress, renderExpirationTime); let value; @@ -964,6 +961,9 @@ function mountIndeterminateComponent( // Proceed under the assumption that this is a class instance workInProgress.tag = ClassComponent; + // Throw out any hooks that were used. + resetHooks(); + // Push context providers early to prevent context stack mismatches. // During mounting we don't know the child context yet as the instance doesn't exist. // We will invalidate the child context in finishClassComponent() right after rendering. @@ -1001,6 +1001,7 @@ function mountIndeterminateComponent( } else { // Proceed under the assumption that this is a function component workInProgress.tag = FunctionComponent; + value = finishHooks(Component, props, value, context); if (__DEV__) { if (Component) { warningWithoutStack( diff --git a/packages/react-reconciler/src/ReactFiberCommitWork.js b/packages/react-reconciler/src/ReactFiberCommitWork.js index 60662d2388f3e..c8f430ec6c4b6 100644 --- a/packages/react-reconciler/src/ReactFiberCommitWork.js +++ b/packages/react-reconciler/src/ReactFiberCommitWork.js @@ -19,12 +19,15 @@ import type {FiberRoot} from './ReactFiberRoot'; import type {ExpirationTime} from './ReactFiberExpirationTime'; import type {CapturedValue, CapturedError} from './ReactCapturedValue'; import type {SuspenseState} from './ReactFiberSuspenseComponent'; +import type {FunctionComponentUpdateQueue} from './ReactFiberHooks'; import { enableSchedulerTracing, enableProfilerTimer, } from 'shared/ReactFeatureFlags'; import { + FunctionComponent, + ForwardRef, ClassComponent, HostRoot, HostComponent, @@ -180,6 +183,22 @@ function safelyDetachRef(current: Fiber) { } } +function safelyCallDestroy(current, destroy) { + if (__DEV__) { + invokeGuardedCallback(null, destroy, null); + if (hasCaughtError()) { + const error = clearCaughtError(); + captureCommitPhaseError(current, error); + } + } else { + try { + destroy(); + } catch (error) { + captureCommitPhaseError(current, error); + } + } +} + function commitBeforeMutationLifeCycles( current: Fiber | null, finishedWork: Fiber, @@ -235,6 +254,28 @@ function commitBeforeMutationLifeCycles( } } +function destroyRemainingEffects(firstToDestroy, stopAt) { + let effect = firstToDestroy; + do { + const destroy = effect.value; + if (destroy !== null) { + destroy(); + } + effect = effect.next; + } while (effect !== stopAt); +} + +function destroyMountedEffects(current) { + const oldUpdateQueue: FunctionComponentUpdateQueue | null = (current.updateQueue: any); + if (oldUpdateQueue !== null) { + const oldLastEffect = oldUpdateQueue.lastEffect; + if (oldLastEffect !== null) { + const oldFirstEffect = oldLastEffect.next; + destroyRemainingEffects(oldFirstEffect, oldFirstEffect); + } + } +} + function commitLifeCycles( finishedRoot: FiberRoot, current: Fiber | null, @@ -242,6 +283,116 @@ function commitLifeCycles( committedExpirationTime: ExpirationTime, ): void { switch (finishedWork.tag) { + case FunctionComponent: + case ForwardRef: { + const updateQueue: FunctionComponentUpdateQueue | null = (finishedWork.updateQueue: any); + if (updateQueue !== null) { + // Mount new effects and destroy the old ones by comparing to the + // current list of effects. This could be a bit simpler if we avoided + // the need to compare to the previous effect list by transferring the + // old `destroy` method to the new effect during the render phase. + // That's how I originally implemented it, but it requires an additional + // field on the effect object. + // + // This supports removing effects from the end of the list. If we adopt + // the constraint that hooks are append only, that would also save a bit + // on code size. + const newLastEffect = updateQueue.lastEffect; + if (newLastEffect !== null) { + const newFirstEffect = newLastEffect.next; + let oldLastEffect = null; + if (current !== null) { + const oldUpdateQueue: FunctionComponentUpdateQueue | null = (current.updateQueue: any); + if (oldUpdateQueue !== null) { + oldLastEffect = oldUpdateQueue.lastEffect; + } + } + if (oldLastEffect !== null) { + const oldFirstEffect = oldLastEffect.next; + let newEffect = newFirstEffect; + let oldEffect = oldFirstEffect; + + // Before mounting the new effects, unmount all the old ones. + do { + if (oldEffect !== null) { + if (newEffect.inputs !== oldEffect.inputs) { + const destroy = oldEffect.value; + if (destroy !== null) { + destroy(); + } + } + oldEffect = oldEffect.next; + if (oldEffect === oldFirstEffect) { + oldEffect = null; + } + } + newEffect = newEffect.next; + } while (newEffect !== newFirstEffect); + + // Unmount any remaining effects in the old list that do not + // appear in the new one. + if (oldEffect !== null) { + destroyRemainingEffects(oldEffect, oldFirstEffect); + } + + // Now loop through the list again to mount the new effects + oldEffect = oldFirstEffect; + do { + const create = newEffect.value; + if (oldEffect !== null) { + if (newEffect.inputs !== oldEffect.inputs) { + const newDestroy = create(); + newEffect.value = + typeof newDestroy === 'function' ? newDestroy : null; + } else { + newEffect.value = oldEffect.value; + } + oldEffect = oldEffect.next; + if (oldEffect === oldFirstEffect) { + oldEffect = null; + } + } else { + const newDestroy = create(); + newEffect.value = + typeof newDestroy === 'function' ? newDestroy : null; + } + newEffect = newEffect.next; + } while (newEffect !== newFirstEffect); + } else { + let newEffect = newFirstEffect; + do { + const create = newEffect.value; + const newDestroy = create(); + newEffect.value = + typeof newDestroy === 'function' ? newDestroy : null; + newEffect = newEffect.next; + } while (newEffect !== newFirstEffect); + } + } else if (current !== null) { + // There are no effects, which means all current effects must + // be destroyed + destroyMountedEffects(current); + } + + const callbackList = updateQueue.callbackList; + if (callbackList !== null) { + updateQueue.callbackList = null; + for (let i = 0; i < callbackList.length; i++) { + const update = callbackList[i]; + // Assume this is non-null, since otherwise it would not be part + // of the callback list. + const callback: () => mixed = (update.callback: any); + update.callback = null; + callback(); + } + } + } else if (current !== null) { + // There are no effects, which means all current effects must + // be destroyed + destroyMountedEffects(current); + } + break; + } case ClassComponent: { const instance = finishedWork.stateNode; if (finishedWork.effectTag & Update) { @@ -496,6 +647,25 @@ function commitUnmount(current: Fiber): void { onCommitUnmount(current); switch (current.tag) { + case FunctionComponent: + case ForwardRef: { + const updateQueue: FunctionComponentUpdateQueue | null = (current.updateQueue: any); + if (updateQueue !== null) { + const lastEffect = updateQueue.lastEffect; + if (lastEffect !== null) { + const firstEffect = lastEffect.next; + let effect = firstEffect; + do { + const destroy = effect.value; + if (destroy !== null) { + safelyCallDestroy(current, destroy); + } + effect = effect.next; + } while (effect !== firstEffect); + } + } + break; + } case ClassComponent: { safelyDetachRef(current); const instance = current.stateNode; diff --git a/packages/react-reconciler/src/ReactFiberDispatcher.js b/packages/react-reconciler/src/ReactFiberDispatcher.js index 354539dd79eff..9c5e790fe440c 100644 --- a/packages/react-reconciler/src/ReactFiberDispatcher.js +++ b/packages/react-reconciler/src/ReactFiberDispatcher.js @@ -8,7 +8,23 @@ */ import {readContext} from './ReactFiberNewContext'; +import { + useState, + useReducer, + useEffect, + useCallback, + useMemo, + useRef, + useAPI, +} from './ReactFiberHooks'; export const Dispatcher = { readContext, + useState, + useReducer, + useEffect, + useCallback, + useMemo, + useRef, + useAPI, }; diff --git a/packages/react-reconciler/src/ReactFiberHooks.js b/packages/react-reconciler/src/ReactFiberHooks.js new file mode 100644 index 0000000000000..66720a3707ef0 --- /dev/null +++ b/packages/react-reconciler/src/ReactFiberHooks.js @@ -0,0 +1,691 @@ +/** + * Copyright (c) 2013-present, Facebook, Inc. + * + * This source code is licensed under the MIT license found in the + * LICENSE file in the root direcreatey of this source tree. + * + * @flow + */ + +import type {Fiber} from './ReactFiber'; +import type {ExpirationTime} from './ReactFiberExpirationTime'; + +import {NoWork} from './ReactFiberExpirationTime'; +import {Callback as CallbackEffect} from 'shared/ReactSideEffectTags'; +import { + scheduleWork, + computeExpirationForFiber, + requestCurrentTime, +} from './ReactFiberScheduler'; + +import invariant from 'shared/invariant'; + +type Update = { + expirationTime: ExpirationTime, + action: A, + callback: null | (S => mixed), + next: Update | null, +}; + +type UpdateQueue = { + last: Update | null, + dispatch: any, +}; + +type Hook = { + memoizedState: any, + + baseState: any, + baseUpdate: Update | null, + queue: UpdateQueue | null, + + next: Hook | null, +}; + +type Effect = { + // For an unmounted effect, this points to the effect constructor. Once it's + // mounted, it points to a destroy function (or null). I've opted to reuse + // the same field to save memory. + value: any, + inputs: Array, + next: Effect, +}; + +export type FunctionComponentUpdateQueue = { + callbackList: Array> | null, + lastEffect: Effect | null, +}; + +type BasicStateAction = S | (S => S); + +type MaybeCallback = void | null | (S => mixed); + +type Dispatch = (A, MaybeCallback) => void; + +// These are set right before calling the component. +let renderExpirationTime: ExpirationTime = NoWork; +// The work-in-progress fiber. I've named it differently to distinguish it from +// the work-in-progress hook. +let currentlyRenderingFiber: Fiber | null = null; + +// Hooks are stored as a linked list on the fiber's memoizedState field. The +// current hook list is the list that belongs to the current fiber. The +// work-in-progress hook list is a new list that will be added to the +// work-in-progress fiber. +let firstCurrentHook: Hook | null = null; +let currentHook: Hook | null = null; +let firstWorkInProgressHook: Hook | null = null; +let workInProgressHook: Hook | null = null; + +let remainingExpirationTime: ExpirationTime = NoWork; +let componentUpdateQueue: FunctionComponentUpdateQueue | null = null; + +// Updates scheduled during render will trigger an immediate re-render at the +// end of the current pass. We can't store these updates on the normal queue, +// because if the work is aborted, they should be discarded. Because this is +// a relatively rare case, we also don't want to add an additional field to +// either the hook or queue object types. So we store them in a lazily create +// map of queue -> render-phase updates, which are discarded once the component +// completes without re-rendering. + +// Whether the work-in-progress hook is a re-rendered hook +let isReRender: boolean = false; +// Whether an update was scheduled during the currently executing render pass. +let didScheduleRenderPhaseUpdate: boolean = false; +// Lazily created map of render-phase updates +let renderPhaseUpdates: Map< + UpdateQueue, + Update, +> | null = null; +// Counter to prevent infinite loops. +let numberOfReRenders: number = 0; +const RE_RENDER_LIMIT = 25; + +function resolveCurrentlyRenderingFiber(): Fiber { + invariant( + currentlyRenderingFiber !== null, + 'Hooks can only be called inside the body of a functional component.', + ); + return currentlyRenderingFiber; +} + +export function prepareToUseHooks( + current: Fiber | null, + workInProgress: Fiber, + nextRenderExpirationTime: ExpirationTime, +): void { + renderExpirationTime = nextRenderExpirationTime; + currentlyRenderingFiber = workInProgress; + firstCurrentHook = current !== null ? current.memoizedState : null; + + // The following should have already been reset + // currentHook = null; + // workInProgressHook = null; + + // remainingExpirationTime = NoWork; + // componentUpdateQueue = null; + + // isReRender = false; + // didScheduleRenderPhaseUpdate = false; + // renderPhaseUpdates = null; + // numberOfReRenders = 0; +} + +export function finishHooks( + Component: any, + props: any, + children: any, + refOrContext: any, +): any { + // This must be called after every functional component to prevent hooks from + // being used in classes. + + while (didScheduleRenderPhaseUpdate) { + // Updates were scheduled during the render phase. They are stored in + // the `renderPhaseUpdates` map. Call the component again, reusing the + // work-in-progress hooks and applying the additional updates on top. Keep + // restarting until no more updates are scheduled. + didScheduleRenderPhaseUpdate = false; + numberOfReRenders += 1; + + // Start over from the beginning of the list + currentHook = null; + workInProgressHook = null; + componentUpdateQueue = null; + + children = Component(props, refOrContext); + } + renderPhaseUpdates = null; + numberOfReRenders = 0; + + const renderedWork: Fiber = (currentlyRenderingFiber: any); + + renderedWork.memoizedState = firstWorkInProgressHook; + renderedWork.expirationTime = remainingExpirationTime; + if (componentUpdateQueue !== null) { + renderedWork.updateQueue = (componentUpdateQueue: any); + } + + renderExpirationTime = NoWork; + currentlyRenderingFiber = null; + + firstCurrentHook = null; + currentHook = null; + firstWorkInProgressHook = null; + workInProgressHook = null; + + remainingExpirationTime = NoWork; + componentUpdateQueue = null; + + // Always set during createWorkInProgress + // isReRender = false; + + // These were reset above + // didScheduleRenderPhaseUpdate = false; + // renderPhaseUpdates = null; + // numberOfReRenders = 0; + + return children; +} + +export function resetHooks(): void { + // This is called instead of `finishHooks` if the component throws. It's also + // called inside mountIndeterminateComponent if we determine the component + // is a module-style component. + renderExpirationTime = NoWork; + currentlyRenderingFiber = null; + + firstCurrentHook = null; + currentHook = null; + firstWorkInProgressHook = null; + workInProgressHook = null; + + remainingExpirationTime = NoWork; + componentUpdateQueue = null; + + // Always set during createWorkInProgress + // isReRender = false; + + didScheduleRenderPhaseUpdate = false; + renderPhaseUpdates = null; + numberOfReRenders = 0; +} + +function createHook(): Hook { + return { + memoizedState: null, + + baseState: null, + queue: null, + baseUpdate: null, + + next: null, + }; +} + +function cloneHook(hook: Hook): Hook { + return { + memoizedState: hook.memoizedState, + + baseState: hook.memoizedState, + queue: hook.queue, + baseUpdate: hook.baseUpdate, + + next: null, + }; +} + +function createWorkInProgressHook(): Hook { + if (workInProgressHook === null) { + // This is the first hook in the list + if (firstWorkInProgressHook === null) { + isReRender = false; + currentHook = firstCurrentHook; + if (currentHook === null) { + // This is a newly mounted hook + workInProgressHook = createHook(); + } else { + // Clone the current hook. + workInProgressHook = cloneHook(currentHook); + } + firstWorkInProgressHook = workInProgressHook; + } else { + // There's already a work-in-progress. Reuse it. + isReRender = true; + currentHook = firstCurrentHook; + workInProgressHook = firstWorkInProgressHook; + } + } else { + if (workInProgressHook.next === null) { + isReRender = false; + let hook; + if (currentHook === null) { + // This is a newly mounted hook + hook = createHook(); + } else { + currentHook = currentHook.next; + if (currentHook === null) { + // This is a newly mounted hook + hook = createHook(); + } else { + // Clone the current hook. + hook = cloneHook(currentHook); + } + } + // Append to the end of the list + workInProgressHook = workInProgressHook.next = hook; + } else { + // There's already a work-in-progress. Reuse it. + isReRender = true; + workInProgressHook = workInProgressHook.next; + currentHook = currentHook !== null ? currentHook.next : null; + } + } + return workInProgressHook; +} + +function createFunctionComponentUpdateQueue(): FunctionComponentUpdateQueue { + return { + callbackList: null, + lastEffect: null, + }; +} + +function basicStateReducer(state: S, action: BasicStateAction): S { + return typeof action === 'function' ? action(state) : action; +} + +export function useState( + initialState: S | (() => S), +): [S, Dispatch>] { + return useReducer( + basicStateReducer, + // useReducer has a special case to support lazy useState initializers + (initialState: any), + ); +} + +export function useReducer( + reducer: (S, A) => S, + initialState: S, + initialAction: A | void | null, +): [S, Dispatch] { + currentlyRenderingFiber = resolveCurrentlyRenderingFiber(); + workInProgressHook = createWorkInProgressHook(); + if (isReRender) { + // This is a re-render. Apply the new render phase updates to the previous + // work-in-progress hook. + const queue: UpdateQueue = (workInProgressHook.queue: any); + const dispatch: Dispatch = (queue.dispatch: any); + if (renderPhaseUpdates !== null) { + // Render phase updates are stored in a map of queue -> linked list + const firstRenderPhaseUpdate = renderPhaseUpdates.get(queue); + if (firstRenderPhaseUpdate !== undefined) { + renderPhaseUpdates.delete(queue); + let newState = workInProgressHook.memoizedState; + let update = firstRenderPhaseUpdate; + do { + // Process this render phase update. We don't have to check the + // priority because it will always be the same as the current + // render's. + const action = update.action; + newState = reducer(newState, action); + const callback = update.callback; + if (callback !== null) { + pushCallback(currentlyRenderingFiber, update); + } + update = update.next; + } while (update !== null); + + workInProgressHook.memoizedState = newState; + + // Don't persist the state accumlated from the render phase updates to + // the base state unless the queue is empty. + // TODO: Not sure if this is the desired semantics, but it's what we + // do for gDSFP. I can't remember why. + if (workInProgressHook.baseUpdate === queue.last) { + workInProgressHook.baseState = newState; + } + + return [newState, dispatch]; + } + } + return [workInProgressHook.memoizedState, dispatch]; + } else if (currentHook !== null) { + const queue: UpdateQueue = (workInProgressHook.queue: any); + + // The last update in the entire queue + const last = queue.last; + // The last update that is part of the base state. + const baseUpdate = workInProgressHook.baseUpdate; + + // Find the first unprocessed update. + let first; + if (baseUpdate !== null) { + if (last !== null) { + // For the first update, the queue is a circular linked list where + // `queue.last.next = queue.first`. Once the first update commits, and + // the `baseUpdate` is no longer empty, we can unravel the list. + last.next = null; + } + first = baseUpdate.next; + } else { + first = last !== null ? last.next : null; + } + if (first !== null) { + let newState = workInProgressHook.baseState; + let newBaseState = null; + let newBaseUpdate = null; + let prevUpdate = baseUpdate; + let update = first; + let didSkip = false; + do { + const updateExpirationTime = update.expirationTime; + if (updateExpirationTime > renderExpirationTime) { + // Priority is insufficient. Skip this update. If this is the first + // skipped update, the previous update/state is the new base + // update/state. + if (!didSkip) { + didSkip = true; + newBaseUpdate = prevUpdate; + newBaseState = newState; + } + // Update the remaining priority in the queue. + if ( + remainingExpirationTime === NoWork || + updateExpirationTime < remainingExpirationTime + ) { + remainingExpirationTime = updateExpirationTime; + } + } else { + // Process this update. + const action = update.action; + newState = reducer(newState, action); + const callback = update.callback; + if (callback !== null) { + pushCallback(currentlyRenderingFiber, update); + } + } + prevUpdate = update; + update = update.next; + } while (update !== null && update !== first); + + if (!didSkip) { + newBaseUpdate = prevUpdate; + newBaseState = newState; + } + + workInProgressHook.memoizedState = newState; + workInProgressHook.baseUpdate = newBaseUpdate; + workInProgressHook.baseState = newBaseState; + } + + const dispatch: Dispatch = (queue.dispatch: any); + return [workInProgressHook.memoizedState, dispatch]; + } else { + if (reducer === basicStateReducer) { + // Special case for `useState`. + if (typeof initialState === 'function') { + initialState = initialState(); + } + } else if (initialAction !== undefined && initialAction !== null) { + initialState = reducer(initialState, initialAction); + } + workInProgressHook.memoizedState = workInProgressHook.baseState = initialState; + const queue: UpdateQueue = (workInProgressHook.queue = { + last: null, + dispatch: null, + }); + const dispatch: Dispatch = (queue.dispatch = (dispatchAction.bind( + null, + currentlyRenderingFiber, + queue, + ): any)); + return [workInProgressHook.memoizedState, dispatch]; + } +} + +function pushCallback(workInProgress: Fiber, update: Update): void { + if (componentUpdateQueue === null) { + componentUpdateQueue = createFunctionComponentUpdateQueue(); + componentUpdateQueue.callbackList = [update]; + } else { + const callbackList = componentUpdateQueue.callbackList; + if (callbackList === null) { + componentUpdateQueue.callbackList = [update]; + } else { + callbackList.push(update); + } + } + workInProgress.effectTag |= CallbackEffect; +} + +function pushEffect(value, inputs) { + const effect: Effect = { + value, + inputs, + // Circular + next: (null: any), + }; + if (componentUpdateQueue === null) { + componentUpdateQueue = createFunctionComponentUpdateQueue(); + componentUpdateQueue.lastEffect = effect.next = effect; + } else { + const lastEffect = componentUpdateQueue.lastEffect; + if (lastEffect === null) { + componentUpdateQueue.lastEffect = effect.next = effect; + } else { + const firstEffect = lastEffect.next; + lastEffect.next = effect; + effect.next = firstEffect; + componentUpdateQueue.lastEffect = effect; + } + } + return effect; +} + +export function useRef(initialValue: T): {current: T} { + currentlyRenderingFiber = resolveCurrentlyRenderingFiber(); + workInProgressHook = createWorkInProgressHook(); + let ref; + if (currentHook === null) { + ref = {current: initialValue}; + if (__DEV__) { + Object.seal(ref); + } + workInProgressHook.memoizedState = ref; + } else { + ref = workInProgressHook.memoizedState; + } + return ref; +} + +export function useEffect( + create: () => mixed, + inputs: Array | void | null, +): void { + currentlyRenderingFiber = resolveCurrentlyRenderingFiber(); + workInProgressHook = createWorkInProgressHook(); + + let nextEffect; + let nextInputs = inputs !== undefined && inputs !== null ? inputs : [create]; + if (currentHook !== null) { + const prevEffect = currentHook.memoizedState; + const prevInputs = prevEffect.inputs; + if (inputsAreEqual(nextInputs, prevInputs)) { + nextEffect = pushEffect(prevEffect.value, prevInputs); + } else { + nextEffect = pushEffect(create, nextInputs); + } + } else { + nextEffect = pushEffect(create, nextInputs); + } + + // TODO: If we decide not to support removing hooks from the end of the list, + // we only need to schedule an effect if the inputs changed. + currentlyRenderingFiber.effectTag |= CallbackEffect; + workInProgressHook.memoizedState = nextEffect; +} + +export function useAPI( + ref: {current: T | null} | ((inst: T | null) => mixed) | null | void, + create: () => T, + inputs: Array | void | null, +): void { + // TODO: If inputs are provided, should we skip comparing the ref itself? + const nextInputs = + inputs !== null && inputs !== undefined + ? inputs.concat([ref]) + : [ref, create]; + + // TODO: I've implemented this on top of useEffect because it's almost the + // same thing, and it would require an equal amount of code. It doesn't seem + // like a common enough use case to justify the additional size. + useEffect(() => { + if (typeof ref === 'function') { + const refCallback = ref; + const inst = create(); + refCallback(inst); + return () => refCallback(null); + } else if (ref !== null && ref !== undefined) { + const refObject = ref; + const inst = create(); + refObject.current = inst; + return () => { + refObject.current = null; + }; + } + }, nextInputs); +} + +export function useCallback( + callback: T, + inputs: Array | void | null, +): T { + currentlyRenderingFiber = resolveCurrentlyRenderingFiber(); + workInProgressHook = createWorkInProgressHook(); + + const nextInputs = + inputs !== undefined && inputs !== null ? inputs : [callback]; + + if (currentHook !== null) { + const prevState = currentHook.memoizedState; + const prevInputs = prevState[1]; + if (inputsAreEqual(nextInputs, prevInputs)) { + return prevState[0]; + } + } + + workInProgressHook.memoizedState = [callback, nextInputs]; + return callback; +} + +export function useMemo( + nextCreate: () => T, + inputs: Array | void | null, +): T { + currentlyRenderingFiber = resolveCurrentlyRenderingFiber(); + workInProgressHook = createWorkInProgressHook(); + + const nextInputs = + inputs !== undefined && inputs !== null ? inputs : [nextCreate]; + + if (currentHook !== null) { + const prevState = currentHook.memoizedState; + const prevInputs = prevState[1]; + if (inputsAreEqual(nextInputs, prevInputs)) { + return prevState[0]; + } + } + + const nextValue = nextCreate(); + workInProgressHook.memoizedState = [nextValue, nextInputs]; + return nextValue; +} + +function dispatchAction( + fiber: Fiber, + queue: UpdateQueue, + action: A, + callback: void | null | (S => mixed), +) { + invariant( + numberOfReRenders < RE_RENDER_LIMIT, + 'Too many re-renders. React limits the number of renders to prevent ' + + 'an infinite loop.', + ); + + const alternate = fiber.alternate; + if ( + fiber === currentlyRenderingFiber || + (alternate !== null && alternate === currentlyRenderingFiber) + ) { + // This is a render phase update. Stash it in a lazily-created map of + // queue -> linked list of updates. After this render pass, we'll restart + // and apply the stashed updates on top of the work-in-progress hook. + didScheduleRenderPhaseUpdate = true; + const update: Update = { + expirationTime: renderExpirationTime, + action, + callback: callback !== undefined ? callback : null, + next: null, + }; + if (renderPhaseUpdates === null) { + renderPhaseUpdates = new Map(); + } + const firstRenderPhaseUpdate = renderPhaseUpdates.get(queue); + if (firstRenderPhaseUpdate === undefined) { + renderPhaseUpdates.set(queue, update); + } else { + // Append the update to the end of the list. + let lastRenderPhaseUpdate = firstRenderPhaseUpdate; + while (lastRenderPhaseUpdate.next !== null) { + lastRenderPhaseUpdate = lastRenderPhaseUpdate.next; + } + lastRenderPhaseUpdate.next = update; + } + } else { + const currentTime = requestCurrentTime(); + const expirationTime = computeExpirationForFiber(currentTime, fiber); + const update: Update = { + expirationTime, + action, + callback: callback !== undefined ? callback : null, + next: null, + }; + // Append the update to the end of the list. + const last = queue.last; + if (last === null) { + // This is the first update. Create a circular list. + update.next = update; + } else { + const first = last.next; + if (first !== null) { + // Still circular. + update.next = first; + } + last.next = update; + } + queue.last = update; + scheduleWork(fiber, expirationTime); + } +} + +function inputsAreEqual(arr1, arr2) { + // Don't bother comparing lengths because these arrays are always + // passed inline. + for (let i = 0; i < arr1.length; i++) { + // Inlined Object.is polyfill. + // https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Global_Objects/Object/is + const val1 = arr1[i]; + const val2 = arr2[i]; + if ( + (val1 === val2 && (val1 !== 0 || 1 / val1 === 1 / (val2: any))) || + (val1 !== val1 && val2 !== val2) // eslint-disable-line no-self-compare + ) { + continue; + } + return false; + } + return true; +} diff --git a/packages/react-reconciler/src/ReactFiberScheduler.js b/packages/react-reconciler/src/ReactFiberScheduler.js index 34be16bdb2525..11f2c7a1b6c9c 100644 --- a/packages/react-reconciler/src/ReactFiberScheduler.js +++ b/packages/react-reconciler/src/ReactFiberScheduler.js @@ -122,6 +122,7 @@ import { popContext as popLegacyContext, } from './ReactFiberContext'; import {popProvider, resetContextDependences} from './ReactFiberNewContext'; +import {resetHooks} from './ReactFiberHooks'; import {popHostContext, popHostContainer} from './ReactFiberHostContext'; import { recordCommitTime, @@ -1222,6 +1223,9 @@ function renderRoot( try { workLoop(isYieldy); } catch (thrownValue) { + resetContextDependences(); + resetHooks(); + if (nextUnitOfWork === null) { // This is a fatal error. didFatal = true; @@ -1284,6 +1288,7 @@ function renderRoot( isWorking = false; ReactCurrentOwner.currentDispatcher = null; resetContextDependences(); + resetHooks(); // Yield back to main thread. if (didFatal) { diff --git a/packages/react-reconciler/src/__tests__/ReactHooks-test.internal.js b/packages/react-reconciler/src/__tests__/ReactHooks-test.internal.js new file mode 100644 index 0000000000000..5bcf446214020 --- /dev/null +++ b/packages/react-reconciler/src/__tests__/ReactHooks-test.internal.js @@ -0,0 +1,1064 @@ +/** + * Copyright (c) 2013-present, Facebook, Inc. + * + * This source code is licensed under the MIT license found in the + * LICENSE file in the root directory of this source tree. + * + * @emails react-core + * @jest-environment node + */ + +/* eslint-disable no-func-assign */ + +'use strict'; + +let React; +let ReactFeatureFlags; +let ReactNoop; +let useState; +let useReducer; +let useEffect; +let useCallback; +let useMemo; +let useRef; +let useAPI; +let forwardRef; + +describe('ReactHooks', () => { + beforeEach(() => { + jest.resetModules(); + ReactFeatureFlags = require('shared/ReactFeatureFlags'); + ReactFeatureFlags.debugRenderPhaseSideEffectsForStrictMode = false; + React = require('react'); + ReactNoop = require('react-noop-renderer'); + useState = React.useState; + useReducer = React.useReducer; + useEffect = React.useEffect; + useCallback = React.useCallback; + useMemo = React.useMemo; + useRef = React.useRef; + useAPI = React.useAPI; + forwardRef = React.forwardRef; + }); + + function span(prop) { + return {type: 'span', hidden: false, children: [], prop}; + } + + function Text(props) { + ReactNoop.yield(props.text); + return ; + } + + it('resumes after an interruption', () => { + function Counter(props, ref) { + const [count, updateCount] = useState(0); + useAPI(ref, () => ({updateCount})); + return ; + } + Counter = forwardRef(Counter); + + // Initial mount + const counter = React.createRef(null); + ReactNoop.render(); + expect(ReactNoop.flush()).toEqual(['Count: 0']); + expect(ReactNoop.getChildren()).toEqual([span('Count: 0')]); + + // Schedule some updates + counter.current.updateCount(1); + counter.current.updateCount(count => count + 10); + // Partially flush without committing + ReactNoop.flushThrough(['Count: 11']); + expect(ReactNoop.getChildren()).toEqual([span('Count: 0')]); + + // Interrupt with a high priority update + ReactNoop.flushSync(() => { + ReactNoop.render(); + }); + expect(ReactNoop.clearYields()).toEqual(['Total: 0']); + + // Resume rendering + ReactNoop.flush(); + expect(ReactNoop.getChildren()).toEqual([span('Total: 11')]); + }); + + it('throws inside class components', () => { + class BadCounter extends React.Component { + render() { + const [count] = useState(0); + return ; + } + } + ReactNoop.render(); + + expect(() => ReactNoop.flush()).toThrow( + 'Hooks can only be called inside the body of a functional component.', + ); + + // Confirm that a subsequent hook works properly. + function GoodCounter(props, ref) { + const [count] = useState(props.initialCount); + return ; + } + ReactNoop.render(); + expect(ReactNoop.flush()).toEqual([10]); + }); + + it('throws inside module-style components', () => { + function Counter() { + return { + render() { + const [count] = useState(0); + return ; + }, + }; + } + ReactNoop.render(); + expect(() => ReactNoop.flush()).toThrow( + 'Hooks can only be called inside the body of a functional component.', + ); + + // Confirm that a subsequent hook works properly. + function GoodCounter(props) { + const [count] = useState(props.initialCount); + return ; + } + ReactNoop.render(); + expect(ReactNoop.flush()).toEqual([10]); + }); + + it('throws when called outside the render phase', () => { + expect(() => useState(0)).toThrow( + 'Hooks can only be called inside the body of a functional component.', + ); + }); + + describe('useState', () => { + it('simple mount and update', () => { + function Counter(props, ref) { + const [count, updateCount] = useState(0); + useAPI(ref, () => ({updateCount})); + return ; + } + Counter = forwardRef(Counter); + const counter = React.createRef(null); + ReactNoop.render(); + ReactNoop.flush(); + expect(ReactNoop.getChildren()).toEqual([span('Count: 0')]); + + counter.current.updateCount(1); + ReactNoop.flush(); + expect(ReactNoop.getChildren()).toEqual([span('Count: 1')]); + + counter.current.updateCount(count => count + 10); + ReactNoop.flush(); + expect(ReactNoop.getChildren()).toEqual([span('Count: 11')]); + }); + + it('lazy state initializer', () => { + function Counter(props, ref) { + const [count, updateCount] = useState(() => { + ReactNoop.yield('getInitialState'); + return props.initialState; + }); + useAPI(ref, () => ({updateCount})); + return ; + } + Counter = forwardRef(Counter); + const counter = React.createRef(null); + ReactNoop.render(); + expect(ReactNoop.flush()).toEqual(['getInitialState', 'Count: 42']); + expect(ReactNoop.getChildren()).toEqual([span('Count: 42')]); + + counter.current.updateCount(7); + expect(ReactNoop.flush()).toEqual(['Count: 7']); + expect(ReactNoop.getChildren()).toEqual([span('Count: 7')]); + }); + + it('multiple states', () => { + function Counter(props, ref) { + const [count, updateCount] = useState(0); + const [label, updateLabel] = useState('Count'); + useAPI(ref, () => ({updateCount, updateLabel})); + return ; + } + Counter = forwardRef(Counter); + const counter = React.createRef(null); + ReactNoop.render(); + ReactNoop.flush(); + expect(ReactNoop.getChildren()).toEqual([span('Count: 0')]); + + counter.current.updateCount(7); + expect(ReactNoop.flush()).toEqual(['Count: 7']); + + counter.current.updateLabel('Total'); + expect(ReactNoop.flush()).toEqual(['Total: 7']); + }); + + it('callbacks', () => { + function Counter(props, ref) { + const [count, updateCount] = useState(0); + useAPI(ref, () => ({updateCount})); + return ; + } + Counter = forwardRef(Counter); + const counter = React.createRef(null); + ReactNoop.render(); + ReactNoop.flush(); + expect(ReactNoop.getChildren()).toEqual([span('Count: 0')]); + + counter.current.updateCount(7, count => { + ReactNoop.yield(`Did update count`); + }); + expect(ReactNoop.flush()).toEqual(['Count: 7', 'Did update count']); + + // Update twice in the same batch + counter.current.updateCount(1, () => { + ReactNoop.yield(`Did update count (first callback)`); + }); + counter.current.updateCount(2, () => { + ReactNoop.yield(`Did update count (second callback)`); + }); + expect(ReactNoop.flush()).toEqual([ + // Component only renders once + 'Count: 2', + 'Did update count (first callback)', + 'Did update count (second callback)', + ]); + }); + + it('does not fire callbacks more than once when rebasing', () => { + function Counter(props, ref) { + const [count, updateCount] = useState(0); + useAPI(ref, () => ({updateCount})); + return ; + } + Counter = forwardRef(Counter); + const counter = React.createRef(null); + ReactNoop.render(); + ReactNoop.flush(); + expect(ReactNoop.getChildren()).toEqual([span('Count: 0')]); + + counter.current.updateCount(1, count => { + ReactNoop.yield(`Did update count (low pri)`); + }); + ReactNoop.flushSync(() => { + counter.current.updateCount(2, count => { + ReactNoop.yield(`Did update count (high pri)`); + }); + }); + expect(ReactNoop.clearYields()).toEqual([ + 'Count: 2', + 'Did update count (high pri)', + ]); + // The high-pri update is processed again when we render at low priority, + // but its callback should not fire again. + expect(ReactNoop.flush()).toEqual([ + 'Count: 2', + 'Did update count (low pri)', + ]); + }); + + it('returns the same updater function every time', () => { + let updaters = []; + function Counter() { + const [count, updateCount] = useState(0); + updaters.push(updateCount); + return ; + } + ReactNoop.render(); + ReactNoop.flush(); + expect(ReactNoop.getChildren()).toEqual([span('Count: 0')]); + + updaters[0](1); + ReactNoop.flush(); + expect(ReactNoop.getChildren()).toEqual([span('Count: 1')]); + + updaters[0](count => count + 10); + ReactNoop.flush(); + expect(ReactNoop.getChildren()).toEqual([span('Count: 11')]); + + expect(updaters).toEqual([updaters[0], updaters[0], updaters[0]]); + }); + }); + + describe('updates during the render phase', () => { + it('restarts the render function and applies the new updates on top', () => { + function ScrollView({row: newRow}) { + let [isScrollingDown, setIsScrollingDown] = useState(false); + let [row, setRow] = useState(null); + + if (row !== newRow) { + // Row changed since last render. Update isScrollingDown. + setIsScrollingDown(row !== null && newRow > row); + setRow(newRow); + } + + return ; + } + + ReactNoop.render(); + ReactNoop.flush(); + expect(ReactNoop.getChildren()).toEqual([span('Scrolling down: false')]); + + ReactNoop.render(); + ReactNoop.flush(); + expect(ReactNoop.getChildren()).toEqual([span('Scrolling down: true')]); + + ReactNoop.render(); + ReactNoop.flush(); + expect(ReactNoop.getChildren()).toEqual([span('Scrolling down: true')]); + + ReactNoop.render(); + ReactNoop.flush(); + expect(ReactNoop.getChildren()).toEqual([span('Scrolling down: true')]); + + ReactNoop.render(); + ReactNoop.flush(); + expect(ReactNoop.getChildren()).toEqual([span('Scrolling down: false')]); + + ReactNoop.render(); + ReactNoop.flush(); + expect(ReactNoop.getChildren()).toEqual([span('Scrolling down: false')]); + }); + + it('keeps restarting until there are no more new updates', () => { + function Counter({row: newRow}) { + let [count, setCount] = useState(0); + if (count < 3) { + setCount(count + 1); + } + ReactNoop.yield('Render: ' + count); + return ; + } + + ReactNoop.render(); + expect(ReactNoop.flush()).toEqual([ + 'Render: 0', + 'Render: 1', + 'Render: 2', + 'Render: 3', + 3, + ]); + expect(ReactNoop.getChildren()).toEqual([span(3)]); + }); + + it('updates multiple times within same render function', () => { + function Counter({row: newRow}) { + let [count, setCount] = useState(0); + if (count < 12) { + setCount(c => c + 1); + setCount(c => c + 1); + setCount(c => c + 1); + } + ReactNoop.yield('Render: ' + count); + return ; + } + + ReactNoop.render(); + expect(ReactNoop.flush()).toEqual([ + // Should increase by three each time + 'Render: 0', + 'Render: 3', + 'Render: 6', + 'Render: 9', + 'Render: 12', + 12, + ]); + expect(ReactNoop.getChildren()).toEqual([span(12)]); + }); + + it('throws after too many iterations', () => { + function Counter({row: newRow}) { + let [count, setCount] = useState(0); + setCount(count + 1); + ReactNoop.yield('Render: ' + count); + return ; + } + ReactNoop.render(); + expect(() => ReactNoop.flush()).toThrow( + 'Too many re-renders. React limits the number of renders to prevent ' + + 'an infinite loop.', + ); + }); + + it('works with useReducer', () => { + function reducer(state, action) { + return action === 'increment' ? state + 1 : state; + } + function Counter({row: newRow}) { + let [count, dispatch] = useReducer(reducer, 0); + if (count < 3) { + dispatch('increment'); + } + ReactNoop.yield('Render: ' + count); + return ; + } + + ReactNoop.render(); + expect(ReactNoop.flush()).toEqual([ + 'Render: 0', + 'Render: 1', + 'Render: 2', + 'Render: 3', + 3, + ]); + expect(ReactNoop.getChildren()).toEqual([span(3)]); + }); + + it('uses reducer passed at time of render, not time of dispatch', () => { + // This test is a bit contrived but it demonstrates a subtle edge case. + + // Reducer A increments by 1. Reducer B increments by 10. + function reducerA(state, action) { + switch (action) { + case 'increment': + return state + 1; + case 'reset': + return 0; + } + } + function reducerB(state, action) { + switch (action) { + case 'increment': + return state + 10; + case 'reset': + return 0; + } + } + + function Counter({row: newRow}, ref) { + let [reducer, setReducer] = useState(() => reducerA); + let [count, dispatch] = useReducer(reducer, 0); + useAPI(ref, () => ({dispatch})); + if (count < 20) { + dispatch('increment'); + // Swap reducers each time we increment + if (reducer === reducerA) { + setReducer(() => reducerB); + } else { + setReducer(() => reducerA); + } + } + ReactNoop.yield('Render: ' + count); + return ; + } + Counter = forwardRef(Counter); + const counter = React.createRef(null); + ReactNoop.render(); + expect(ReactNoop.flush()).toEqual([ + // The count should increase by alternating amounts of 10 and 1 + // until we reach 21. + 'Render: 0', + 'Render: 10', + 'Render: 11', + 'Render: 21', + 21, + ]); + expect(ReactNoop.getChildren()).toEqual([span(21)]); + + // Test that it works on update, too. This time the log is a bit different + // because we started with reducerB instead of reducerA. + counter.current.dispatch('reset'); + ReactNoop.render(); + expect(ReactNoop.flush()).toEqual([ + 'Render: 0', + 'Render: 1', + 'Render: 11', + 'Render: 12', + 'Render: 22', + 22, + ]); + expect(ReactNoop.getChildren()).toEqual([span(22)]); + }); + }); + + describe('useReducer', () => { + it('simple mount and update', () => { + const INCREMENT = 'INCREMENT'; + const DECREMENT = 'DECREMENT'; + + function reducer(state, action) { + switch (action) { + case 'INCREMENT': + return state + 1; + case 'DECREMENT': + return state - 1; + default: + return state; + } + } + + function Counter(props, ref) { + const [count, dispatch] = useReducer(reducer, 0); + useAPI(ref, () => ({dispatch})); + return ; + } + Counter = forwardRef(Counter); + const counter = React.createRef(null); + ReactNoop.render(); + ReactNoop.flush(); + expect(ReactNoop.getChildren()).toEqual([span('Count: 0')]); + + counter.current.dispatch(INCREMENT); + ReactNoop.flush(); + expect(ReactNoop.getChildren()).toEqual([span('Count: 1')]); + + counter.current.dispatch(DECREMENT); + counter.current.dispatch(DECREMENT); + counter.current.dispatch(DECREMENT); + ReactNoop.flush(); + expect(ReactNoop.getChildren()).toEqual([span('Count: -2')]); + }); + + it('accepts an initial action', () => { + const INCREMENT = 'INCREMENT'; + const DECREMENT = 'DECREMENT'; + + function reducer(state, action) { + switch (action) { + case 'INITIALIZE': + return 10; + case 'INCREMENT': + return state + 1; + case 'DECREMENT': + return state - 1; + default: + return state; + } + } + + const initialAction = 'INITIALIZE'; + + function Counter(props, ref) { + const [count, dispatch] = useReducer(reducer, 0, initialAction); + useAPI(ref, () => ({dispatch})); + return ; + } + Counter = forwardRef(Counter); + const counter = React.createRef(null); + ReactNoop.render(); + ReactNoop.flush(); + expect(ReactNoop.getChildren()).toEqual([span('Count: 10')]); + + counter.current.dispatch(INCREMENT); + ReactNoop.flush(); + expect(ReactNoop.getChildren()).toEqual([span('Count: 11')]); + + counter.current.dispatch(DECREMENT); + counter.current.dispatch(DECREMENT); + counter.current.dispatch(DECREMENT); + ReactNoop.flush(); + expect(ReactNoop.getChildren()).toEqual([span('Count: 8')]); + }); + }); + + describe('useEffect', () => { + it('simple mount and update', () => { + function Counter(props) { + useEffect(() => { + ReactNoop.yield(`Did commit [${props.count}]`); + }); + return ; + } + ReactNoop.render(); + expect(ReactNoop.flush()).toEqual(['Count: 0', 'Did commit [0]']); + expect(ReactNoop.getChildren()).toEqual([span('Count: 0')]); + + ReactNoop.render(); + expect(ReactNoop.flush()).toEqual(['Count: 1', 'Did commit [1]']); + expect(ReactNoop.getChildren()).toEqual([span('Count: 1')]); + }); + + it('unmounts previous effect', () => { + function Counter(props) { + useEffect(() => { + ReactNoop.yield(`Did create [${props.count}]`); + return () => { + ReactNoop.yield(`Did destroy [${props.count}]`); + }; + }); + return ; + } + ReactNoop.render(); + expect(ReactNoop.flush()).toEqual(['Count: 0', 'Did create [0]']); + expect(ReactNoop.getChildren()).toEqual([span('Count: 0')]); + + ReactNoop.render(); + expect(ReactNoop.flush()).toEqual([ + 'Count: 1', + 'Did destroy [0]', + 'Did create [1]', + ]); + expect(ReactNoop.getChildren()).toEqual([span('Count: 1')]); + }); + + it('unmounts on deletion', () => { + function Counter(props) { + useEffect(() => { + ReactNoop.yield(`Did create [${props.count}]`); + return () => { + ReactNoop.yield(`Did destroy [${props.count}]`); + }; + }); + return ; + } + ReactNoop.render(); + expect(ReactNoop.flush()).toEqual(['Count: 0', 'Did create [0]']); + expect(ReactNoop.getChildren()).toEqual([span('Count: 0')]); + + ReactNoop.render(null); + expect(ReactNoop.flush()).toEqual(['Did destroy [0]']); + expect(ReactNoop.getChildren()).toEqual([]); + }); + + it('skips effect if constructor has not changed', () => { + function effect() { + ReactNoop.yield(`Did mount`); + return () => { + ReactNoop.yield(`Did unmount`); + }; + } + function Counter(props) { + useEffect(effect); + return ; + } + ReactNoop.render(); + expect(ReactNoop.flush()).toEqual(['Count: 0', 'Did mount']); + expect(ReactNoop.getChildren()).toEqual([span('Count: 0')]); + + ReactNoop.render(); + // No effect, because constructor was hoisted outside render + expect(ReactNoop.flush()).toEqual(['Count: 1']); + expect(ReactNoop.getChildren()).toEqual([span('Count: 1')]); + + ReactNoop.render(null); + expect(ReactNoop.flush()).toEqual(['Did unmount']); + expect(ReactNoop.getChildren()).toEqual([]); + }); + + it('skips effect if inputs have not changed', () => { + function Counter(props) { + const text = `${props.label}: ${props.count}`; + useEffect( + () => { + ReactNoop.yield(`Did create [${text}]`); + return () => { + ReactNoop.yield(`Did destroy [${text}]`); + }; + }, + [props.label, props.count], + ); + return ; + } + ReactNoop.render(); + expect(ReactNoop.flush()).toEqual(['Count: 0', 'Did create [Count: 0]']); + expect(ReactNoop.getChildren()).toEqual([span('Count: 0')]); + + ReactNoop.render(); + // Count changed + expect(ReactNoop.flush()).toEqual([ + 'Count: 1', + 'Did destroy [Count: 0]', + 'Did create [Count: 1]', + ]); + expect(ReactNoop.getChildren()).toEqual([span('Count: 1')]); + + ReactNoop.render(); + // Nothing changed, so no effect should have fired + expect(ReactNoop.flush()).toEqual(['Count: 1']); + expect(ReactNoop.getChildren()).toEqual([span('Count: 1')]); + + ReactNoop.render(); + // Label changed + expect(ReactNoop.flush()).toEqual([ + 'Total: 1', + 'Did destroy [Count: 1]', + 'Did create [Total: 1]', + ]); + expect(ReactNoop.getChildren()).toEqual([span('Total: 1')]); + }); + + it('multiple effects', () => { + function Counter(props) { + useEffect(() => { + ReactNoop.yield(`Did commit 1 [${props.count}]`); + }); + useEffect(() => { + ReactNoop.yield(`Did commit 2 [${props.count}]`); + }); + return ; + } + ReactNoop.render(); + expect(ReactNoop.flush()).toEqual([ + 'Count: 0', + 'Did commit 1 [0]', + 'Did commit 2 [0]', + ]); + expect(ReactNoop.getChildren()).toEqual([span('Count: 0')]); + + ReactNoop.render(); + expect(ReactNoop.flush()).toEqual([ + 'Count: 1', + 'Did commit 1 [1]', + 'Did commit 2 [1]', + ]); + expect(ReactNoop.getChildren()).toEqual([span('Count: 1')]); + }); + + it('unmounts all previous effects before creating any new ones', () => { + function Counter(props) { + useEffect(() => { + ReactNoop.yield(`Mount A [${props.count}]`); + return () => { + ReactNoop.yield(`Unmount A [${props.count}]`); + }; + }); + useEffect(() => { + ReactNoop.yield(`Mount B [${props.count}]`); + return () => { + ReactNoop.yield(`Unmount B [${props.count}]`); + }; + }); + return ; + } + ReactNoop.render(); + expect(ReactNoop.flush()).toEqual([ + 'Count: 0', + 'Mount A [0]', + 'Mount B [0]', + ]); + expect(ReactNoop.getChildren()).toEqual([span('Count: 0')]); + + ReactNoop.render(); + expect(ReactNoop.flush()).toEqual([ + 'Count: 1', + 'Unmount A [0]', + 'Unmount B [0]', + 'Mount A [1]', + 'Mount B [1]', + ]); + expect(ReactNoop.getChildren()).toEqual([span('Count: 1')]); + }); + }); + + describe('useCallback', () => { + it('memoizes callback by comparing inputs', () => { + class IncrementButton extends React.PureComponent { + increment = () => { + this.props.increment(); + }; + render() { + return ; + } + } + + function Counter({incrementBy}) { + const [count, updateCount] = useState(0); + const increment = useCallback(() => updateCount(c => c + incrementBy), [ + incrementBy, + ]); + return ( + + + + + ); + } + + const button = React.createRef(null); + ReactNoop.render(); + expect(ReactNoop.flush()).toEqual(['Increment', 'Count: 0']); + expect(ReactNoop.getChildren()).toEqual([ + span('Increment'), + span('Count: 0'), + ]); + + button.current.increment(); + expect(ReactNoop.flush()).toEqual([ + // Button should not re-render, because its props haven't changed + // 'Increment', + 'Count: 1', + ]); + expect(ReactNoop.getChildren()).toEqual([ + span('Increment'), + span('Count: 1'), + ]); + + // Increase the increment amount + ReactNoop.render(); + expect(ReactNoop.flush()).toEqual([ + // Inputs did change this time + 'Increment', + 'Count: 1', + ]); + expect(ReactNoop.getChildren()).toEqual([ + span('Increment'), + span('Count: 1'), + ]); + + // Callback should have updated + button.current.increment(); + expect(ReactNoop.flush()).toEqual(['Count: 11']); + expect(ReactNoop.getChildren()).toEqual([ + span('Increment'), + span('Count: 11'), + ]); + }); + }); + + describe('useMemo', () => { + it('memoizes value by comparing to previous inputs', () => { + function CapitalizedText(props) { + const text = props.text; + const capitalizedText = useMemo( + () => { + ReactNoop.yield(`Capitalize '${text}'`); + return text.toUpperCase(); + }, + [text], + ); + return ; + } + + ReactNoop.render(); + expect(ReactNoop.flush()).toEqual(["Capitalize 'hello'", 'HELLO']); + expect(ReactNoop.getChildren()).toEqual([span('HELLO')]); + + ReactNoop.render(); + expect(ReactNoop.flush()).toEqual(["Capitalize 'hi'", 'HI']); + expect(ReactNoop.getChildren()).toEqual([span('HI')]); + + ReactNoop.render(); + expect(ReactNoop.flush()).toEqual(['HI']); + expect(ReactNoop.getChildren()).toEqual([span('HI')]); + + ReactNoop.render(); + expect(ReactNoop.flush()).toEqual(["Capitalize 'goodbye'", 'GOODBYE']); + expect(ReactNoop.getChildren()).toEqual([span('GOODBYE')]); + }); + + it('compares function if no inputs are provided', () => { + function LazyCompute(props) { + const computed = useMemo(props.compute); + return ; + } + + function computeA() { + ReactNoop.yield('compute A'); + return 'A'; + } + + function computeB() { + ReactNoop.yield('compute B'); + return 'B'; + } + + ReactNoop.render(); + expect(ReactNoop.flush()).toEqual(['compute A', 'A']); + + ReactNoop.render(); + expect(ReactNoop.flush()).toEqual(['A']); + + ReactNoop.render(); + expect(ReactNoop.flush()).toEqual(['A']); + + ReactNoop.render(); + expect(ReactNoop.flush()).toEqual(['compute B', 'B']); + }); + }); + + describe('useRef', () => { + it('creates a ref object initialized with the provided value', () => { + jest.useFakeTimers(); + + function useDebouncedCallback(callback, ms, inputs) { + const timeoutID = useRef(-1); + useEffect(() => { + return function unmount() { + clearTimeout(timeoutID.current); + }; + }, []); + const debouncedCallback = useCallback( + (...args) => { + clearTimeout(timeoutID.current); + timeoutID.current = setTimeout(callback, ms, ...args); + }, + [callback, ms], + ); + return useCallback(debouncedCallback, inputs); + } + + let ping; + function App() { + ping = useDebouncedCallback( + value => { + ReactNoop.yield('ping: ' + value); + }, + 100, + [], + ); + return null; + } + + ReactNoop.render(); + expect(ReactNoop.flush()).toEqual([]); + + ping(1); + ping(2); + ping(3); + + expect(ReactNoop.flush()).toEqual([]); + + jest.advanceTimersByTime(100); + + expect(ReactNoop.flush()).toEqual(['ping: 3']); + + ping(4); + jest.advanceTimersByTime(20); + ping(5); + ping(6); + jest.advanceTimersByTime(80); + + expect(ReactNoop.flush()).toEqual([]); + + jest.advanceTimersByTime(20); + expect(ReactNoop.flush()).toEqual(['ping: 6']); + }); + }); + + describe('progressive enhancement', () => { + it('mount additional state', () => { + let updateA; + let updateB; + let updateC; + + function App(props) { + const [A, _updateA] = useState(0); + const [B, _updateB] = useState(0); + updateA = _updateA; + updateB = _updateB; + + let C; + if (props.loadC) { + const [_C, _updateC] = useState(0); + C = _C; + updateC = _updateC; + } else { + C = '[not loaded]'; + } + + return ; + } + + ReactNoop.render(); + expect(ReactNoop.flush()).toEqual(['A: 0, B: 0, C: [not loaded]']); + expect(ReactNoop.getChildren()).toEqual([ + span('A: 0, B: 0, C: [not loaded]'), + ]); + + updateA(2); + updateB(3); + expect(ReactNoop.flush()).toEqual(['A: 2, B: 3, C: [not loaded]']); + expect(ReactNoop.getChildren()).toEqual([ + span('A: 2, B: 3, C: [not loaded]'), + ]); + + ReactNoop.render(); + expect(ReactNoop.flush()).toEqual(['A: 2, B: 3, C: 0']); + expect(ReactNoop.getChildren()).toEqual([span('A: 2, B: 3, C: 0')]); + + updateC(4); + expect(ReactNoop.flush()).toEqual(['A: 2, B: 3, C: 4']); + expect(ReactNoop.getChildren()).toEqual([span('A: 2, B: 3, C: 4')]); + }); + + it('unmount state', () => { + let updateA; + let updateB; + let updateC; + + function App(props) { + const [A, _updateA] = useState(0); + const [B, _updateB] = useState(0); + updateA = _updateA; + updateB = _updateB; + + let C; + if (props.loadC) { + const [_C, _updateC] = useState(0); + C = _C; + updateC = _updateC; + } else { + C = '[not loaded]'; + } + + return ; + } + + ReactNoop.render(); + expect(ReactNoop.flush()).toEqual(['A: 0, B: 0, C: 0']); + expect(ReactNoop.getChildren()).toEqual([span('A: 0, B: 0, C: 0')]); + + updateA(2); + updateB(3); + updateC(4); + expect(ReactNoop.flush()).toEqual(['A: 2, B: 3, C: 4']); + expect(ReactNoop.getChildren()).toEqual([span('A: 2, B: 3, C: 4')]); + + ReactNoop.render(); + expect(ReactNoop.flush()).toEqual(['A: 2, B: 3, C: [not loaded]']); + expect(ReactNoop.getChildren()).toEqual([ + span('A: 2, B: 3, C: [not loaded]'), + ]); + + updateC(4); + // TODO: This hook triggered a re-render even though it's unmounted. + // Should we warn? + expect(ReactNoop.flush()).toEqual(['A: 2, B: 3, C: [not loaded]']); + expect(ReactNoop.getChildren()).toEqual([ + span('A: 2, B: 3, C: [not loaded]'), + ]); + + updateB(4); + expect(ReactNoop.flush()).toEqual(['A: 2, B: 4, C: [not loaded]']); + expect(ReactNoop.getChildren()).toEqual([ + span('A: 2, B: 4, C: [not loaded]'), + ]); + }); + + it('unmount effects', () => { + function App(props) { + useEffect(() => { + ReactNoop.yield('Mount A'); + return () => { + ReactNoop.yield('Unmount A'); + }; + }, []); + + if (props.showMore) { + useEffect(() => { + ReactNoop.yield('Mount B'); + return () => { + ReactNoop.yield('Unmount B'); + }; + }, []); + } + + return null; + } + + ReactNoop.render(); + expect(ReactNoop.flush()).toEqual(['Mount A']); + + ReactNoop.render(); + expect(ReactNoop.flush()).toEqual(['Mount B']); + + ReactNoop.render(); + expect(ReactNoop.flush()).toEqual(['Unmount B']); + + ReactNoop.render(null); + expect(ReactNoop.flush()).toEqual(['Unmount A']); + }); + }); +}); diff --git a/packages/react-reconciler/src/__tests__/ReactNewContext-test.internal.js b/packages/react-reconciler/src/__tests__/ReactNewContext-test.internal.js index 312affa1e818d..4197b821b3020 100644 --- a/packages/react-reconciler/src/__tests__/ReactNewContext-test.internal.js +++ b/packages/react-reconciler/src/__tests__/ReactNewContext-test.internal.js @@ -12,6 +12,7 @@ let ReactFeatureFlags = require('shared/ReactFeatureFlags'); let React = require('react'); +let useContext; let ReactNoop; let gen; @@ -21,6 +22,7 @@ describe('ReactNewContext', () => { ReactFeatureFlags = require('shared/ReactFeatureFlags'); ReactFeatureFlags.debugRenderPhaseSideEffectsForStrictMode = false; React = require('react'); + useContext = React.useContext; ReactNoop = require('react-noop-renderer'); gen = require('random-seed'); }); @@ -34,33 +36,26 @@ describe('ReactNewContext', () => { return {type: 'span', children: [], prop, hidden: false}; } - function readContext(Context, observedBits) { - const dispatcher = - React.__SECRET_INTERNALS_DO_NOT_USE_OR_YOU_WILL_BE_FIRED.ReactCurrentOwner - .currentDispatcher; - return dispatcher.readContext(Context, observedBits); - } - // We have several ways of reading from context. sharedContextTests runs // a suite of tests for a given context consumer implementation. sharedContextTests('Context.Consumer', Context => Context.Consumer); sharedContextTests( - 'readContext(Context) inside function component', + 'useContext inside functional component', Context => function Consumer(props) { const observedBits = props.unstable_observedBits; - const contextValue = readContext(Context, observedBits); + const contextValue = useContext(Context, observedBits); const render = props.children; return render(contextValue); }, ); sharedContextTests( - 'readContext(Context) inside class component', + 'useContext inside class component', Context => class Consumer extends React.Component { render() { const observedBits = this.props.unstable_observedBits; - const contextValue = readContext(Context, observedBits); + const contextValue = useContext(Context, observedBits); const render = this.props.children; return render(contextValue); } @@ -1194,7 +1189,7 @@ describe('ReactNewContext', () => { return ( {foo => { - const bar = readContext(BarContext); + const bar = useContext(BarContext); return ; }} @@ -1238,7 +1233,7 @@ describe('ReactNewContext', () => { }); }); - describe('readContext', () => { + describe('useContext', () => { it('can use the same context multiple times in the same function', () => { const Context = React.createContext({foo: 0, bar: 0, baz: 0}, (a, b) => { let result = 0; @@ -1264,13 +1259,13 @@ describe('ReactNewContext', () => { } function FooAndBar() { - const {foo} = readContext(Context, 0b001); - const {bar} = readContext(Context, 0b010); + const {foo} = useContext(Context, 0b001); + const {bar} = useContext(Context, 0b010); return ; } function Baz() { - const {baz} = readContext(Context, 0b100); + const {baz} = useContext(Context, 0b100); return ; } diff --git a/packages/react/src/React.js b/packages/react/src/React.js index b04aef44cc693..f843ad7c72719 100644 --- a/packages/react/src/React.js +++ b/packages/react/src/React.js @@ -27,6 +27,16 @@ import {createContext} from './ReactContext'; import {lazy} from './ReactLazy'; import forwardRef from './forwardRef'; import memo from './memo'; +import { + useContext, + useState, + useReducer, + useRef, + useEffect, + useCallback, + useMemo, + useAPI, +} from './ReactHooks'; import { createElementWithValidation, createFactoryWithValidation, @@ -53,6 +63,15 @@ const React = { lazy, memo, + useContext, + useState, + useReducer, + useRef, + useEffect, + useCallback, + useMemo, + useAPI, + Fragment: REACT_FRAGMENT_TYPE, StrictMode: REACT_STRICT_MODE_TYPE, Suspense: REACT_SUSPENSE_TYPE, diff --git a/packages/react/src/ReactHooks.js b/packages/react/src/ReactHooks.js new file mode 100644 index 0000000000000..89b5b2977c7b1 --- /dev/null +++ b/packages/react/src/ReactHooks.js @@ -0,0 +1,83 @@ +/** + * Copyright (c) 2013-present, Facebook, Inc. + * + * This source code is licensed under the MIT license found in the + * LICENSE file in the root directory of this source tree. + * + * @flow + */ + +import type {ReactContext} from 'shared/ReactTypes'; + +import invariant from 'shared/invariant'; + +import ReactCurrentOwner from './ReactCurrentOwner'; + +function resolveDispatcher() { + const dispatcher = ReactCurrentOwner.currentDispatcher; + invariant( + dispatcher !== null, + 'Hooks can only be called inside the body of a functional component.', + ); + return dispatcher; +} + +export function useContext( + Context: ReactContext, + observedBits: number | boolean | void, +) { + const dispatcher = resolveDispatcher(); + return dispatcher.readContext(Context, observedBits); +} + +export function useState(initialState: S | (() => S)) { + const dispatcher = resolveDispatcher(); + return dispatcher.useState(initialState); +} + +export function useReducer( + reducer: (S, A) => S, + initialState: S, + initialAction: A | void | null, +) { + const dispatcher = resolveDispatcher(); + return dispatcher.useReducer(reducer, initialState, initialAction); +} + +export function useRef(initialValue: T): {current: T} { + const dispatcher = resolveDispatcher(); + return dispatcher.useRef(initialValue); +} + +export function useEffect( + create: () => mixed, + inputs: Array | void | null, +) { + const dispatcher = resolveDispatcher(); + return dispatcher.useEffect(create, inputs); +} + +export function useCallback( + callback: () => mixed, + inputs: Array | void | null, +) { + const dispatcher = resolveDispatcher(); + return dispatcher.useCallback(callback, inputs); +} + +export function useMemo( + create: () => mixed, + inputs: Array | void | null, +) { + const dispatcher = resolveDispatcher(); + return dispatcher.useMemo(create, inputs); +} + +export function useAPI( + ref: {current: T | null} | ((inst: T | null) => mixed) | null | void, + create: () => T, + inputs: Array | void | null, +): void { + const dispatcher = resolveDispatcher(); + return dispatcher.useAPI(ref, create, inputs); +} From 105f2de545dc9e374ed4ac55b3628eeeb555f4a6 Mon Sep 17 00:00:00 2001 From: Andrew Clark Date: Wed, 5 Sep 2018 17:20:13 -0700 Subject: [PATCH 02/19] Put hooks behind feature flag --- .../src/__tests__/ReactHooks-test.internal.js | 1 + .../ReactNewContext-test.internal.js | 1 + packages/react/src/React.js | 21 +++++++++++-------- packages/shared/ReactFeatureFlags.js | 1 + .../ReactFeatureFlags.native-fabric-fb.js | 1 + .../ReactFeatureFlags.native-fabric-oss.js | 1 + .../forks/ReactFeatureFlags.native-fb.js | 1 + .../forks/ReactFeatureFlags.native-oss.js | 1 + .../forks/ReactFeatureFlags.persistent.js | 1 + .../forks/ReactFeatureFlags.test-renderer.js | 1 + .../ReactFeatureFlags.test-renderer.www.js | 1 + .../shared/forks/ReactFeatureFlags.www.js | 3 +++ 12 files changed, 25 insertions(+), 9 deletions(-) diff --git a/packages/react-reconciler/src/__tests__/ReactHooks-test.internal.js b/packages/react-reconciler/src/__tests__/ReactHooks-test.internal.js index 5bcf446214020..4d43bb0241e48 100644 --- a/packages/react-reconciler/src/__tests__/ReactHooks-test.internal.js +++ b/packages/react-reconciler/src/__tests__/ReactHooks-test.internal.js @@ -29,6 +29,7 @@ describe('ReactHooks', () => { jest.resetModules(); ReactFeatureFlags = require('shared/ReactFeatureFlags'); ReactFeatureFlags.debugRenderPhaseSideEffectsForStrictMode = false; + ReactFeatureFlags.enableHooks = true; React = require('react'); ReactNoop = require('react-noop-renderer'); useState = React.useState; diff --git a/packages/react-reconciler/src/__tests__/ReactNewContext-test.internal.js b/packages/react-reconciler/src/__tests__/ReactNewContext-test.internal.js index 4197b821b3020..5aace46d135b1 100644 --- a/packages/react-reconciler/src/__tests__/ReactNewContext-test.internal.js +++ b/packages/react-reconciler/src/__tests__/ReactNewContext-test.internal.js @@ -21,6 +21,7 @@ describe('ReactNewContext', () => { jest.resetModules(); ReactFeatureFlags = require('shared/ReactFeatureFlags'); ReactFeatureFlags.debugRenderPhaseSideEffectsForStrictMode = false; + ReactFeatureFlags.enableHooks = true; React = require('react'); useContext = React.useContext; ReactNoop = require('react-noop-renderer'); diff --git a/packages/react/src/React.js b/packages/react/src/React.js index f843ad7c72719..1213fa50b4e4a 100644 --- a/packages/react/src/React.js +++ b/packages/react/src/React.js @@ -13,6 +13,7 @@ import { REACT_STRICT_MODE_TYPE, REACT_SUSPENSE_TYPE, } from 'shared/ReactSymbols'; +import {enableHooks} from 'shared/ReactFeatureFlags'; import {Component, PureComponent} from './ReactBaseClasses'; import {createRef} from './ReactCreateRef'; @@ -63,15 +64,6 @@ const React = { lazy, memo, - useContext, - useState, - useReducer, - useRef, - useEffect, - useCallback, - useMemo, - useAPI, - Fragment: REACT_FRAGMENT_TYPE, StrictMode: REACT_STRICT_MODE_TYPE, Suspense: REACT_SUSPENSE_TYPE, @@ -94,4 +86,15 @@ if (enableStableConcurrentModeAPIs) { React.unstable_Profiler = REACT_PROFILER_TYPE; } +if (enableHooks) { + React.useContext = useContext; + React.useState = useState; + React.useReducer = useReducer; + React.useRef = useRef; + React.useEffect = useEffect; + React.useCallback = useCallback; + React.useMemo = useMemo; + React.useAPI = useAPI; +} + export default React; diff --git a/packages/shared/ReactFeatureFlags.js b/packages/shared/ReactFeatureFlags.js index ada19d784e042..028b5a8e32d96 100644 --- a/packages/shared/ReactFeatureFlags.js +++ b/packages/shared/ReactFeatureFlags.js @@ -9,6 +9,7 @@ export const enableUserTimingAPI = __DEV__; +export const enableHooks = false; // Helps identify side effects in begin-phase lifecycle hooks and setState reducers: export const debugRenderPhaseSideEffects = false; diff --git a/packages/shared/forks/ReactFeatureFlags.native-fabric-fb.js b/packages/shared/forks/ReactFeatureFlags.native-fabric-fb.js index af1f294596c22..971f6bcbd3f9b 100644 --- a/packages/shared/forks/ReactFeatureFlags.native-fabric-fb.js +++ b/packages/shared/forks/ReactFeatureFlags.native-fabric-fb.js @@ -15,6 +15,7 @@ import typeof * as FabricFeatureFlagsType from './ReactFeatureFlags.native-fabri export const debugRenderPhaseSideEffects = false; export const debugRenderPhaseSideEffectsForStrictMode = false; export const enableUserTimingAPI = __DEV__; +export const enableHooks = false; export const warnAboutDeprecatedLifecycles = false; export const replayFailedUnitOfWorkWithInvokeGuardedCallback = __DEV__; export const enableProfilerTimer = __PROFILE__; diff --git a/packages/shared/forks/ReactFeatureFlags.native-fabric-oss.js b/packages/shared/forks/ReactFeatureFlags.native-fabric-oss.js index 20e5ba6f6f684..3f647674e34d8 100644 --- a/packages/shared/forks/ReactFeatureFlags.native-fabric-oss.js +++ b/packages/shared/forks/ReactFeatureFlags.native-fabric-oss.js @@ -15,6 +15,7 @@ import typeof * as FabricFeatureFlagsType from './ReactFeatureFlags.native-fabri export const debugRenderPhaseSideEffects = false; export const debugRenderPhaseSideEffectsForStrictMode = false; export const enableUserTimingAPI = __DEV__; +export const enableHooks = false; export const warnAboutDeprecatedLifecycles = false; export const replayFailedUnitOfWorkWithInvokeGuardedCallback = __DEV__; export const enableProfilerTimer = __PROFILE__; diff --git a/packages/shared/forks/ReactFeatureFlags.native-fb.js b/packages/shared/forks/ReactFeatureFlags.native-fb.js index 737035b32c5d3..6e9d290359c60 100644 --- a/packages/shared/forks/ReactFeatureFlags.native-fb.js +++ b/packages/shared/forks/ReactFeatureFlags.native-fb.js @@ -14,6 +14,7 @@ import typeof * as FeatureFlagsShimType from './ReactFeatureFlags.native-fb'; // Re-export dynamic flags from the fbsource version. export const { + enableHooks, debugRenderPhaseSideEffects, debugRenderPhaseSideEffectsForStrictMode, warnAboutDeprecatedLifecycles, diff --git a/packages/shared/forks/ReactFeatureFlags.native-oss.js b/packages/shared/forks/ReactFeatureFlags.native-oss.js index bd11e8daf7702..96f7607431a1b 100644 --- a/packages/shared/forks/ReactFeatureFlags.native-oss.js +++ b/packages/shared/forks/ReactFeatureFlags.native-oss.js @@ -14,6 +14,7 @@ import typeof * as FeatureFlagsShimType from './ReactFeatureFlags.native-oss'; export const debugRenderPhaseSideEffects = false; export const debugRenderPhaseSideEffectsForStrictMode = false; +export const enableHooks = false; export const enableUserTimingAPI = __DEV__; export const replayFailedUnitOfWorkWithInvokeGuardedCallback = __DEV__; export const warnAboutDeprecatedLifecycles = false; diff --git a/packages/shared/forks/ReactFeatureFlags.persistent.js b/packages/shared/forks/ReactFeatureFlags.persistent.js index 6880b7fa72bec..bcb561d62b21e 100644 --- a/packages/shared/forks/ReactFeatureFlags.persistent.js +++ b/packages/shared/forks/ReactFeatureFlags.persistent.js @@ -15,6 +15,7 @@ import typeof * as PersistentFeatureFlagsType from './ReactFeatureFlags.persiste export const debugRenderPhaseSideEffects = false; export const debugRenderPhaseSideEffectsForStrictMode = false; export const enableUserTimingAPI = __DEV__; +export const enableHooks = false; export const warnAboutDeprecatedLifecycles = false; export const replayFailedUnitOfWorkWithInvokeGuardedCallback = __DEV__; export const enableProfilerTimer = __PROFILE__; diff --git a/packages/shared/forks/ReactFeatureFlags.test-renderer.js b/packages/shared/forks/ReactFeatureFlags.test-renderer.js index 94ab168cd025b..9431ff5623d7e 100644 --- a/packages/shared/forks/ReactFeatureFlags.test-renderer.js +++ b/packages/shared/forks/ReactFeatureFlags.test-renderer.js @@ -15,6 +15,7 @@ import typeof * as PersistentFeatureFlagsType from './ReactFeatureFlags.persiste export const debugRenderPhaseSideEffects = false; export const debugRenderPhaseSideEffectsForStrictMode = false; export const enableUserTimingAPI = __DEV__; +export const enableHooks = false; export const warnAboutDeprecatedLifecycles = false; export const replayFailedUnitOfWorkWithInvokeGuardedCallback = false; export const enableProfilerTimer = false; diff --git a/packages/shared/forks/ReactFeatureFlags.test-renderer.www.js b/packages/shared/forks/ReactFeatureFlags.test-renderer.www.js index c3f67290eb750..fb1c9bd817b9e 100644 --- a/packages/shared/forks/ReactFeatureFlags.test-renderer.www.js +++ b/packages/shared/forks/ReactFeatureFlags.test-renderer.www.js @@ -15,6 +15,7 @@ import typeof * as PersistentFeatureFlagsType from './ReactFeatureFlags.persiste export const debugRenderPhaseSideEffects = false; export const debugRenderPhaseSideEffectsForStrictMode = false; export const enableUserTimingAPI = __DEV__; +export const enableHooks = false; export const warnAboutDeprecatedLifecycles = false; export const replayFailedUnitOfWorkWithInvokeGuardedCallback = false; export const enableProfilerTimer = false; diff --git a/packages/shared/forks/ReactFeatureFlags.www.js b/packages/shared/forks/ReactFeatureFlags.www.js index 755d418ed796a..bf0057ef3620c 100644 --- a/packages/shared/forks/ReactFeatureFlags.www.js +++ b/packages/shared/forks/ReactFeatureFlags.www.js @@ -20,6 +20,9 @@ export const { disableInputAttributeSyncing, } = require('ReactFeatureFlags'); +// The rest of the flags are static for better dead code elimination. +export const enableHooks = false; + // In www, we have experimental support for gathering data // from User Timing API calls in production. By default, we // only emit performance.mark/measure calls in __DEV__. But if From 11d0781eea7dfa65e0ea9e54d18ff937122f9524 Mon Sep 17 00:00:00 2001 From: Andrew Clark Date: Sun, 23 Sep 2018 18:12:46 -0700 Subject: [PATCH 03/19] Defer useEffect until after paint Effects scheduled by useEffect should not fire until after the browser has had a chance to paint. However, they should be fired before any subsequent mutations. Also adds useMutationEffect and useLayoutEffect. useMutationEffect fires during the host update phase. useLayoutEffect fires during the post- update phase (the same phase as componentDidMount and componentDidUpdate). --- .../src/ReactFiberCommitWork.js | 179 ++--- .../src/ReactFiberDispatcher.js | 22 +- .../react-reconciler/src/ReactFiberHooks.js | 131 +++- .../react-reconciler/src/ReactFiberRoot.js | 7 + .../src/ReactFiberScheduler.js | 116 +++- .../src/ReactHookEffectTags.js | 19 + .../src/__tests__/ReactHooks-test.internal.js | 643 ++++++++++++++++-- .../ReactNewContext-test.internal.js | 26 +- packages/react/src/React.js | 24 +- packages/react/src/ReactHooks.js | 19 +- packages/shared/ReactSideEffectTags.js | 33 +- 11 files changed, 966 insertions(+), 253 deletions(-) create mode 100644 packages/react-reconciler/src/ReactHookEffectTags.js diff --git a/packages/react-reconciler/src/ReactFiberCommitWork.js b/packages/react-reconciler/src/ReactFiberCommitWork.js index c8f430ec6c4b6..bad7139e5bbe8 100644 --- a/packages/react-reconciler/src/ReactFiberCommitWork.js +++ b/packages/react-reconciler/src/ReactFiberCommitWork.js @@ -86,6 +86,16 @@ import { requestCurrentTime, scheduleWork, } from './ReactFiberScheduler'; +import { + NoEffect as NoHookEffect, + UnmountSnapshot, + UnmountMutation, + MountMutation, + UnmountLayout, + MountLayout, + UnmountPassive, + MountPassive, +} from './ReactHookEffectTags'; let didWarnAboutUndefinedSnapshotBeforeUpdate: Set | null = null; if (__DEV__) { @@ -204,6 +214,11 @@ function commitBeforeMutationLifeCycles( finishedWork: Fiber, ): void { switch (finishedWork.tag) { + case FunctionComponent: + case ForwardRef: { + commitHookEffectList(UnmountSnapshot, NoHookEffect, finishedWork); + return; + } case ClassComponent: { if (finishedWork.effectTag & Snapshot) { if (current !== null) { @@ -254,26 +269,39 @@ function commitBeforeMutationLifeCycles( } } -function destroyRemainingEffects(firstToDestroy, stopAt) { - let effect = firstToDestroy; - do { - const destroy = effect.value; - if (destroy !== null) { - destroy(); - } - effect = effect.next; - } while (effect !== stopAt); +function commitHookEffectList( + unmountTag: number, + mountTag: number, + finishedWork: Fiber, +) { + const updateQueue: FunctionComponentUpdateQueue | null = (finishedWork.updateQueue: any); + let lastEffect = updateQueue !== null ? updateQueue.lastEffect : null; + if (lastEffect !== null) { + const firstEffect = lastEffect.next; + let effect = firstEffect; + do { + if ((effect.tag & unmountTag) !== NoHookEffect) { + // Unmount + const destroy = effect.destroy; + effect.destroy = null; + if (destroy !== null) { + destroy(); + } + } + if ((effect.tag & mountTag) !== NoHookEffect) { + // Mount + const create = effect.create; + const destroy = create(); + effect.destroy = typeof destroy === 'function' ? destroy : null; + } + effect = effect.next; + } while (effect !== firstEffect); + } } -function destroyMountedEffects(current) { - const oldUpdateQueue: FunctionComponentUpdateQueue | null = (current.updateQueue: any); - if (oldUpdateQueue !== null) { - const oldLastEffect = oldUpdateQueue.lastEffect; - if (oldLastEffect !== null) { - const oldFirstEffect = oldLastEffect.next; - destroyRemainingEffects(oldFirstEffect, oldFirstEffect); - } - } +export function commitPassiveHookEffects(finishedWork: Fiber): void { + commitHookEffectList(UnmountPassive, NoHookEffect, finishedWork); + commitHookEffectList(NoHookEffect, MountPassive, finishedWork); } function commitLifeCycles( @@ -285,98 +313,12 @@ function commitLifeCycles( switch (finishedWork.tag) { case FunctionComponent: case ForwardRef: { - const updateQueue: FunctionComponentUpdateQueue | null = (finishedWork.updateQueue: any); - if (updateQueue !== null) { - // Mount new effects and destroy the old ones by comparing to the - // current list of effects. This could be a bit simpler if we avoided - // the need to compare to the previous effect list by transferring the - // old `destroy` method to the new effect during the render phase. - // That's how I originally implemented it, but it requires an additional - // field on the effect object. - // - // This supports removing effects from the end of the list. If we adopt - // the constraint that hooks are append only, that would also save a bit - // on code size. - const newLastEffect = updateQueue.lastEffect; - if (newLastEffect !== null) { - const newFirstEffect = newLastEffect.next; - let oldLastEffect = null; - if (current !== null) { - const oldUpdateQueue: FunctionComponentUpdateQueue | null = (current.updateQueue: any); - if (oldUpdateQueue !== null) { - oldLastEffect = oldUpdateQueue.lastEffect; - } - } - if (oldLastEffect !== null) { - const oldFirstEffect = oldLastEffect.next; - let newEffect = newFirstEffect; - let oldEffect = oldFirstEffect; - - // Before mounting the new effects, unmount all the old ones. - do { - if (oldEffect !== null) { - if (newEffect.inputs !== oldEffect.inputs) { - const destroy = oldEffect.value; - if (destroy !== null) { - destroy(); - } - } - oldEffect = oldEffect.next; - if (oldEffect === oldFirstEffect) { - oldEffect = null; - } - } - newEffect = newEffect.next; - } while (newEffect !== newFirstEffect); - - // Unmount any remaining effects in the old list that do not - // appear in the new one. - if (oldEffect !== null) { - destroyRemainingEffects(oldEffect, oldFirstEffect); - } - - // Now loop through the list again to mount the new effects - oldEffect = oldFirstEffect; - do { - const create = newEffect.value; - if (oldEffect !== null) { - if (newEffect.inputs !== oldEffect.inputs) { - const newDestroy = create(); - newEffect.value = - typeof newDestroy === 'function' ? newDestroy : null; - } else { - newEffect.value = oldEffect.value; - } - oldEffect = oldEffect.next; - if (oldEffect === oldFirstEffect) { - oldEffect = null; - } - } else { - const newDestroy = create(); - newEffect.value = - typeof newDestroy === 'function' ? newDestroy : null; - } - newEffect = newEffect.next; - } while (newEffect !== newFirstEffect); - } else { - let newEffect = newFirstEffect; - do { - const create = newEffect.value; - const newDestroy = create(); - newEffect.value = - typeof newDestroy === 'function' ? newDestroy : null; - newEffect = newEffect.next; - } while (newEffect !== newFirstEffect); - } - } else if (current !== null) { - // There are no effects, which means all current effects must - // be destroyed - destroyMountedEffects(current); - } - - const callbackList = updateQueue.callbackList; + commitHookEffectList(UnmountLayout, MountLayout, finishedWork); + const newUpdateQueue: FunctionComponentUpdateQueue | null = (finishedWork.updateQueue: any); + if (newUpdateQueue !== null) { + const callbackList = newUpdateQueue.callbackList; if (callbackList !== null) { - updateQueue.callbackList = null; + newUpdateQueue.callbackList = null; for (let i = 0; i < callbackList.length; i++) { const update = callbackList[i]; // Assume this is non-null, since otherwise it would not be part @@ -386,10 +328,6 @@ function commitLifeCycles( callback(); } } - } else if (current !== null) { - // There are no effects, which means all current effects must - // be destroyed - destroyMountedEffects(current); } break; } @@ -656,7 +594,7 @@ function commitUnmount(current: Fiber): void { const firstEffect = lastEffect.next; let effect = firstEffect; do { - const destroy = effect.value; + const destroy = effect.destroy; if (destroy !== null) { safelyCallDestroy(current, destroy); } @@ -1039,11 +977,24 @@ function commitDeletion(current: Fiber): void { function commitWork(current: Fiber | null, finishedWork: Fiber): void { if (!supportsMutation) { + switch (finishedWork.tag) { + case FunctionComponent: + case ForwardRef: { + commitHookEffectList(UnmountMutation, MountMutation, finishedWork); + return; + } + } + commitContainer(finishedWork); return; } switch (finishedWork.tag) { + case FunctionComponent: + case ForwardRef: { + commitHookEffectList(UnmountMutation, MountMutation, finishedWork); + return; + } case ClassComponent: { return; } diff --git a/packages/react-reconciler/src/ReactFiberDispatcher.js b/packages/react-reconciler/src/ReactFiberDispatcher.js index 9c5e790fe440c..10c0dee649a64 100644 --- a/packages/react-reconciler/src/ReactFiberDispatcher.js +++ b/packages/react-reconciler/src/ReactFiberDispatcher.js @@ -9,22 +9,28 @@ import {readContext} from './ReactFiberNewContext'; import { - useState, - useReducer, - useEffect, + useAPI, useCallback, + useContext, + useEffect, + useLayoutEffect, useMemo, + useMutationEffect, + useReducer, useRef, - useAPI, + useState, } from './ReactFiberHooks'; export const Dispatcher = { readContext, - useState, - useReducer, - useEffect, + useAPI, useCallback, + useContext, + useEffect, + useLayoutEffect, useMemo, + useMutationEffect, + useReducer, useRef, - useAPI, + useState, }; diff --git a/packages/react-reconciler/src/ReactFiberHooks.js b/packages/react-reconciler/src/ReactFiberHooks.js index 66720a3707ef0..8d61bcf1270b6 100644 --- a/packages/react-reconciler/src/ReactFiberHooks.js +++ b/packages/react-reconciler/src/ReactFiberHooks.js @@ -7,11 +7,28 @@ * @flow */ +import type {ReactContext} from 'shared/ReactTypes'; import type {Fiber} from './ReactFiber'; import type {ExpirationTime} from './ReactFiberExpirationTime'; +import type {HookEffectTag} from './ReactHookEffectTags'; import {NoWork} from './ReactFiberExpirationTime'; -import {Callback as CallbackEffect} from 'shared/ReactSideEffectTags'; +import {readContext} from './ReactFiberNewContext'; +import { + Snapshot as SnapshotEffect, + Update as UpdateEffect, + Callback as CallbackEffect, + Passive as PassiveEffect, +} from 'shared/ReactSideEffectTags'; +import { + NoEffect as NoHookEffect, + UnmountSnapshot, + UnmountMutation, + MountMutation, + MountLayout, + UnmountPassive, + MountPassive, +} from './ReactHookEffectTags'; import { scheduleWork, computeExpirationForFiber, @@ -43,10 +60,9 @@ type Hook = { }; type Effect = { - // For an unmounted effect, this points to the effect constructor. Once it's - // mounted, it points to a destroy function (or null). I've opted to reuse - // the same field to save memory. - value: any, + tag: HookEffectTag, + create: () => mixed, + destroy: (() => mixed) | null, inputs: Array, next: Effect, }; @@ -166,6 +182,9 @@ export function finishHooks( renderedWork.updateQueue = (componentUpdateQueue: any); } + const didRenderTooFewHooks = + currentHook !== null && currentHook.next !== null; + renderExpirationTime = NoWork; currentlyRenderingFiber = null; @@ -185,6 +204,12 @@ export function finishHooks( // renderPhaseUpdates = null; // numberOfReRenders = 0; + invariant( + !didRenderTooFewHooks, + 'Rendered fewer hooks than expected. This may be caused by an accidental ' + + 'early return statement.', + ); + return children; } @@ -295,6 +320,16 @@ function basicStateReducer(state: S, action: BasicStateAction): S { return typeof action === 'function' ? action(state) : action; } +export function useContext( + context: ReactContext, + observedBits: void | number | boolean, +): T { + // Ensure we're in a functional component (class components support only the + // .unstable_read() form) + resolveCurrentlyRenderingFiber(); + return readContext(context, observedBits); +} + export function useState( initialState: S | (() => S), ): [S, Dispatch>] { @@ -460,9 +495,11 @@ function pushCallback(workInProgress: Fiber, update: Update): void { workInProgress.effectTag |= CallbackEffect; } -function pushEffect(value, inputs) { +function pushEffect(tag, create, destroy, inputs) { const effect: Effect = { - value, + tag, + create, + destroy, inputs, // Circular next: (null: any), @@ -500,10 +537,38 @@ export function useRef(initialValue: T): {current: T} { return ref; } +export function useMutationEffect( + create: () => mixed, + inputs: Array | void | null, +): void { + useEffectImpl( + SnapshotEffect | UpdateEffect, + UnmountSnapshot | MountMutation, + create, + inputs, + ); +} + +export function useLayoutEffect( + create: () => mixed, + inputs: Array | void | null, +): void { + useEffectImpl(UpdateEffect, UnmountMutation | MountLayout, create, inputs); +} + export function useEffect( create: () => mixed, inputs: Array | void | null, ): void { + useEffectImpl( + UpdateEffect | PassiveEffect, + UnmountPassive | MountPassive, + create, + inputs, + ); +} + +function useEffectImpl(fiberEffectTag, hookEffectTag, create, inputs): void { currentlyRenderingFiber = resolveCurrentlyRenderingFiber(); workInProgressHook = createWorkInProgressHook(); @@ -512,19 +577,18 @@ export function useEffect( if (currentHook !== null) { const prevEffect = currentHook.memoizedState; const prevInputs = prevEffect.inputs; - if (inputsAreEqual(nextInputs, prevInputs)) { - nextEffect = pushEffect(prevEffect.value, prevInputs); - } else { - nextEffect = pushEffect(create, nextInputs); - } + nextEffect = pushEffect( + inputsAreEqual(nextInputs, prevInputs) ? NoHookEffect : hookEffectTag, + create, + prevEffect.destroy, + nextInputs, + ); } else { - nextEffect = pushEffect(create, nextInputs); + nextEffect = pushEffect(hookEffectTag, create, null, nextInputs); } - // TODO: If we decide not to support removing hooks from the end of the list, - // we only need to schedule an effect if the inputs changed. - currentlyRenderingFiber.effectTag |= CallbackEffect; workInProgressHook.memoizedState = nextEffect; + currentlyRenderingFiber.effectTag |= fiberEffectTag; } export function useAPI( @@ -541,21 +605,26 @@ export function useAPI( // TODO: I've implemented this on top of useEffect because it's almost the // same thing, and it would require an equal amount of code. It doesn't seem // like a common enough use case to justify the additional size. - useEffect(() => { - if (typeof ref === 'function') { - const refCallback = ref; - const inst = create(); - refCallback(inst); - return () => refCallback(null); - } else if (ref !== null && ref !== undefined) { - const refObject = ref; - const inst = create(); - refObject.current = inst; - return () => { - refObject.current = null; - }; - } - }, nextInputs); + useEffectImpl( + UpdateEffect, + UnmountMutation | MountLayout, + () => { + if (typeof ref === 'function') { + const refCallback = ref; + const inst = create(); + refCallback(inst); + return () => refCallback(null); + } else if (ref !== null && ref !== undefined) { + const refObject = ref; + const inst = create(); + refObject.current = inst; + return () => { + refObject.current = null; + }; + } + }, + nextInputs, + ); } export function useCallback( diff --git a/packages/react-reconciler/src/ReactFiberRoot.js b/packages/react-reconciler/src/ReactFiberRoot.js index d257f3c93ab6f..307f7c82330b8 100644 --- a/packages/react-reconciler/src/ReactFiberRoot.js +++ b/packages/react-reconciler/src/ReactFiberRoot.js @@ -36,6 +36,9 @@ type BaseFiberRootProperties = {| // The currently active root fiber. This is the mutable root of the tree. current: Fiber, + serialEffectCallback: (() => mixed) | null, + serialEffectCallbackHandle: any, + // The following priority levels are used to distinguish between 1) // uncommitted work, 2) uncommitted work that is suspended, and 3) uncommitted // work that may be unsuspended. We choose not to track each individual @@ -114,6 +117,8 @@ export function createFiberRoot( current: uninitializedFiber, containerInfo: containerInfo, pendingChildren: null, + serialEffectCallback: null, + serialEffectCallbackHandle: null, earliestPendingTime: NoWork, latestPendingTime: NoWork, @@ -143,6 +148,8 @@ export function createFiberRoot( current: uninitializedFiber, containerInfo: containerInfo, pendingChildren: null, + serialEffectCallback: null, + serialEffectCallbackHandle: null, earliestPendingTime: NoWork, latestPendingTime: NoWork, diff --git a/packages/react-reconciler/src/ReactFiberScheduler.js b/packages/react-reconciler/src/ReactFiberScheduler.js index 11f2c7a1b6c9c..f7437c37269c5 100644 --- a/packages/react-reconciler/src/ReactFiberScheduler.js +++ b/packages/react-reconciler/src/ReactFiberScheduler.js @@ -12,7 +12,15 @@ import type {Batch, FiberRoot} from './ReactFiberRoot'; import type {ExpirationTime} from './ReactFiberExpirationTime'; import type {Interaction} from 'scheduler/src/Tracing'; -import {__interactionsRef, __subscriberRef} from 'scheduler/tracing'; +import { + __interactionsRef, + __subscriberRef, + unstable_wrap as Schedule_tracing_wrap, +} from 'scheduler/tracing'; +import { + unstable_scheduleCallback as Schedule_scheduleCallback, + unstable_cancelCallback as Schedule_cancelCallback, +} from 'scheduler'; import { invokeGuardedCallback, hasCaughtError, @@ -34,6 +42,7 @@ import { Ref, Incomplete, HostEffectMask, + Passive, } from 'shared/ReactSideEffectTags'; import { HostRoot, @@ -151,6 +160,7 @@ import { commitLifeCycles, commitAttachRef, commitDetachRef, + commitPassiveHookEffects, } from './ReactFiberCommitWork'; import {Dispatcher} from './ReactFiberDispatcher'; @@ -254,6 +264,7 @@ let nextRenderDidError: boolean = false; let nextEffect: Fiber | null = null; let isCommitting: boolean = false; +let needsPassiveCommit: boolean = false; let legacyErrorBoundariesThatAlreadyFailed: Set | null = null; @@ -453,8 +464,6 @@ function commitBeforeMutationLifecycles() { commitBeforeMutationLifeCycles(current, nextEffect); } - // Don't cleanup effects yet; - // This will be done by commitAllLifeCycles() nextEffect = nextEffect.nextEffect; } @@ -494,15 +503,51 @@ function commitAllLifeCycles( commitAttachRef(nextEffect); } - const next = nextEffect.nextEffect; - // Ensure that we clean these up so that we don't accidentally keep them. - // I'm not actually sure this matters because we can't reset firstEffect - // and lastEffect since they're on every node, not just the effectful - // ones. So we have to clean everything as we reuse nodes anyway. - nextEffect.nextEffect = null; - // Ensure that we reset the effectTag here so that we can rely on effect - // tags to reason about the current life-cycle. - nextEffect = next; + if (effectTag & Passive) { + needsPassiveCommit = true; + } + + nextEffect = nextEffect.nextEffect; + } +} + +function commitPassiveEffects(root: FiberRoot, firstEffect: Fiber): void { + // Set this to true to prevent re-entrancy + const previousIsRendering = isRendering; + isRendering = true; + + let effect = firstEffect; + do { + if (effect.effectTag & Passive) { + let didError = false; + let error; + if (__DEV__) { + invokeGuardedCallback(null, commitPassiveHookEffects, null, effect); + if (hasCaughtError()) { + didError = true; + error = clearCaughtError(); + } + } else { + try { + commitPassiveHookEffects(effect); + } catch (e) { + didError = true; + error = e; + } + } + if (didError) { + captureCommitPhaseError(effect, error); + } + } + effect = effect.nextEffect; + } while (effect !== null); + + isRendering = previousIsRendering; + + // Check if work was scheduled by one of the effects + const rootExpirationTime = root.expirationTime; + if (rootExpirationTime !== NoWork) { + requestWork(root, rootExpirationTime); } } @@ -522,6 +567,20 @@ function markLegacyErrorBoundaryAsFailed(instance: mixed) { } function commitRoot(root: FiberRoot, finishedWork: Fiber): void { + const existingSerialEffectCallback = root.serialEffectCallback; + const existingSerialEffectCallbackHandle = root.serialEffectCallbackHandle; + if (existingSerialEffectCallback !== null) { + // A passive callback was scheduled during the previous commit, but it did + // not get a chance to flush. Flush it now to ensure serial execution. + // This should fire before any new mutations. + root.serialEffectCallback = null; + if (existingSerialEffectCallbackHandle !== null) { + root.serialEffectCallbackHandle = null; + Schedule_cancelCallback(existingSerialEffectCallbackHandle); + } + existingSerialEffectCallback(); + } + isWorking = true; isCommitting = true; startCommitTimer(); @@ -711,6 +770,34 @@ function commitRoot(root: FiberRoot, finishedWork: Fiber): void { } } + if (firstEffect !== null && needsPassiveCommit) { + const resolvedFirstEffect = firstEffect; + // This commit included a passive effect. These do not need to fire until + // after the next paint. Schedule an callback to fire them in an async + // event. To ensure serial execution, the callback will be flushed early if + // we enter another commit phase before then. + needsPassiveCommit = false; + let serialEffectCallback; + if (enableSchedulerTracing) { + // TODO: Avoid this extra callback by mutating the tracing ref directly, + // like we do at the beginning of commitRoot. I've opted not to do that + // here because that code is still in flux. + serialEffectCallback = Schedule_tracing_wrap(() => { + root.serialEffectCallback = null; + commitPassiveEffects(root, resolvedFirstEffect); + }); + } else { + serialEffectCallback = () => { + root.serialEffectCallback = null; + commitPassiveEffects(root, resolvedFirstEffect); + }; + } + root.serialEffectCallback = serialEffectCallback; + root.serialEffectCallbackHandle = Schedule_scheduleCallback( + serialEffectCallback, + ); + } + isCommitting = false; isWorking = false; stopCommitLifeCyclesTimer(); @@ -1423,11 +1510,6 @@ function dispatch( value: mixed, expirationTime: ExpirationTime, ) { - invariant( - !isWorking || isCommitting, - 'dispatch: Cannot dispatch during the render phase.', - ); - let fiber = sourceFiber.return; while (fiber !== null) { switch (fiber.tag) { diff --git a/packages/react-reconciler/src/ReactHookEffectTags.js b/packages/react-reconciler/src/ReactHookEffectTags.js new file mode 100644 index 0000000000000..d54df30cf4e73 --- /dev/null +++ b/packages/react-reconciler/src/ReactHookEffectTags.js @@ -0,0 +1,19 @@ +/** + * Copyright (c) Facebook, Inc. and its affiliates. + * + * This source code is licensed under the MIT license found in the + * LICENSE file in the root directory of this source tree. + * + * @flow + */ + +export type HookEffectTag = number; + +export const NoEffect = /* */ 0b00000000; +export const UnmountSnapshot = /* */ 0b00000010; +export const UnmountMutation = /* */ 0b00000100; +export const MountMutation = /* */ 0b00001000; +export const UnmountLayout = /* */ 0b00010000; +export const MountLayout = /* */ 0b00100000; +export const MountPassive = /* */ 0b01000000; +export const UnmountPassive = /* */ 0b10000000; diff --git a/packages/react-reconciler/src/__tests__/ReactHooks-test.internal.js b/packages/react-reconciler/src/__tests__/ReactHooks-test.internal.js index 4d43bb0241e48..f73b77abe2e7c 100644 --- a/packages/react-reconciler/src/__tests__/ReactHooks-test.internal.js +++ b/packages/react-reconciler/src/__tests__/ReactHooks-test.internal.js @@ -18,15 +18,41 @@ let ReactNoop; let useState; let useReducer; let useEffect; +let useMutationEffect; +let useLayoutEffect; let useCallback; let useMemo; let useRef; let useAPI; let forwardRef; +let flushPassiveEffects; describe('ReactHooks', () => { beforeEach(() => { jest.resetModules(); + + jest.mock('scheduler', () => { + let scheduledCallbacks = new Map(); + + flushPassiveEffects = () => { + scheduledCallbacks.forEach(cb => { + cb(); + }); + scheduledCallbacks = new Map(); + }; + + return { + unstable_scheduleCallback(callback) { + const handle = {}; + scheduledCallbacks.set(handle, callback); + return handle; + }, + unstable_cancelCallback(handle) { + scheduledCallbacks.delete(handle); + }, + }; + }); + ReactFeatureFlags = require('shared/ReactFeatureFlags'); ReactFeatureFlags.debugRenderPhaseSideEffectsForStrictMode = false; ReactFeatureFlags.enableHooks = true; @@ -35,6 +61,8 @@ describe('ReactHooks', () => { useState = React.useState; useReducer = React.useReducer; useEffect = React.useEffect; + useMutationEffect = React.useMutationEffect; + useLayoutEffect = React.useLayoutEffect; useCallback = React.useCallback; useMemo = React.useMemo; useRef = React.useRef; @@ -563,12 +591,170 @@ describe('ReactHooks', () => { return ; } ReactNoop.render(); - expect(ReactNoop.flush()).toEqual(['Count: 0', 'Did commit [0]']); + expect(ReactNoop.flush()).toEqual(['Count: 0']); expect(ReactNoop.getChildren()).toEqual([span('Count: 0')]); + flushPassiveEffects(); + expect(ReactNoop.clearYields()).toEqual(['Did commit [0]']); ReactNoop.render(); - expect(ReactNoop.flush()).toEqual(['Count: 1', 'Did commit [1]']); + expect(ReactNoop.flush()).toEqual(['Count: 1']); expect(ReactNoop.getChildren()).toEqual([span('Count: 1')]); + // Effects are deferred until after the commit + flushPassiveEffects(); + expect(ReactNoop.clearYields()).toEqual(['Did commit [1]']); + }); + + it( + 'flushes effects serially by flushing old effects before flushing ' + + "new ones, if they haven't already fired", + () => { + function getCommittedText() { + const children = ReactNoop.getChildren(); + if (children === null) { + return null; + } + return children[0].prop; + } + + function Counter(props) { + useEffect(() => { + ReactNoop.yield( + `Committed state when effect was fired: ${getCommittedText()}`, + ); + }); + return ; + } + ReactNoop.render(); + expect(ReactNoop.flush()).toEqual([0]); + expect(ReactNoop.getChildren()).toEqual([span(0)]); + + // Before the effects have a chance to flush, schedule another update + ReactNoop.render(); + expect(ReactNoop.flush()).toEqual([ + 1, + // The previous effect flushes before the host mutations + 'Committed state when effect was fired: 0', + ]); + expect(ReactNoop.getChildren()).toEqual([span(1)]); + + flushPassiveEffects(); + expect(ReactNoop.clearYields()).toEqual([ + 'Committed state when effect was fired: 1', + ]); + }, + ); + + it('updates have async priority', () => { + function Counter(props) { + const [count, updateCount] = useState('(empty)'); + useEffect( + () => { + ReactNoop.yield(`Schedule update [${props.count}]`); + updateCount(props.count); + }, + [props.count], + ); + return ; + } + ReactNoop.render(); + expect(ReactNoop.flush()).toEqual(['Count: (empty)']); + expect(ReactNoop.getChildren()).toEqual([span('Count: (empty)')]); + flushPassiveEffects(); + expect(ReactNoop.clearYields()).toEqual(['Schedule update [0]']); + expect(ReactNoop.flush()).toEqual(['Count: 0']); + + ReactNoop.render(); + expect(ReactNoop.flush()).toEqual(['Count: 0']); + expect(ReactNoop.getChildren()).toEqual([span('Count: 0')]); + flushPassiveEffects(); + expect(ReactNoop.clearYields()).toEqual(['Schedule update [1]']); + expect(ReactNoop.flush()).toEqual(['Count: 1']); + }); + + it('updates have async priority even if effects are flushed early', () => { + function Counter(props) { + const [count, updateCount] = useState('(empty)'); + useEffect( + () => { + ReactNoop.yield(`Schedule update [${props.count}]`); + updateCount(props.count); + }, + [props.count], + ); + return ; + } + ReactNoop.render(); + expect(ReactNoop.flush()).toEqual(['Count: (empty)']); + expect(ReactNoop.getChildren()).toEqual([span('Count: (empty)')]); + + // Rendering again should flush the previous commit's effects + ReactNoop.render(); + ReactNoop.flushThrough([ + 'Count: (empty)', + 'Schedule update [0]', + 'Count: 0', + ]); + expect(ReactNoop.getChildren()).toEqual([span('Count: (empty)')]); + + expect(ReactNoop.flush()).toEqual(['Schedule update [1]', 'Count: 1']); + expect(ReactNoop.getChildren()).toEqual([span('Count: 1')]); + }); + + it( + 'in sync mode, useEffect is deferred and updates finish synchronously ' + + '(in a single batch)', + () => { + function Counter(props) { + const [count, updateCount] = useState('(empty)'); + useEffect( + () => { + // Update multiple times. These should all be batched together in + // a single render. + updateCount(props.count); + updateCount(props.count); + updateCount(props.count); + updateCount(props.count); + updateCount(props.count); + updateCount(props.count); + }, + [props.count], + ); + return ; + } + ReactNoop.renderLegacySyncRoot(); + // Even in sync mode, effects are deferred until after paint + expect(ReactNoop.flush()).toEqual(['Count: (empty)']); + expect(ReactNoop.getChildren()).toEqual([span('Count: (empty)')]); + // Now fire the effects + flushPassiveEffects(); + // There were multiple updates, but there should only be a + // single render + expect(ReactNoop.clearYields()).toEqual(['Count: 0']); + expect(ReactNoop.getChildren()).toEqual([span('Count: 0')]); + }, + ); + + it('flushSync is not allowed', () => { + function Counter(props) { + const [count, updateCount] = useState('(empty)'); + useEffect( + () => { + ReactNoop.yield(`Schedule update [${props.count}]`); + ReactNoop.flushSync(() => { + updateCount(props.count); + }); + }, + [props.count], + ); + return ; + } + ReactNoop.render(); + expect(ReactNoop.flush()).toEqual(['Count: (empty)']); + expect(ReactNoop.getChildren()).toEqual([span('Count: (empty)')]); + + expect(() => { + flushPassiveEffects(); + }).toThrow('flushSync was called from inside a lifecycle method'); }); it('unmounts previous effect', () => { @@ -582,16 +768,19 @@ describe('ReactHooks', () => { return ; } ReactNoop.render(); - expect(ReactNoop.flush()).toEqual(['Count: 0', 'Did create [0]']); + expect(ReactNoop.flush()).toEqual(['Count: 0']); expect(ReactNoop.getChildren()).toEqual([span('Count: 0')]); + flushPassiveEffects(); + expect(ReactNoop.clearYields()).toEqual(['Did create [0]']); ReactNoop.render(); - expect(ReactNoop.flush()).toEqual([ - 'Count: 1', + expect(ReactNoop.flush()).toEqual(['Count: 1']); + expect(ReactNoop.getChildren()).toEqual([span('Count: 1')]); + flushPassiveEffects(); + expect(ReactNoop.clearYields()).toEqual([ 'Did destroy [0]', 'Did create [1]', ]); - expect(ReactNoop.getChildren()).toEqual([span('Count: 1')]); }); it('unmounts on deletion', () => { @@ -605,8 +794,10 @@ describe('ReactHooks', () => { return ; } ReactNoop.render(); - expect(ReactNoop.flush()).toEqual(['Count: 0', 'Did create [0]']); + expect(ReactNoop.flush()).toEqual(['Count: 0']); expect(ReactNoop.getChildren()).toEqual([span('Count: 0')]); + flushPassiveEffects(); + expect(ReactNoop.clearYields()).toEqual(['Did create [0]']); ReactNoop.render(null); expect(ReactNoop.flush()).toEqual(['Did destroy [0]']); @@ -625,8 +816,10 @@ describe('ReactHooks', () => { return ; } ReactNoop.render(); - expect(ReactNoop.flush()).toEqual(['Count: 0', 'Did mount']); + expect(ReactNoop.flush()).toEqual(['Count: 0']); expect(ReactNoop.getChildren()).toEqual([span('Count: 0')]); + flushPassiveEffects(); + expect(ReactNoop.clearYields()).toEqual(['Did mount']); ReactNoop.render(); // No effect, because constructor was hoisted outside render @@ -653,31 +846,37 @@ describe('ReactHooks', () => { return ; } ReactNoop.render(); - expect(ReactNoop.flush()).toEqual(['Count: 0', 'Did create [Count: 0]']); + expect(ReactNoop.flush()).toEqual(['Count: 0']); + flushPassiveEffects(); + expect(ReactNoop.clearYields()).toEqual(['Did create [Count: 0]']); expect(ReactNoop.getChildren()).toEqual([span('Count: 0')]); ReactNoop.render(); // Count changed - expect(ReactNoop.flush()).toEqual([ - 'Count: 1', + expect(ReactNoop.flush()).toEqual(['Count: 1']); + expect(ReactNoop.getChildren()).toEqual([span('Count: 1')]); + flushPassiveEffects(); + expect(ReactNoop.clearYields()).toEqual([ 'Did destroy [Count: 0]', 'Did create [Count: 1]', ]); - expect(ReactNoop.getChildren()).toEqual([span('Count: 1')]); ReactNoop.render(); // Nothing changed, so no effect should have fired expect(ReactNoop.flush()).toEqual(['Count: 1']); + flushPassiveEffects(); + expect(ReactNoop.clearYields()).toEqual(null); expect(ReactNoop.getChildren()).toEqual([span('Count: 1')]); ReactNoop.render(); // Label changed - expect(ReactNoop.flush()).toEqual([ - 'Total: 1', + expect(ReactNoop.flush()).toEqual(['Total: 1']); + expect(ReactNoop.getChildren()).toEqual([span('Total: 1')]); + flushPassiveEffects(); + expect(ReactNoop.clearYields()).toEqual([ 'Did destroy [Count: 1]', 'Did create [Total: 1]', ]); - expect(ReactNoop.getChildren()).toEqual([span('Total: 1')]); }); it('multiple effects', () => { @@ -691,20 +890,22 @@ describe('ReactHooks', () => { return ; } ReactNoop.render(); - expect(ReactNoop.flush()).toEqual([ - 'Count: 0', + expect(ReactNoop.flush()).toEqual(['Count: 0']); + expect(ReactNoop.getChildren()).toEqual([span('Count: 0')]); + flushPassiveEffects(); + expect(ReactNoop.clearYields()).toEqual([ 'Did commit 1 [0]', 'Did commit 2 [0]', ]); - expect(ReactNoop.getChildren()).toEqual([span('Count: 0')]); ReactNoop.render(); - expect(ReactNoop.flush()).toEqual([ - 'Count: 1', + expect(ReactNoop.flush()).toEqual(['Count: 1']); + expect(ReactNoop.getChildren()).toEqual([span('Count: 1')]); + flushPassiveEffects(); + expect(ReactNoop.clearYields()).toEqual([ 'Did commit 1 [1]', 'Did commit 2 [1]', ]); - expect(ReactNoop.getChildren()).toEqual([span('Count: 1')]); }); it('unmounts all previous effects before creating any new ones', () => { @@ -724,22 +925,371 @@ describe('ReactHooks', () => { return ; } ReactNoop.render(); - expect(ReactNoop.flush()).toEqual([ - 'Count: 0', + expect(ReactNoop.flush()).toEqual(['Count: 0']); + expect(ReactNoop.getChildren()).toEqual([span('Count: 0')]); + flushPassiveEffects(); + expect(ReactNoop.clearYields()).toEqual(['Mount A [0]', 'Mount B [0]']); + + ReactNoop.render(); + expect(ReactNoop.flush()).toEqual(['Count: 1']); + expect(ReactNoop.getChildren()).toEqual([span('Count: 1')]); + flushPassiveEffects(); + expect(ReactNoop.clearYields()).toEqual([ + 'Unmount A [0]', + 'Unmount B [0]', + 'Mount A [1]', + 'Mount B [1]', + ]); + }); + + it('handles errors on mount', () => { + function Counter(props) { + useEffect(() => { + ReactNoop.yield(`Mount A [${props.count}]`); + return () => { + ReactNoop.yield(`Unmount A [${props.count}]`); + }; + }); + useEffect(() => { + ReactNoop.yield('Oops!'); + throw new Error('Oops!'); + // eslint-disable-next-line no-unreachable + ReactNoop.yield(`Mount B [${props.count}]`); + return () => { + ReactNoop.yield(`Unmount B [${props.count}]`); + }; + }); + return ; + } + ReactNoop.render(); + expect(ReactNoop.flush()).toEqual(['Count: 0']); + expect(ReactNoop.getChildren()).toEqual([span('Count: 0')]); + expect(() => flushPassiveEffects()).toThrow('Oops'); + expect(ReactNoop.clearYields()).toEqual([ 'Mount A [0]', - 'Mount B [0]', + 'Oops!', + // Clean up effect A. There's no effect B to clean-up, because it + // never mounted. + 'Unmount A [0]', ]); + expect(ReactNoop.getChildren()).toEqual([]); + }); + + it('handles errors on update', () => { + function Counter(props) { + useEffect(() => { + ReactNoop.yield(`Mount A [${props.count}]`); + return () => { + ReactNoop.yield(`Unmount A [${props.count}]`); + }; + }); + useEffect(() => { + if (props.count === 1) { + ReactNoop.yield('Oops!'); + throw new Error('Oops!'); + } + ReactNoop.yield(`Mount B [${props.count}]`); + return () => { + ReactNoop.yield(`Unmount B [${props.count}]`); + }; + }); + return ; + } + ReactNoop.render(); + expect(ReactNoop.flush()).toEqual(['Count: 0']); expect(ReactNoop.getChildren()).toEqual([span('Count: 0')]); + flushPassiveEffects(); + expect(ReactNoop.clearYields()).toEqual(['Mount A [0]', 'Mount B [0]']); + // This update will trigger an errror ReactNoop.render(); - expect(ReactNoop.flush()).toEqual([ - 'Count: 1', + expect(ReactNoop.flush()).toEqual(['Count: 1']); + expect(ReactNoop.getChildren()).toEqual([span('Count: 1')]); + expect(() => flushPassiveEffects()).toThrow('Oops'); + expect(ReactNoop.clearYields()).toEqual([ 'Unmount A [0]', 'Unmount B [0]', 'Mount A [1]', - 'Mount B [1]', + 'Oops!', + // Clean up effect A. There's no effect B to clean-up, because it + // never mounted. + 'Unmount A [1]', ]); + expect(ReactNoop.getChildren()).toEqual([]); + }); + + it('handles errors on unmount', () => { + function Counter(props) { + useEffect(() => { + ReactNoop.yield(`Mount A [${props.count}]`); + return () => { + ReactNoop.yield('Oops!'); + throw new Error('Oops!'); + // eslint-disable-next-line no-unreachable + ReactNoop.yield(`Unmount A [${props.count}]`); + }; + }); + useEffect(() => { + ReactNoop.yield(`Mount B [${props.count}]`); + return () => { + ReactNoop.yield(`Unmount B [${props.count}]`); + }; + }); + return ; + } + ReactNoop.render(); + expect(ReactNoop.flush()).toEqual(['Count: 0']); + expect(ReactNoop.getChildren()).toEqual([span('Count: 0')]); + flushPassiveEffects(); + expect(ReactNoop.clearYields()).toEqual(['Mount A [0]', 'Mount B [0]']); + + // This update will trigger an errror + ReactNoop.render(); + expect(ReactNoop.flush()).toEqual(['Count: 1']); expect(ReactNoop.getChildren()).toEqual([span('Count: 1')]); + expect(() => flushPassiveEffects()).toThrow('Oops'); + expect(ReactNoop.clearYields()).toEqual([ + 'Oops!', + // B unmounts even though an error was thrown in the previous effect + 'Unmount B [0]', + ]); + expect(ReactNoop.getChildren()).toEqual([]); + }); + }); + + describe('useMutationEffect and useLayoutEffect', () => { + it('fires layout effects after the host has been mutated', () => { + function getCommittedText() { + const children = ReactNoop.getChildren(); + if (children === null) { + return null; + } + return children[0].prop; + } + + function Counter(props) { + useLayoutEffect(() => { + ReactNoop.yield(`Current: ${getCommittedText()}`); + }); + return ; + } + + ReactNoop.render(); + expect(ReactNoop.flush()).toEqual([0, 'Current: 0']); + expect(ReactNoop.getChildren()).toEqual([span(0)]); + + ReactNoop.render(); + expect(ReactNoop.flush()).toEqual([1, 'Current: 1']); + expect(ReactNoop.getChildren()).toEqual([span(1)]); + }); + + it('fires mutation effects before layout effects', () => { + let committedText = '(empty)'; + + function Counter(props) { + useMutationEffect(() => { + ReactNoop.yield(`Mount mutation [current: ${committedText}]`); + committedText = props.count + ''; + return () => { + ReactNoop.yield(`Unmount mutation [current: ${committedText}]`); + }; + }); + useLayoutEffect(() => { + ReactNoop.yield(`Mount layout [current: ${committedText}]`); + return () => { + ReactNoop.yield(`Unmount layout [current: ${committedText}]`); + }; + }); + useEffect(() => { + ReactNoop.yield(`Mount normal [current: ${committedText}]`); + return () => { + ReactNoop.yield(`Unmount normal [current: ${committedText}]`); + }; + }); + return null; + } + + ReactNoop.render(); + expect(ReactNoop.flush()).toEqual([ + 'Mount mutation [current: (empty)]', + 'Mount layout [current: 0]', + ]); + expect(committedText).toEqual('0'); + flushPassiveEffects(); + expect(ReactNoop.clearYields()).toEqual(['Mount normal [current: 0]']); + + // Unmount everything + ReactNoop.render(null); + expect(ReactNoop.flush()).toEqual([ + 'Unmount mutation [current: 0]', + 'Unmount layout [current: 0]', + 'Unmount normal [current: 0]', + ]); + }); + + it('force flushes passive effects before firing new mutation effects', () => { + let committedText = '(empty)'; + + function Counter(props) { + useMutationEffect(() => { + ReactNoop.yield(`Mount mutation [current: ${committedText}]`); + committedText = props.count + ''; + return () => { + ReactNoop.yield(`Unmount mutation [current: ${committedText}]`); + }; + }); + useEffect(() => { + ReactNoop.yield(`Mount normal [current: ${committedText}]`); + return () => { + ReactNoop.yield(`Unmount normal [current: ${committedText}]`); + }; + }); + return null; + } + + ReactNoop.render(); + expect(ReactNoop.flush()).toEqual(['Mount mutation [current: (empty)]']); + expect(committedText).toEqual('0'); + + ReactNoop.render(); + expect(ReactNoop.flush()).toEqual([ + 'Mount normal [current: 0]', + 'Unmount mutation [current: 0]', + 'Mount mutation [current: 0]', + ]); + expect(committedText).toEqual('1'); + + flushPassiveEffects(); + expect(ReactNoop.clearYields()).toEqual(['Mount normal [current: 1]']); + }); + + it('force flushes passive effects before firing new layout effects', () => { + let committedText = '(empty)'; + + function Counter(props) { + useLayoutEffect(() => { + // Normally this would go in a mutation effect, but this test + // intentionally omits a mutation effect. + committedText = props.count + ''; + + ReactNoop.yield(`Mount layout [current: ${committedText}]`); + return () => { + ReactNoop.yield(`Unmount layout [current: ${committedText}]`); + }; + }); + useEffect(() => { + ReactNoop.yield(`Mount normal [current: ${committedText}]`); + return () => { + ReactNoop.yield(`Unmount normal [current: ${committedText}]`); + }; + }); + return null; + } + + ReactNoop.render(); + expect(ReactNoop.flush()).toEqual(['Mount layout [current: 0]']); + expect(committedText).toEqual('0'); + + ReactNoop.render(); + expect(ReactNoop.flush()).toEqual([ + 'Mount normal [current: 0]', + 'Unmount layout [current: 0]', + 'Mount layout [current: 1]', + ]); + expect(committedText).toEqual('1'); + + flushPassiveEffects(); + expect(ReactNoop.clearYields()).toEqual(['Mount normal [current: 1]']); + }); + + it('fires all mutation effects before firing any layout effects', () => { + let committedA = '(empty)'; + let committedB = '(empty)'; + + function CounterA(props) { + useMutationEffect(() => { + ReactNoop.yield( + `Mount A mutation [A: ${committedA}, B: ${committedB}]`, + ); + committedA = props.count + ''; + return () => { + ReactNoop.yield( + `Unmount A mutation [A: ${committedA}, B: ${committedB}]`, + ); + }; + }); + useLayoutEffect(() => { + ReactNoop.yield( + `Mount layout A [A: ${committedA}, B: ${committedB}]`, + ); + return () => { + ReactNoop.yield( + `Unmount layout A [A: ${committedA}, B: ${committedB}]`, + ); + }; + }); + return null; + } + + function CounterB(props) { + useMutationEffect(() => { + ReactNoop.yield( + `Mount B mutation [A: ${committedA}, B: ${committedB}]`, + ); + committedB = props.count + ''; + return () => { + ReactNoop.yield( + `Unmount B mutation [A: ${committedA}, B: ${committedB}]`, + ); + }; + }); + useLayoutEffect(() => { + ReactNoop.yield( + `Mount layout B [A: ${committedA}, B: ${committedB}]`, + ); + return () => { + ReactNoop.yield( + `Unmount layout B [A: ${committedA}, B: ${committedB}]`, + ); + }; + }); + return null; + } + + ReactNoop.render( + + + + , + ); + expect(ReactNoop.flush()).toEqual([ + // All mutation effects fire before all layout effects + 'Mount A mutation [A: (empty), B: (empty)]', + 'Mount B mutation [A: 0, B: (empty)]', + 'Mount layout A [A: 0, B: 0]', + 'Mount layout B [A: 0, B: 0]', + ]); + expect([committedA, committedB]).toEqual(['0', '0']); + + ReactNoop.render( + + + + , + ); + expect(ReactNoop.flush()).toEqual([ + // Note: This shows that the clean-up function of a layout effect is + // fired in the same phase as the set-up function of a mutation. + 'Unmount A mutation [A: 0, B: 0]', + 'Unmount B mutation [A: 0, B: 0]', + 'Mount A mutation [A: 0, B: 0]', + 'Unmount layout A [A: 1, B: 0]', + 'Mount B mutation [A: 1, B: 0]', + 'Unmount layout B [A: 1, B: 1]', + 'Mount layout A [A: 1, B: 1]', + 'Mount layout B [A: 1, B: 1]', + ]); + expect([committedA, committedB]).toEqual(['1', '1']); }); }); @@ -1006,26 +1556,11 @@ describe('ReactHooks', () => { updateC(4); expect(ReactNoop.flush()).toEqual(['A: 2, B: 3, C: 4']); expect(ReactNoop.getChildren()).toEqual([span('A: 2, B: 3, C: 4')]); - ReactNoop.render(); - expect(ReactNoop.flush()).toEqual(['A: 2, B: 3, C: [not loaded]']); - expect(ReactNoop.getChildren()).toEqual([ - span('A: 2, B: 3, C: [not loaded]'), - ]); - - updateC(4); - // TODO: This hook triggered a re-render even though it's unmounted. - // Should we warn? - expect(ReactNoop.flush()).toEqual(['A: 2, B: 3, C: [not loaded]']); - expect(ReactNoop.getChildren()).toEqual([ - span('A: 2, B: 3, C: [not loaded]'), - ]); - - updateB(4); - expect(ReactNoop.flush()).toEqual(['A: 2, B: 4, C: [not loaded]']); - expect(ReactNoop.getChildren()).toEqual([ - span('A: 2, B: 4, C: [not loaded]'), - ]); + expect(() => ReactNoop.flush()).toThrow( + 'Rendered fewer hooks than expected. This may be caused by an ' + + 'accidental early return statement.', + ); }); it('unmount effects', () => { @@ -1050,16 +1585,20 @@ describe('ReactHooks', () => { } ReactNoop.render(); - expect(ReactNoop.flush()).toEqual(['Mount A']); + expect(ReactNoop.flush()).toEqual([]); + flushPassiveEffects(); + expect(ReactNoop.clearYields()).toEqual(['Mount A']); ReactNoop.render(); - expect(ReactNoop.flush()).toEqual(['Mount B']); + expect(ReactNoop.flush()).toEqual([]); + flushPassiveEffects(); + expect(ReactNoop.clearYields()).toEqual(['Mount B']); ReactNoop.render(); - expect(ReactNoop.flush()).toEqual(['Unmount B']); - - ReactNoop.render(null); - expect(ReactNoop.flush()).toEqual(['Unmount A']); + expect(() => ReactNoop.flush()).toThrow( + 'Rendered fewer hooks than expected. This may be caused by an ' + + 'accidental early return statement.', + ); }); }); }); diff --git a/packages/react-reconciler/src/__tests__/ReactNewContext-test.internal.js b/packages/react-reconciler/src/__tests__/ReactNewContext-test.internal.js index 5aace46d135b1..194539e6ec84a 100644 --- a/packages/react-reconciler/src/__tests__/ReactNewContext-test.internal.js +++ b/packages/react-reconciler/src/__tests__/ReactNewContext-test.internal.js @@ -37,6 +37,13 @@ describe('ReactNewContext', () => { return {type: 'span', children: [], prop, hidden: false}; } + function readContext(Context, observedBits) { + const dispatcher = + React.__SECRET_INTERNALS_DO_NOT_USE_OR_YOU_WILL_BE_FIRED.ReactCurrentOwner + .currentDispatcher; + return dispatcher.readContext(Context, observedBits); + } + // We have several ways of reading from context. sharedContextTests runs // a suite of tests for a given context consumer implementation. sharedContextTests('Context.Consumer', Context => Context.Consumer); @@ -51,12 +58,12 @@ describe('ReactNewContext', () => { }, ); sharedContextTests( - 'useContext inside class component', + 'readContext(Context) inside class component', Context => class Consumer extends React.Component { render() { const observedBits = this.props.unstable_observedBits; - const contextValue = useContext(Context, observedBits); + const contextValue = readContext(Context, observedBits); const render = this.props.children; return render(contextValue); } @@ -1190,7 +1197,7 @@ describe('ReactNewContext', () => { return ( {foo => { - const bar = useContext(BarContext); + const bar = readContext(BarContext); return ; }} @@ -1325,6 +1332,19 @@ describe('ReactNewContext', () => { span('Baz: 2'), ]); }); + + it('throws when used in a class component', () => { + const Context = React.createContext(0); + class Foo extends React.Component { + render() { + return useContext(Context); + } + } + ReactNoop.render(); + expect(ReactNoop.flush).toThrow( + 'Hooks can only be called inside the body of a functional component.', + ); + }); }); it('unwinds after errors in complete phase', () => { diff --git a/packages/react/src/React.js b/packages/react/src/React.js index 1213fa50b4e4a..b2bbf2e8d2fa9 100644 --- a/packages/react/src/React.js +++ b/packages/react/src/React.js @@ -29,14 +29,16 @@ import {lazy} from './ReactLazy'; import forwardRef from './forwardRef'; import memo from './memo'; import { + useAPI, + useCallback, useContext, - useState, - useReducer, - useRef, useEffect, - useCallback, + useLayoutEffect, useMemo, - useAPI, + useMutationEffect, + useReducer, + useRef, + useState, } from './ReactHooks'; import { createElementWithValidation, @@ -87,14 +89,16 @@ if (enableStableConcurrentModeAPIs) { } if (enableHooks) { + React.useAPI = useAPI; + React.useCallback = useCallback; React.useContext = useContext; - React.useState = useState; - React.useReducer = useReducer; - React.useRef = useRef; React.useEffect = useEffect; - React.useCallback = useCallback; + React.useLayoutEffect = useLayoutEffect; React.useMemo = useMemo; - React.useAPI = useAPI; + React.useMutationEffect = useMutationEffect; + React.useReducer = useReducer; + React.useRef = useRef; + React.useState = useState; } export default React; diff --git a/packages/react/src/ReactHooks.js b/packages/react/src/ReactHooks.js index 89b5b2977c7b1..1c9b7271ea905 100644 --- a/packages/react/src/ReactHooks.js +++ b/packages/react/src/ReactHooks.js @@ -8,7 +8,6 @@ */ import type {ReactContext} from 'shared/ReactTypes'; - import invariant from 'shared/invariant'; import ReactCurrentOwner from './ReactCurrentOwner'; @@ -27,7 +26,7 @@ export function useContext( observedBits: number | boolean | void, ) { const dispatcher = resolveDispatcher(); - return dispatcher.readContext(Context, observedBits); + return dispatcher.useContext(Context, observedBits); } export function useState(initialState: S | (() => S)) { @@ -57,6 +56,22 @@ export function useEffect( return dispatcher.useEffect(create, inputs); } +export function useMutationEffect( + create: () => mixed, + inputs: Array | void | null, +) { + const dispatcher = resolveDispatcher(); + return dispatcher.useMutationEffect(create, inputs); +} + +export function useLayoutEffect( + create: () => mixed, + inputs: Array | void | null, +) { + const dispatcher = resolveDispatcher(); + return dispatcher.useLayoutEffect(create, inputs); +} + export function useCallback( callback: () => mixed, inputs: Array | void | null, diff --git a/packages/shared/ReactSideEffectTags.js b/packages/shared/ReactSideEffectTags.js index d6a2c6a06b442..5f3c47f8fd593 100644 --- a/packages/shared/ReactSideEffectTags.js +++ b/packages/shared/ReactSideEffectTags.js @@ -10,25 +10,26 @@ export type SideEffectTag = number; // Don't change these two values. They're used by React Dev Tools. -export const NoEffect = /* */ 0b00000000000; -export const PerformedWork = /* */ 0b00000000001; +export const NoEffect = /* */ 0b000000000000; +export const PerformedWork = /* */ 0b000000000001; // You can change the rest (and add more). -export const Placement = /* */ 0b00000000010; -export const Update = /* */ 0b00000000100; -export const PlacementAndUpdate = /* */ 0b00000000110; -export const Deletion = /* */ 0b00000001000; -export const ContentReset = /* */ 0b00000010000; -export const Callback = /* */ 0b00000100000; -export const DidCapture = /* */ 0b00001000000; -export const Ref = /* */ 0b00010000000; -export const Snapshot = /* */ 0b00100000000; +export const Placement = /* */ 0b000000000010; +export const Update = /* */ 0b000000000100; +export const PlacementAndUpdate = /* */ 0b000000000110; +export const Deletion = /* */ 0b000000001000; +export const ContentReset = /* */ 0b000000010000; +export const Callback = /* */ 0b000000100000; +export const DidCapture = /* */ 0b000001000000; +export const Ref = /* */ 0b000010000000; +export const Snapshot = /* */ 0b000100000000; +export const Passive = /* */ 0b001000000000; -// Update & Callback & Ref & Snapshot -export const LifecycleEffectMask = /* */ 0b00110100100; +// Passive & Update & Callback & Ref & Snapshot +export const LifecycleEffectMask = /* */ 0b001110100100; // Union of all host effects -export const HostEffectMask = /* */ 0b00111111111; +export const HostEffectMask = /* */ 0b001111111111; -export const Incomplete = /* */ 0b01000000000; -export const ShouldCapture = /* */ 0b10000000000; +export const Incomplete = /* */ 0b010000000000; +export const ShouldCapture = /* */ 0b100000000000; From dd019d34db1d8d067637033c36856c8b259cb35b Mon Sep 17 00:00:00 2001 From: Alex Taylor Date: Sun, 30 Sep 2018 18:32:11 -0600 Subject: [PATCH 04/19] Add support for hooks to ReactDOMServer Co-authored-by: Alex Taylor Co-authored-by: Andrew Clark --- ...DOMServerIntegrationHooks-test.internal.js | 650 ++++++++++++++++++ .../src/server/ReactPartialRenderer.js | 30 +- .../src/server/ReactPartialRendererHooks.js | 366 ++++++++++ .../react-reconciler/src/ReactFiberHooks.js | 124 ++-- .../src/__tests__/ReactHooks-test.internal.js | 51 ++ 5 files changed, 1149 insertions(+), 72 deletions(-) create mode 100644 packages/react-dom/src/__tests__/ReactDOMServerIntegrationHooks-test.internal.js create mode 100644 packages/react-dom/src/server/ReactPartialRendererHooks.js diff --git a/packages/react-dom/src/__tests__/ReactDOMServerIntegrationHooks-test.internal.js b/packages/react-dom/src/__tests__/ReactDOMServerIntegrationHooks-test.internal.js new file mode 100644 index 0000000000000..99d6ccbb783d9 --- /dev/null +++ b/packages/react-dom/src/__tests__/ReactDOMServerIntegrationHooks-test.internal.js @@ -0,0 +1,650 @@ +/** + * Copyright (c) 2013-present, Facebook, Inc. + * + * This source code is licensed under the MIT license found in the + * LICENSE file in the root directory of this source tree. + * + * @emails react-core + */ + +/* eslint-disable no-func-assign */ + +'use strict'; + +const ReactDOMServerIntegrationUtils = require('./utils/ReactDOMServerIntegrationTestUtils'); + +let React; +let ReactFeatureFlags; +let ReactDOM; +let ReactDOMServer; +let useState; +let useReducer; +let useEffect; +let useContext; +let useCallback; +let useMemo; +let useRef; +let useAPI; +let useMutationEffect; +let useLayoutEffect; +let forwardRef; +let yieldedValues; +let yieldValue; +let clearYields; + +function initModules() { + // Reset warning cache. + jest.resetModuleRegistry(); + + ReactFeatureFlags = require('shared/ReactFeatureFlags'); + ReactFeatureFlags.debugRenderPhaseSideEffectsForStrictMode = false; + ReactFeatureFlags.enableHooks = true; + React = require('react'); + ReactDOM = require('react-dom'); + ReactDOMServer = require('react-dom/server'); + useState = React.useState; + useReducer = React.useReducer; + useEffect = React.useEffect; + useContext = React.useContext; + useCallback = React.useCallback; + useMemo = React.useMemo; + useRef = React.useRef; + useAPI = React.useAPI; + useMutationEffect = React.useMutationEffect; + useLayoutEffect = React.useLayoutEffect; + forwardRef = React.forwardRef; + + yieldedValues = []; + yieldValue = value => { + yieldedValues.push(value); + }; + clearYields = () => { + const ret = yieldedValues; + yieldedValues = []; + return ret; + }; + + // Make them available to the helpers. + return { + ReactDOM, + ReactDOMServer, + }; +} + +const { + resetModules, + itRenders, + itThrowsWhenRendering, + serverRender, +} = ReactDOMServerIntegrationUtils(initModules); + +describe('ReactDOMServerHooks', () => { + beforeEach(() => { + resetModules(); + }); + + function Text(props) { + yieldValue(props.text); + return {props.text}; + } + + describe('useState', () => { + itRenders('basic render', async render => { + function Counter(props) { + const [count] = useState(0); + return Count: {count}; + } + + const domNode = await render(); + expect(domNode.textContent).toEqual('Count: 0'); + }); + + itRenders('lazy state initialization', async render => { + function Counter(props) { + const [count] = useState(() => { + return 0; + }); + return Count: {count}; + } + + const domNode = await render(); + expect(domNode.textContent).toEqual('Count: 0'); + }); + + it('does not trigger a re-renders when updater is invoked outside current render function', async () => { + function UpdateCount({setCount, count, children}) { + if (count < 3) { + setCount(c => c + 1); + } + return {children}; + } + function Counter() { + let [count, setCount] = useState(0); + return ( +
+ + Count: {count} + +
+ ); + } + + const domNode = await serverRender(); + expect(domNode.textContent).toEqual('Count: 0'); + }); + + itThrowsWhenRendering( + 'if used inside a class component', + async render => { + class Counter extends React.Component { + render() { + let [count] = useState(0); + return ; + } + } + + return render(); + }, + 'Hooks can only be called inside the body of a functional component.', + ); + + itRenders('multiple times when an updater is called', async render => { + function Counter() { + let [count, setCount] = useState(0); + if (count < 12) { + setCount(c => c + 1); + setCount(c => c + 1); + setCount(c => c + 1); + } + return ; + } + + const domNode = await render(); + expect(domNode.textContent).toEqual('Count: 12'); + }); + + itRenders('until there are no more new updates', async render => { + function Counter() { + let [count, setCount] = useState(0); + if (count < 3) { + setCount(count + 1); + } + return Count: {count}; + } + + const domNode = await render(); + expect(domNode.textContent).toEqual('Count: 3'); + }); + + itThrowsWhenRendering( + 'after too many iterations', + async render => { + function Counter() { + let [count, setCount] = useState(0); + setCount(count + 1); + return {count}; + } + return render(); + }, + 'Too many re-renders. React limits the number of renders to prevent ' + + 'an infinite loop.', + ); + }); + + describe('useReducer', () => { + itRenders('with initial state', async render => { + function reducer(state, action) { + return action === 'increment' ? state + 1 : state; + } + function Counter() { + let [count] = useReducer(reducer, 0); + yieldValue('Render: ' + count); + return ; + } + + const domNode = await render(); + + expect(clearYields()).toEqual(['Render: 0', 0]); + expect(domNode.tagName).toEqual('SPAN'); + expect(domNode.textContent).toEqual('0'); + }); + + itRenders('lazy initialization with initialAction', async render => { + function reducer(state, action) { + return action === 'increment' ? state + 1 : state; + } + function Counter() { + let [count] = useReducer(reducer, 0, 'increment'); + yieldValue('Render: ' + count); + return ; + } + + const domNode = await render(); + + expect(clearYields()).toEqual(['Render: 1', 1]); + expect(domNode.tagName).toEqual('SPAN'); + expect(domNode.textContent).toEqual('1'); + }); + + itRenders( + 'multiple times when updates happen during the render phase', + async render => { + function reducer(state, action) { + return action === 'increment' ? state + 1 : state; + } + function Counter() { + let [count, dispatch] = useReducer(reducer, 0); + if (count < 3) { + dispatch('increment'); + } + yieldValue('Render: ' + count); + return ; + } + + const domNode = await render(); + + expect(clearYields()).toEqual([ + 'Render: 0', + 'Render: 1', + 'Render: 2', + 'Render: 3', + 3, + ]); + expect(domNode.tagName).toEqual('SPAN'); + expect(domNode.textContent).toEqual('3'); + }, + ); + + itRenders( + 'using reducer passed at time of render, not time of dispatch', + async render => { + // This test is a bit contrived but it demonstrates a subtle edge case. + + // Reducer A increments by 1. Reducer B increments by 10. + function reducerA(state, action) { + switch (action) { + case 'increment': + return state + 1; + case 'reset': + return 0; + } + } + function reducerB(state, action) { + switch (action) { + case 'increment': + return state + 10; + case 'reset': + return 0; + } + } + + function Counter() { + let [reducer, setReducer] = useState(() => reducerA); + let [count, dispatch] = useReducer(reducer, 0); + if (count < 20) { + dispatch('increment'); + // Swap reducers each time we increment + if (reducer === reducerA) { + setReducer(() => reducerB); + } else { + setReducer(() => reducerA); + } + } + yieldValue('Render: ' + count); + return ; + } + + const domNode = await render(); + + expect(clearYields()).toEqual([ + // The count should increase by alternating amounts of 10 and 1 + // until we reach 21. + 'Render: 0', + 'Render: 10', + 'Render: 11', + 'Render: 21', + 21, + ]); + expect(domNode.tagName).toEqual('SPAN'); + expect(domNode.textContent).toEqual('21'); + }, + ); + }); + + describe('useMemo', () => { + itRenders('basic render', async render => { + function CapitalizedText(props) { + const text = props.text; + const capitalizedText = useMemo( + () => { + yieldValue(`Capitalize '${text}'`); + return text.toUpperCase(); + }, + [text], + ); + return ; + } + + const domNode = await render(); + expect(clearYields()).toEqual(["Capitalize 'hello'", 'HELLO']); + expect(domNode.tagName).toEqual('SPAN'); + expect(domNode.textContent).toEqual('HELLO'); + }); + + itRenders('if no inputs are provided', async render => { + function LazyCompute(props) { + const computed = useMemo(props.compute); + return ; + } + + function computeA() { + yieldValue('compute A'); + return 'A'; + } + + const domNode = await render(); + expect(clearYields()).toEqual(['compute A', 'A']); + expect(domNode.tagName).toEqual('SPAN'); + expect(domNode.textContent).toEqual('A'); + }); + + itRenders( + 'multiple times when updates happen during the render phase', + async render => { + function CapitalizedText(props) { + const [text, setText] = useState(props.text); + const capitalizedText = useMemo( + () => { + yieldValue(`Capitalize '${text}'`); + return text.toUpperCase(); + }, + [text], + ); + + if (text === 'hello') { + setText('hello, world.'); + } + return ; + } + + const domNode = await render(); + expect(clearYields()).toEqual([ + "Capitalize 'hello'", + "Capitalize 'hello, world.'", + 'HELLO, WORLD.', + ]); + expect(domNode.tagName).toEqual('SPAN'); + expect(domNode.textContent).toEqual('HELLO, WORLD.'); + }, + ); + + itRenders( + 'should only invoke the memoized function when the inputs change', + async render => { + function CapitalizedText(props) { + const [text, setText] = useState(props.text); + const [count, setCount] = useState(0); + const capitalizedText = useMemo( + () => { + yieldValue(`Capitalize '${text}'`); + return text.toUpperCase(); + }, + [text], + ); + + yieldValue(count); + + if (count < 3) { + setCount(count + 1); + } + + if (text === 'hello' && count === 2) { + setText('hello, world.'); + } + return ; + } + + const domNode = await render(); + expect(clearYields()).toEqual([ + "Capitalize 'hello'", + 0, + 1, + 2, + // `capitalizedText` only recomputes when the text has changed + "Capitalize 'hello, world.'", + 3, + 'HELLO, WORLD.', + ]); + expect(domNode.tagName).toEqual('SPAN'); + expect(domNode.textContent).toEqual('HELLO, WORLD.'); + }, + ); + }); + + describe('useRef', () => { + itRenders('basic render', async render => { + function Counter(props) { + const count = useRef(0); + return Count: {count.current}; + } + + const domNode = await render(); + expect(domNode.textContent).toEqual('Count: 0'); + }); + + itRenders( + 'multiple times when updates happen during the render phase', + async render => { + function Counter(props) { + const [count, setCount] = useState(0); + const ref = useRef(count); + + if (count < 3) { + const newCount = count + 1; + + ref.current = newCount; + setCount(newCount); + } + + yieldValue(count); + + return Count: {ref.current}; + } + + const domNode = await render(); + expect(clearYields()).toEqual([0, 1, 2, 3]); + expect(domNode.textContent).toEqual('Count: 3'); + }, + ); + + itRenders( + 'always return the same reference through multiple renders', + async render => { + let firstRef = null; + function Counter(props) { + const [count, setCount] = useState(0); + const ref = useRef(count); + if (firstRef === null) { + firstRef = ref; + } else if (firstRef !== ref) { + throw new Error('should never change'); + } + + if (count < 3) { + setCount(count + 1); + } else { + firstRef = null; + } + + yieldValue(count); + + return Count: {ref.current}; + } + + const domNode = await render(); + expect(clearYields()).toEqual([0, 1, 2, 3]); + expect(domNode.textContent).toEqual('Count: 0'); + }, + ); + }); + + describe('useEffect', () => { + itRenders('should ignore effects on the server', async render => { + function Counter(props) { + useEffect(() => { + yieldValue('should not be invoked'); + }); + return ; + } + const domNode = await render(); + expect(clearYields()).toEqual(['Count: 0']); + expect(domNode.tagName).toEqual('SPAN'); + expect(domNode.textContent).toEqual('Count: 0'); + }); + }); + + describe('useCallback', () => { + itRenders('should ignore callbacks on the server', async render => { + function Counter(props) { + useCallback(() => { + yieldValue('should not be invoked'); + }); + return ; + } + const domNode = await render(); + expect(clearYields()).toEqual(['Count: 0']); + expect(domNode.tagName).toEqual('SPAN'); + expect(domNode.textContent).toEqual('Count: 0'); + }); + }); + + describe('useAPI', () => { + it('should not be invoked on the server', async () => { + function Counter(props, ref) { + useAPI(ref, () => { + throw new Error('should not be invoked'); + }); + return ; + } + Counter = forwardRef(Counter); + const counter = React.createRef(); + counter.current = 0; + const domNode = await serverRender( + , + ); + expect(clearYields()).toEqual(['Count: 0']); + expect(domNode.tagName).toEqual('SPAN'); + expect(domNode.textContent).toEqual('Count: 0'); + }); + }); + + describe('useMutationEffect', () => { + it('should warn when invoked during render', async () => { + function Counter() { + useMutationEffect(() => { + throw new Error('should not be invoked'); + }); + + return ; + } + const domNode = await serverRender(, 1); + expect(clearYields()).toEqual(['Count: 0']); + expect(domNode.tagName).toEqual('SPAN'); + expect(domNode.textContent).toEqual('Count: 0'); + }); + }); + + describe('useLayoutEffect', () => { + it('should warn when invoked during render', async () => { + function Counter() { + useLayoutEffect(() => { + throw new Error('should not be invoked'); + }); + + return ; + } + const domNode = await serverRender(, 1); + expect(clearYields()).toEqual(['Count: 0']); + expect(domNode.tagName).toEqual('SPAN'); + expect(domNode.textContent).toEqual('Count: 0'); + }); + }); + + describe('useContext', () => { + itRenders( + 'can use the same context multiple times in the same function', + async render => { + const Context = React.createContext( + {foo: 0, bar: 0, baz: 0}, + (a, b) => { + let result = 0; + if (a.foo !== b.foo) { + result |= 0b001; + } + if (a.bar !== b.bar) { + result |= 0b010; + } + if (a.baz !== b.baz) { + result |= 0b100; + } + return result; + }, + ); + + function Provider(props) { + return ( + + {props.children} + + ); + } + + function FooAndBar() { + const {foo} = useContext(Context, 0b001); + const {bar} = useContext(Context, 0b010); + return ; + } + + function Baz() { + const {baz} = useContext(Context, 0b100); + return ; + } + + class Indirection extends React.Component { + shouldComponentUpdate() { + return false; + } + render() { + return this.props.children; + } + } + + function App(props) { + return ( +
+ + + + + + + + + + +
+ ); + } + + const domNode = await render(); + expect(clearYields()).toEqual(['Foo: 1, Bar: 3', 'Baz: 5']); + expect(domNode.childNodes.length).toBe(2); + expect(domNode.firstChild.tagName).toEqual('SPAN'); + expect(domNode.firstChild.textContent).toEqual('Foo: 1, Bar: 3'); + expect(domNode.lastChild.tagName).toEqual('SPAN'); + expect(domNode.lastChild.textContent).toEqual('Baz: 5'); + }, + ); + }); +}); diff --git a/packages/react-dom/src/server/ReactPartialRenderer.js b/packages/react-dom/src/server/ReactPartialRenderer.js index 6b482be43e378..1ee885c68abc2 100644 --- a/packages/react-dom/src/server/ReactPartialRenderer.js +++ b/packages/react-dom/src/server/ReactPartialRenderer.js @@ -48,6 +48,11 @@ import { createMarkupForRoot, } from './DOMMarkupOperations'; import escapeTextForBrowser from './escapeTextForBrowser'; +import { + prepareToUseHooks, + finishHooks, + Dispatcher, +} from './ReactPartialRendererHooks'; import { Namespaces, getIntrinsicNamespace, @@ -87,15 +92,6 @@ let pushCurrentDebugStack = (stack: Array) => {}; let pushElementToDebugStack = (element: ReactElement) => {}; let popCurrentDebugStack = () => {}; -let Dispatcher = { - readContext( - context: ReactContext, - observedBits: void | number | boolean, - ): T { - return context._currentValue; - }, -}; - if (__DEV__) { ReactDebugCurrentFrame = ReactSharedInternals.ReactDebugCurrentFrame; @@ -573,7 +569,11 @@ function resolve( } } } + const componentIdentity = {}; + prepareToUseHooks(componentIdentity); inst = Component(element.props, publicContext, updater); + inst = finishHooks(Component, element.props, inst, publicContext); + if (inst == null || inst.render == null) { child = inst; validateRenderResult(child, Component); @@ -985,9 +985,17 @@ class ReactDOMServerRenderer { switch (elementType.$$typeof) { case REACT_FORWARD_REF_TYPE: { const element: ReactElement = ((nextChild: any): ReactElement); - const nextChildren = toArray( - elementType.render(element.props, element.ref), + let nextChildren; + const componentIdentity = {}; + prepareToUseHooks(componentIdentity); + nextChildren = elementType.render(element.props, element.ref); + nextChildren = finishHooks( + elementType.render, + element.props, + nextChildren, + element.ref, ); + nextChildren = toArray(nextChildren); const frame: Frame = { type: null, domNamespace: parentNamespace, diff --git a/packages/react-dom/src/server/ReactPartialRendererHooks.js b/packages/react-dom/src/server/ReactPartialRendererHooks.js new file mode 100644 index 0000000000000..bc2d34d6d0922 --- /dev/null +++ b/packages/react-dom/src/server/ReactPartialRendererHooks.js @@ -0,0 +1,366 @@ +/** + * Copyright (c) Facebook, Inc. and its affiliates. + * + * This source code is licensed under the MIT license found in the + * LICENSE file in the root directory of this source tree. + * + * @flow + */ +import type {ReactContext} from 'shared/ReactTypes'; + +import invariant from 'shared/invariant'; +import warning from 'shared/warning'; + +type BasicStateAction = S | (S => S); +type MaybeCallback = void | null | (S => mixed); +type Dispatch = (A, MaybeCallback) => void; + +type Update = { + action: A, + next: Update | null, +}; + +type UpdateQueue = { + last: Update | null, + dispatch: any, +}; + +type Hook = { + memoizedState: any, + queue: UpdateQueue | null, + next: Hook | null, +}; + +let currentlyRenderingComponent: Object | null = null; +let firstWorkInProgressHook: Hook | null = null; +let workInProgressHook: Hook | null = null; +// Whether the work-in-progress hook is a re-rendered hook +let isReRender: boolean = false; +// Whether an update was scheduled during the currently executing render pass. +let didScheduleRenderPhaseUpdate: boolean = false; +// Lazily created map of render-phase updates +let renderPhaseUpdates: Map< + UpdateQueue, + Update, +> | null = null; +// Counter to prevent infinite loops. +let numberOfReRenders: number = 0; +const RE_RENDER_LIMIT = 25; + +function resolveCurrentlyRenderingComponent(): Object { + invariant( + currentlyRenderingComponent !== null, + 'Hooks can only be called inside the body of a functional component.', + ); + return currentlyRenderingComponent; +} + +function createHook(): Hook { + return { + memoizedState: null, + queue: null, + next: null, + }; +} + +function createWorkInProgressHook(): Hook { + if (workInProgressHook === null) { + // This is the first hook in the list + if (firstWorkInProgressHook === null) { + isReRender = false; + firstWorkInProgressHook = workInProgressHook = createHook(); + } else { + // There's already a work-in-progress. Reuse it. + isReRender = true; + workInProgressHook = firstWorkInProgressHook; + } + } else { + if (workInProgressHook.next === null) { + isReRender = false; + // Append to the end of the list + workInProgressHook = workInProgressHook.next = createHook(); + } else { + // There's already a work-in-progress. Reuse it. + isReRender = true; + workInProgressHook = workInProgressHook.next; + } + } + return workInProgressHook; +} + +export function prepareToUseHooks(componentIdentity: Object): void { + currentlyRenderingComponent = componentIdentity; + + // The following should have already been reset + // didScheduleRenderPhaseUpdate = false; + // firstWorkInProgressHook = null; + // numberOfReRenders = 0; + // renderPhaseUpdates = null; + // workInProgressHook = null; +} + +export function finishHooks( + Component: any, + props: any, + children: any, + refOrContext: any, +): any { + // This must be called after every functional component to prevent hooks from + // being used in classes. + + while (didScheduleRenderPhaseUpdate) { + // Updates were scheduled during the render phase. They are stored in + // the `renderPhaseUpdates` map. Call the component again, reusing the + // work-in-progress hooks and applying the additional updates on top. Keep + // restarting until no more updates are scheduled. + didScheduleRenderPhaseUpdate = false; + numberOfReRenders += 1; + + // Start over from the beginning of the list + workInProgressHook = null; + + children = Component(props, refOrContext); + } + currentlyRenderingComponent = null; + firstWorkInProgressHook = null; + numberOfReRenders = 0; + renderPhaseUpdates = null; + workInProgressHook = null; + + // These were reset above + // currentlyRenderingComponent = null; + // didScheduleRenderPhaseUpdate = false; + // firstWorkInProgressHook = null; + // numberOfReRenders = 0; + // renderPhaseUpdates = null; + // workInProgressHook = null; + + return children; +} + +function useContext( + context: ReactContext, + observedBits: void | number | boolean, +): T { + return context._currentValue; +} + +function basicStateReducer(state: S, action: BasicStateAction): S { + return typeof action === 'function' ? action(state) : action; +} + +export function useState( + initialState: S | (() => S), +): [S, Dispatch>] { + return useReducer( + basicStateReducer, + // useReducer has a special case to support lazy useState initializers + (initialState: any), + ); +} + +export function useReducer( + reducer: (S, A) => S, + initialState: S, + initialAction: A | void | null, +): [S, Dispatch] { + currentlyRenderingComponent = resolveCurrentlyRenderingComponent(); + workInProgressHook = createWorkInProgressHook(); + if (isReRender) { + // This is a re-render. Apply the new render phase updates to the previous + // current hook. + const queue: UpdateQueue = (workInProgressHook.queue: any); + const dispatch: Dispatch = (queue.dispatch: any); + if (renderPhaseUpdates !== null) { + // Render phase updates are stored in a map of queue -> linked list + const firstRenderPhaseUpdate = renderPhaseUpdates.get(queue); + if (firstRenderPhaseUpdate !== undefined) { + renderPhaseUpdates.delete(queue); + let newState = workInProgressHook.memoizedState; + let update = firstRenderPhaseUpdate; + do { + // Process this render phase update. We don't have to check the + // priority because it will always be the same as the current + // render's. + const action = update.action; + newState = reducer(newState, action); + update = update.next; + } while (update !== null); + + workInProgressHook.memoizedState = newState; + + return [newState, dispatch]; + } + } + return [workInProgressHook.memoizedState, dispatch]; + } else { + if (reducer === basicStateReducer) { + // Special case for `useState`. + if (typeof initialState === 'function') { + initialState = initialState(); + } + } else if (initialAction !== undefined && initialAction !== null) { + initialState = reducer(initialState, initialAction); + } + workInProgressHook.memoizedState = initialState; + const queue: UpdateQueue = (workInProgressHook.queue = { + last: null, + dispatch: null, + }); + const dispatch: Dispatch = (queue.dispatch = (dispatchAction.bind( + null, + currentlyRenderingComponent, + queue, + ): any)); + return [workInProgressHook.memoizedState, dispatch]; + } +} + +function useMemo( + nextCreate: () => T, + inputs: Array | void | null, +): T { + currentlyRenderingComponent = resolveCurrentlyRenderingComponent(); + workInProgressHook = createWorkInProgressHook(); + + const nextInputs = + inputs !== undefined && inputs !== null ? inputs : [nextCreate]; + + if ( + workInProgressHook !== null && + workInProgressHook.memoizedState !== null + ) { + const prevState = workInProgressHook.memoizedState; + const prevInputs = prevState[1]; + if (inputsAreEqual(nextInputs, prevInputs)) { + return prevState[0]; + } + } + + const nextValue = nextCreate(); + workInProgressHook.memoizedState = [nextValue, nextInputs]; + return nextValue; +} + +function useRef(initialValue: T): {current: T} { + currentlyRenderingComponent = resolveCurrentlyRenderingComponent(); + workInProgressHook = createWorkInProgressHook(); + const previousRef = workInProgressHook.memoizedState; + if (previousRef === null) { + const ref = {current: initialValue}; + if (__DEV__) { + Object.seal(ref); + } + workInProgressHook.memoizedState = ref; + return ref; + } else { + return previousRef; + } +} + +function useMutationEffect( + create: () => mixed, + inputs: Array | void | null, +) { + warning( + false, + 'useMutationEffect does nothing on the server, because its effect cannot ' + + "be encoded into the server renderer's output format. This will lead " + + 'to a mismatch between the initial, non-hydrated UI and the intended ' + + 'UI. To avoid this, useMutationEffect should only be used in ' + + 'components that render exclusively on the client.', + ); +} + +export function useLayoutEffect( + create: () => mixed, + inputs: Array | void | null, +) { + warning( + false, + 'useLayoutEffect does nothing on the server, because its effect cannot ' + + "be encoded into the server renderer's output format. This will lead " + + 'to a mismatch between the initial, non-hydrated UI and the intended ' + + 'UI. To avoid this, useLayoutEffect should only be used in ' + + 'components that render exclusively on the client.', + ); +} + +function dispatchAction( + componentIdentity: Object, + queue: UpdateQueue, + action: A, +) { + invariant( + numberOfReRenders < RE_RENDER_LIMIT, + 'Too many re-renders. React limits the number of renders to prevent ' + + 'an infinite loop.', + ); + + if (componentIdentity === currentlyRenderingComponent) { + // This is a render phase update. Stash it in a lazily-created map of + // queue -> linked list of updates. After this render pass, we'll restart + // and apply the stashed updates on top of the work-in-progress hook. + didScheduleRenderPhaseUpdate = true; + const update: Update = { + action, + next: null, + }; + if (renderPhaseUpdates === null) { + renderPhaseUpdates = new Map(); + } + const firstRenderPhaseUpdate = renderPhaseUpdates.get(queue); + if (firstRenderPhaseUpdate === undefined) { + renderPhaseUpdates.set(queue, update); + } else { + // Append the update to the end of the list. + let lastRenderPhaseUpdate = firstRenderPhaseUpdate; + while (lastRenderPhaseUpdate.next !== null) { + lastRenderPhaseUpdate = lastRenderPhaseUpdate.next; + } + lastRenderPhaseUpdate.next = update; + } + } else { + // This means an update has happened after the functional component has + // returned. On the server this is a no-op. In React Fiber, the update + // would be scheduled for a future render. + } +} + +function inputsAreEqual(arr1, arr2) { + // Don't bother comparing lengths because these arrays are always + // passed inline. + for (let i = 0; i < arr1.length; i++) { + // Inlined Object.is polyfill. + // https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Global_Objects/Object/is + const val1 = arr1[i]; + const val2 = arr2[i]; + if ( + (val1 === val2 && (val1 !== 0 || 1 / val1 === 1 / (val2: any))) || + (val1 !== val1 && val2 !== val2) // eslint-disable-line no-self-compare + ) { + continue; + } + return false; + } + return true; +} + +function noop(): void {} + +export const Dispatcher = { + readContext: useContext, + useContext, + useMemo, + useReducer, + useRef, + useState, + useMutationEffect, + useLayoutEffect, + // useAPI is not run in the server environment + useAPI: noop, + // Callbacks are not run in the server environment. + useCallback: noop, + // Effects are not run in the server environment. + useEffect: noop, +}; diff --git a/packages/react-reconciler/src/ReactFiberHooks.js b/packages/react-reconciler/src/ReactFiberHooks.js index 8d61bcf1270b6..439dffde4872c 100644 --- a/packages/react-reconciler/src/ReactFiberHooks.js +++ b/packages/react-reconciler/src/ReactFiberHooks.js @@ -347,47 +347,48 @@ export function useReducer( ): [S, Dispatch] { currentlyRenderingFiber = resolveCurrentlyRenderingFiber(); workInProgressHook = createWorkInProgressHook(); - if (isReRender) { - // This is a re-render. Apply the new render phase updates to the previous - // work-in-progress hook. - const queue: UpdateQueue = (workInProgressHook.queue: any); - const dispatch: Dispatch = (queue.dispatch: any); - if (renderPhaseUpdates !== null) { - // Render phase updates are stored in a map of queue -> linked list - const firstRenderPhaseUpdate = renderPhaseUpdates.get(queue); - if (firstRenderPhaseUpdate !== undefined) { - renderPhaseUpdates.delete(queue); - let newState = workInProgressHook.memoizedState; - let update = firstRenderPhaseUpdate; - do { - // Process this render phase update. We don't have to check the - // priority because it will always be the same as the current - // render's. - const action = update.action; - newState = reducer(newState, action); - const callback = update.callback; - if (callback !== null) { - pushCallback(currentlyRenderingFiber, update); + let queue: UpdateQueue | null = (workInProgressHook.queue: any); + if (queue !== null) { + // Already have a queue, so this is an update. + if (isReRender) { + // This is a re-render. Apply the new render phase updates to the previous + // work-in-progress hook. + const dispatch: Dispatch = (queue.dispatch: any); + if (renderPhaseUpdates !== null) { + // Render phase updates are stored in a map of queue -> linked list + const firstRenderPhaseUpdate = renderPhaseUpdates.get(queue); + if (firstRenderPhaseUpdate !== undefined) { + renderPhaseUpdates.delete(queue); + let newState = workInProgressHook.memoizedState; + let update = firstRenderPhaseUpdate; + do { + // Process this render phase update. We don't have to check the + // priority because it will always be the same as the current + // render's. + const action = update.action; + newState = reducer(newState, action); + const callback = update.callback; + if (callback !== null) { + pushCallback(currentlyRenderingFiber, update); + } + update = update.next; + } while (update !== null); + + workInProgressHook.memoizedState = newState; + + // Don't persist the state accumlated from the render phase updates to + // the base state unless the queue is empty. + // TODO: Not sure if this is the desired semantics, but it's what we + // do for gDSFP. I can't remember why. + if (workInProgressHook.baseUpdate === queue.last) { + workInProgressHook.baseState = newState; } - update = update.next; - } while (update !== null); - - workInProgressHook.memoizedState = newState; - // Don't persist the state accumlated from the render phase updates to - // the base state unless the queue is empty. - // TODO: Not sure if this is the desired semantics, but it's what we - // do for gDSFP. I can't remember why. - if (workInProgressHook.baseUpdate === queue.last) { - workInProgressHook.baseState = newState; + return [newState, dispatch]; } - - return [newState, dispatch]; } + return [workInProgressHook.memoizedState, dispatch]; } - return [workInProgressHook.memoizedState, dispatch]; - } else if (currentHook !== null) { - const queue: UpdateQueue = (workInProgressHook.queue: any); // The last update in the entire queue const last = queue.last; @@ -457,27 +458,28 @@ export function useReducer( const dispatch: Dispatch = (queue.dispatch: any); return [workInProgressHook.memoizedState, dispatch]; - } else { - if (reducer === basicStateReducer) { - // Special case for `useState`. - if (typeof initialState === 'function') { - initialState = initialState(); - } - } else if (initialAction !== undefined && initialAction !== null) { - initialState = reducer(initialState, initialAction); + } + + // There's no existing queue, so this is the initial render. + if (reducer === basicStateReducer) { + // Special case for `useState`. + if (typeof initialState === 'function') { + initialState = initialState(); } - workInProgressHook.memoizedState = workInProgressHook.baseState = initialState; - const queue: UpdateQueue = (workInProgressHook.queue = { - last: null, - dispatch: null, - }); - const dispatch: Dispatch = (queue.dispatch = (dispatchAction.bind( - null, - currentlyRenderingFiber, - queue, - ): any)); - return [workInProgressHook.memoizedState, dispatch]; + } else if (initialAction !== undefined && initialAction !== null) { + initialState = reducer(initialState, initialAction); } + workInProgressHook.memoizedState = workInProgressHook.baseState = initialState; + queue = workInProgressHook.queue = { + last: null, + dispatch: null, + }; + const dispatch: Dispatch = (queue.dispatch = (dispatchAction.bind( + null, + currentlyRenderingFiber, + queue, + ): any)); + return [workInProgressHook.memoizedState, dispatch]; } function pushCallback(workInProgress: Fiber, update: Update): void { @@ -525,7 +527,8 @@ export function useRef(initialValue: T): {current: T} { currentlyRenderingFiber = resolveCurrentlyRenderingFiber(); workInProgressHook = createWorkInProgressHook(); let ref; - if (currentHook === null) { + + if (workInProgressHook.memoizedState === null) { ref = {current: initialValue}; if (__DEV__) { Object.seal(ref); @@ -637,14 +640,13 @@ export function useCallback( const nextInputs = inputs !== undefined && inputs !== null ? inputs : [callback]; - if (currentHook !== null) { - const prevState = currentHook.memoizedState; + const prevState = workInProgressHook.memoizedState; + if (prevState !== null) { const prevInputs = prevState[1]; if (inputsAreEqual(nextInputs, prevInputs)) { return prevState[0]; } } - workInProgressHook.memoizedState = [callback, nextInputs]; return callback; } @@ -659,8 +661,8 @@ export function useMemo( const nextInputs = inputs !== undefined && inputs !== null ? inputs : [nextCreate]; - if (currentHook !== null) { - const prevState = currentHook.memoizedState; + const prevState = workInProgressHook.memoizedState; + if (prevState !== null) { const prevInputs = prevState[1]; if (inputsAreEqual(nextInputs, prevInputs)) { return prevState[0]; diff --git a/packages/react-reconciler/src/__tests__/ReactHooks-test.internal.js b/packages/react-reconciler/src/__tests__/ReactHooks-test.internal.js index f73b77abe2e7c..28577022d1656 100644 --- a/packages/react-reconciler/src/__tests__/ReactHooks-test.internal.js +++ b/packages/react-reconciler/src/__tests__/ReactHooks-test.internal.js @@ -1417,6 +1417,33 @@ describe('ReactHooks', () => { ReactNoop.render(); expect(ReactNoop.flush()).toEqual(['compute B', 'B']); }); + + it('should not invoke memoized function during re-renders unless inputs change', () => { + function LazyCompute(props) { + const computed = useMemo(() => props.compute(props.input), [ + props.input, + ]); + const [count, setCount] = useState(0); + if (count < 3) { + setCount(count + 1); + } + return ; + } + + function compute(val) { + ReactNoop.yield('compute ' + val); + return val; + } + + ReactNoop.render(); + expect(ReactNoop.flush()).toEqual(['compute A', 'A']); + + ReactNoop.render(); + expect(ReactNoop.flush()).toEqual(['A']); + + ReactNoop.render(); + expect(ReactNoop.flush()).toEqual(['compute B', 'B']); + }); }); describe('useRef', () => { @@ -1476,6 +1503,30 @@ describe('ReactHooks', () => { jest.advanceTimersByTime(20); expect(ReactNoop.flush()).toEqual(['ping: 6']); }); + + it('should return the same ref during re-renders', () => { + function Counter() { + const ref = useRef('val'); + const [count, setCount] = useState(0); + const [firstRef] = useState(ref); + + if (firstRef !== ref) { + throw new Error('should never change'); + } + + if (count < 3) { + setCount(count + 1); + } + + return ; + } + + ReactNoop.render(); + expect(ReactNoop.flush()).toEqual(['val']); + + ReactNoop.render(); + expect(ReactNoop.flush()).toEqual(['val']); + }); }); describe('progressive enhancement', () => { From 6514697f0cb294b48550eeb3bcbddfd11143539c Mon Sep 17 00:00:00 2001 From: Sophie Alpert Date: Wed, 3 Oct 2018 18:31:35 -0700 Subject: [PATCH 05/19] Make sure deletions don't stop passive effects Before the fix, the passive effect in the test is never executed. We were previously waiting until the next commit phase to run effects. Now, we run them before scheduling work. --- .../src/ReactFiberClassComponent.js | 4 + .../src/ReactFiberCommitWork.js | 2 + .../react-reconciler/src/ReactFiberHooks.js | 2 + .../src/ReactFiberReconciler.js | 5 +- .../react-reconciler/src/ReactFiberRoot.js | 7 -- .../src/ReactFiberScheduler.js | 96 ++++++++++++------- .../src/__tests__/ReactHooks-test.internal.js | 78 +++++++++++++-- 7 files changed, 140 insertions(+), 54 deletions(-) diff --git a/packages/react-reconciler/src/ReactFiberClassComponent.js b/packages/react-reconciler/src/ReactFiberClassComponent.js index 57114af3e9f58..b2b50a6a52567 100644 --- a/packages/react-reconciler/src/ReactFiberClassComponent.js +++ b/packages/react-reconciler/src/ReactFiberClassComponent.js @@ -51,6 +51,7 @@ import { requestCurrentTime, computeExpirationForFiber, scheduleWork, + flushPassiveEffectsBeforeSchedulingUpdateOnFiber, } from './ReactFiberScheduler'; const ReactCurrentOwner = ReactSharedInternals.ReactCurrentOwner; @@ -199,6 +200,7 @@ const classComponentUpdater = { update.callback = callback; } + flushPassiveEffectsBeforeSchedulingUpdateOnFiber(fiber); enqueueUpdate(fiber, update); scheduleWork(fiber, expirationTime); }, @@ -218,6 +220,7 @@ const classComponentUpdater = { update.callback = callback; } + flushPassiveEffectsBeforeSchedulingUpdateOnFiber(fiber); enqueueUpdate(fiber, update); scheduleWork(fiber, expirationTime); }, @@ -236,6 +239,7 @@ const classComponentUpdater = { update.callback = callback; } + flushPassiveEffectsBeforeSchedulingUpdateOnFiber(fiber); enqueueUpdate(fiber, update); scheduleWork(fiber, expirationTime); }, diff --git a/packages/react-reconciler/src/ReactFiberCommitWork.js b/packages/react-reconciler/src/ReactFiberCommitWork.js index bad7139e5bbe8..d851bb77c221b 100644 --- a/packages/react-reconciler/src/ReactFiberCommitWork.js +++ b/packages/react-reconciler/src/ReactFiberCommitWork.js @@ -83,6 +83,7 @@ import { } from './ReactFiberHostConfig'; import { captureCommitPhaseError, + flushPassiveEffectsBeforeSchedulingUpdateOnFiber, requestCurrentTime, scheduleWork, } from './ReactFiberScheduler'; @@ -452,6 +453,7 @@ function commitLifeCycles( timedOutAt: NoWork, }; finishedWork.memoizedState = newState; + flushPassiveEffectsBeforeSchedulingUpdateOnFiber(finishedWork); scheduleWork(finishedWork, Sync); return; } diff --git a/packages/react-reconciler/src/ReactFiberHooks.js b/packages/react-reconciler/src/ReactFiberHooks.js index 439dffde4872c..7a2671c80c891 100644 --- a/packages/react-reconciler/src/ReactFiberHooks.js +++ b/packages/react-reconciler/src/ReactFiberHooks.js @@ -32,6 +32,7 @@ import { import { scheduleWork, computeExpirationForFiber, + flushPassiveEffectsBeforeSchedulingUpdateOnFiber, requestCurrentTime, } from './ReactFiberScheduler'; @@ -724,6 +725,7 @@ function dispatchAction( callback: callback !== undefined ? callback : null, next: null, }; + flushPassiveEffectsBeforeSchedulingUpdateOnFiber(fiber); // Append the update to the end of the list. const last = queue.last; if (last === null) { diff --git a/packages/react-reconciler/src/ReactFiberReconciler.js b/packages/react-reconciler/src/ReactFiberReconciler.js index d594267a1094e..b06922c94f80b 100644 --- a/packages/react-reconciler/src/ReactFiberReconciler.js +++ b/packages/react-reconciler/src/ReactFiberReconciler.js @@ -52,6 +52,7 @@ import { syncUpdates, interactiveUpdates, flushInteractiveUpdates, + flushPassiveEffectsBeforeSchedulingUpdateOnFiber, } from './ReactFiberScheduler'; import {createUpdate, enqueueUpdate} from './ReactUpdateQueue'; import ReactFiberInstrumentation from './ReactFiberInstrumentation'; @@ -145,9 +146,11 @@ function scheduleRootUpdate( ); update.callback = callback; } - enqueueUpdate(current, update); + flushPassiveEffectsBeforeSchedulingUpdateOnFiber(current); + enqueueUpdate(current, update); scheduleWork(current, expirationTime); + return expirationTime; } diff --git a/packages/react-reconciler/src/ReactFiberRoot.js b/packages/react-reconciler/src/ReactFiberRoot.js index 307f7c82330b8..d257f3c93ab6f 100644 --- a/packages/react-reconciler/src/ReactFiberRoot.js +++ b/packages/react-reconciler/src/ReactFiberRoot.js @@ -36,9 +36,6 @@ type BaseFiberRootProperties = {| // The currently active root fiber. This is the mutable root of the tree. current: Fiber, - serialEffectCallback: (() => mixed) | null, - serialEffectCallbackHandle: any, - // The following priority levels are used to distinguish between 1) // uncommitted work, 2) uncommitted work that is suspended, and 3) uncommitted // work that may be unsuspended. We choose not to track each individual @@ -117,8 +114,6 @@ export function createFiberRoot( current: uninitializedFiber, containerInfo: containerInfo, pendingChildren: null, - serialEffectCallback: null, - serialEffectCallbackHandle: null, earliestPendingTime: NoWork, latestPendingTime: NoWork, @@ -148,8 +143,6 @@ export function createFiberRoot( current: uninitializedFiber, containerInfo: containerInfo, pendingChildren: null, - serialEffectCallback: null, - serialEffectCallbackHandle: null, earliestPendingTime: NoWork, latestPendingTime: NoWork, diff --git a/packages/react-reconciler/src/ReactFiberScheduler.js b/packages/react-reconciler/src/ReactFiberScheduler.js index f7437c37269c5..a338c1d39d889 100644 --- a/packages/react-reconciler/src/ReactFiberScheduler.js +++ b/packages/react-reconciler/src/ReactFiberScheduler.js @@ -264,7 +264,9 @@ let nextRenderDidError: boolean = false; let nextEffect: Fiber | null = null; let isCommitting: boolean = false; -let needsPassiveCommit: boolean = false; +let rootWithPendingPassiveEffects: FiberRoot | null = null; +let firstPassiveEffect: Fiber | null = null; +let passiveEffectCallbackHandle: * = null; let legacyErrorBoundariesThatAlreadyFailed: Set | null = null; @@ -504,7 +506,7 @@ function commitAllLifeCycles( } if (effectTag & Passive) { - needsPassiveCommit = true; + rootWithPendingPassiveEffects = finishedRoot; } nextEffect = nextEffect.nextEffect; @@ -512,6 +514,9 @@ function commitAllLifeCycles( } function commitPassiveEffects(root: FiberRoot, firstEffect: Fiber): void { + rootWithPendingPassiveEffects = null; + passiveEffectCallbackHandle = null; + // Set this to true to prevent re-entrancy const previousIsRendering = isRendering; isRendering = true; @@ -566,21 +571,42 @@ function markLegacyErrorBoundaryAsFailed(instance: mixed) { } } -function commitRoot(root: FiberRoot, finishedWork: Fiber): void { - const existingSerialEffectCallback = root.serialEffectCallback; - const existingSerialEffectCallbackHandle = root.serialEffectCallbackHandle; - if (existingSerialEffectCallback !== null) { - // A passive callback was scheduled during the previous commit, but it did - // not get a chance to flush. Flush it now to ensure serial execution. - // This should fire before any new mutations. - root.serialEffectCallback = null; - if (existingSerialEffectCallbackHandle !== null) { - root.serialEffectCallbackHandle = null; - Schedule_cancelCallback(existingSerialEffectCallbackHandle); +function flushPassiveEffectsBeforeSchedulingUpdateOnFiber(fiber: Fiber) { + if (rootWithPendingPassiveEffects !== null) { + // TODO: This is an unfortunate extra loop. We end up traversing to the root + // again in scheduleWorkToRoot. But we have to do this one first because it + // needs to happen before adding an update to the queue, and + // scheduleWorkToRoot may perform a synchronous re-render. Maybe we can + // solve this with batchedUpdates, or with the equivalent in the Scheduler + // package. + let node = fiber; + do { + switch (node.tag) { + case HostRoot: { + const root: FiberRoot = node.stateNode; + flushPassiveEffects(root); + return; + } + } + node = node.return; + } while (node !== null); + } +} + +function flushPassiveEffects(root: FiberRoot) { + if (root === rootWithPendingPassiveEffects) { + rootWithPendingPassiveEffects = null; + if (passiveEffectCallbackHandle !== null) { + Schedule_cancelCallback(passiveEffectCallbackHandle); + passiveEffectCallbackHandle = null; + } + if (firstPassiveEffect !== null) { + commitPassiveEffects(root, firstPassiveEffect); } - existingSerialEffectCallback(); } +} +function commitRoot(root: FiberRoot, finishedWork: Fiber): void { isWorking = true; isCommitting = true; startCommitTimer(); @@ -770,31 +796,30 @@ function commitRoot(root: FiberRoot, finishedWork: Fiber): void { } } - if (firstEffect !== null && needsPassiveCommit) { - const resolvedFirstEffect = firstEffect; + if (firstEffect !== null && rootWithPendingPassiveEffects !== null) { // This commit included a passive effect. These do not need to fire until // after the next paint. Schedule an callback to fire them in an async // event. To ensure serial execution, the callback will be flushed early if - // we enter another commit phase before then. - needsPassiveCommit = false; - let serialEffectCallback; + // we enter rootWithPendingPassiveEffects commit phase before then. + const resolvedFirstEffect = (firstPassiveEffect = firstEffect); + + let passiveEffectCallback; if (enableSchedulerTracing) { // TODO: Avoid this extra callback by mutating the tracing ref directly, // like we do at the beginning of commitRoot. I've opted not to do that // here because that code is still in flux. - serialEffectCallback = Schedule_tracing_wrap(() => { - root.serialEffectCallback = null; + passiveEffectCallback = Schedule_tracing_wrap(() => { commitPassiveEffects(root, resolvedFirstEffect); }); } else { - serialEffectCallback = () => { - root.serialEffectCallback = null; - commitPassiveEffects(root, resolvedFirstEffect); - }; + passiveEffectCallback = commitPassiveEffects.bind( + null, + root, + resolvedFirstEffect, + ); } - root.serialEffectCallback = serialEffectCallback; - root.serialEffectCallbackHandle = Schedule_scheduleCallback( - serialEffectCallback, + passiveEffectCallbackHandle = Schedule_scheduleCallback( + passiveEffectCallback, ); } @@ -1227,6 +1252,9 @@ function renderRoot( 'renderRoot was called recursively. This error is likely caused ' + 'by a bug in React. Please file an issue.', ); + + flushPassiveEffects(root); + isWorking = true; ReactCurrentOwner.currentDispatcher = Dispatcher; @@ -1505,11 +1533,8 @@ function renderRoot( onComplete(root, rootWorkInProgress, expirationTime); } -function dispatch( - sourceFiber: Fiber, - value: mixed, - expirationTime: ExpirationTime, -) { +function captureCommitPhaseError(sourceFiber: Fiber, value: mixed) { + const expirationTime = Sync; let fiber = sourceFiber.return; while (fiber !== null) { switch (fiber.tag) { @@ -1554,10 +1579,6 @@ function dispatch( } } -function captureCommitPhaseError(fiber: Fiber, error: mixed) { - return dispatch(fiber, error, Sync); -} - function computeThreadID( expirationTime: ExpirationTime, interactionThreadID: number, @@ -2587,4 +2608,5 @@ export { interactiveUpdates, flushInteractiveUpdates, computeUniqueAsyncExpiration, + flushPassiveEffectsBeforeSchedulingUpdateOnFiber, }; diff --git a/packages/react-reconciler/src/__tests__/ReactHooks-test.internal.js b/packages/react-reconciler/src/__tests__/ReactHooks-test.internal.js index 28577022d1656..ec57236930aee 100644 --- a/packages/react-reconciler/src/__tests__/ReactHooks-test.internal.js +++ b/packages/react-reconciler/src/__tests__/ReactHooks-test.internal.js @@ -604,6 +604,38 @@ describe('ReactHooks', () => { expect(ReactNoop.clearYields()).toEqual(['Did commit [1]']); }); + it('flushes passive effects even with sibling deletions', () => { + function LayoutEffect(props) { + useLayoutEffect(() => { + ReactNoop.yield(`Layout effect`); + }); + return ; + } + function PassiveEffect(props) { + useEffect(() => { + ReactNoop.yield(`Passive effect`); + }, []); + return ; + } + let passive = ; + ReactNoop.render([, passive]); + expect(ReactNoop.flush()).toEqual(['Layout', 'Passive', 'Layout effect']); + expect(ReactNoop.getChildren()).toEqual([ + span('Layout'), + span('Passive'), + ]); + + // Destroying the first child shouldn't prevent the passive effect from + // being executed + ReactNoop.render([passive]); + expect(ReactNoop.flush()).toEqual(['Passive effect']); + expect(ReactNoop.getChildren()).toEqual([span('Passive')]); + + // (No effects are left to flush.) + flushPassiveEffects(); + expect(ReactNoop.clearYields()).toEqual(null); + }); + it( 'flushes effects serially by flushing old effects before flushing ' + "new ones, if they haven't already fired", @@ -631,9 +663,9 @@ describe('ReactHooks', () => { // Before the effects have a chance to flush, schedule another update ReactNoop.render(); expect(ReactNoop.flush()).toEqual([ - 1, - // The previous effect flushes before the host mutations + // The previous effect flushes before the reconciliation 'Committed state when effect was fired: 0', + 1, ]); expect(ReactNoop.getChildren()).toEqual([span(1)]); @@ -689,17 +721,39 @@ describe('ReactHooks', () => { // Rendering again should flush the previous commit's effects ReactNoop.render(); - ReactNoop.flushThrough([ - 'Count: (empty)', - 'Schedule update [0]', - 'Count: 0', - ]); + ReactNoop.flushThrough(['Schedule update [0]', 'Count: 0']); expect(ReactNoop.getChildren()).toEqual([span('Count: (empty)')]); + expect(ReactNoop.flush()).toEqual([]); + expect(ReactNoop.getChildren()).toEqual([span('Count: 0')]); + + flushPassiveEffects(); expect(ReactNoop.flush()).toEqual(['Schedule update [1]', 'Count: 1']); expect(ReactNoop.getChildren()).toEqual([span('Count: 1')]); }); + it('flushes serial effects before enqueueing work', () => { + let _updateCount; + function Counter(props) { + const [count, updateCount] = useState(0); + _updateCount = updateCount; + useEffect(() => { + ReactNoop.yield(`Will set count to 1`); + updateCount(1); + }, []); + return ; + } + ReactNoop.render(); + expect(ReactNoop.flush()).toEqual(['Count: 0']); + expect(ReactNoop.getChildren()).toEqual([span('Count: 0')]); + + // Enqueuing this update forces the passive effect to be flushed -- + // updateCount(1) happens first, so 2 wins. + _updateCount(2); + expect(ReactNoop.flush()).toEqual(['Will set count to 1', 'Count: 2']); + expect(ReactNoop.getChildren()).toEqual([span('Count: 2')]); + }); + it( 'in sync mode, useEffect is deferred and updates finish synchronously ' + '(in a single batch)', @@ -1160,7 +1214,10 @@ describe('ReactHooks', () => { expect(committedText).toEqual('1'); flushPassiveEffects(); - expect(ReactNoop.clearYields()).toEqual(['Mount normal [current: 1]']); + expect(ReactNoop.clearYields()).toEqual([ + 'Unmount normal [current: 1]', + 'Mount normal [current: 1]', + ]); }); it('force flushes passive effects before firing new layout effects', () => { @@ -1199,7 +1256,10 @@ describe('ReactHooks', () => { expect(committedText).toEqual('1'); flushPassiveEffects(); - expect(ReactNoop.clearYields()).toEqual(['Mount normal [current: 1]']); + expect(ReactNoop.clearYields()).toEqual([ + 'Unmount normal [current: 1]', + 'Mount normal [current: 1]', + ]); }); it('fires all mutation effects before firing any layout effects', () => { From 9e9e3970e424e3a6d36b29d22cb7326d6c5705f8 Mon Sep 17 00:00:00 2001 From: Sophie Alpert Date: Fri, 5 Oct 2018 15:32:15 -0700 Subject: [PATCH 06/19] Warn for Hook set-state on unmounted component --- .../__tests__/ReactCompositeComponent-test.js | 4 +-- .../src/ReactFiberScheduler.js | 30 ++++++++++++++----- .../src/__tests__/ReactHooks-test.internal.js | 21 +++++++++++++ 3 files changed, 45 insertions(+), 10 deletions(-) diff --git a/packages/react-dom/src/__tests__/ReactCompositeComponent-test.js b/packages/react-dom/src/__tests__/ReactCompositeComponent-test.js index 115544170a83e..43ac793cf59cd 100644 --- a/packages/react-dom/src/__tests__/ReactCompositeComponent-test.js +++ b/packages/react-dom/src/__tests__/ReactCompositeComponent-test.js @@ -332,7 +332,7 @@ describe('ReactCompositeComponent', () => { ReactDOM.unmountComponentAtNode(container); expect(() => instance.forceUpdate()).toWarnDev( - "Warning: Can't call setState (or forceUpdate) on an unmounted " + + "Warning: Can't perform a React state update on an unmounted " + 'component. This is a no-op, but it indicates a memory leak in your ' + 'application. To fix, cancel all subscriptions and asynchronous ' + 'tasks in the componentWillUnmount method.\n' + @@ -379,7 +379,7 @@ describe('ReactCompositeComponent', () => { expect(() => { instance.setState({value: 2}); }).toWarnDev( - "Warning: Can't call setState (or forceUpdate) on an unmounted " + + "Warning: Can't perform a React state update on an unmounted " + 'component. This is a no-op, but it indicates a memory leak in your ' + 'application. To fix, cancel all subscriptions and asynchronous ' + 'tasks in the componentWillUnmount method.\n' + diff --git a/packages/react-reconciler/src/ReactFiberScheduler.js b/packages/react-reconciler/src/ReactFiberScheduler.js index a338c1d39d889..47fda4d9b5a0a 100644 --- a/packages/react-reconciler/src/ReactFiberScheduler.js +++ b/packages/react-reconciler/src/ReactFiberScheduler.js @@ -45,11 +45,14 @@ import { Passive, } from 'shared/ReactSideEffectTags'; import { - HostRoot, ClassComponent, HostComponent, ContextProvider, + ForwardRef, + FunctionComponent, HostPortal, + HostRoot, + PureComponent, } from 'shared/ReactWorkTags'; import { enableSchedulerTracing, @@ -197,19 +200,21 @@ if (__DEV__) { didWarnSetStateChildContext = false; const didWarnStateUpdateForUnmountedComponent = {}; - warnAboutUpdateOnUnmounted = function(fiber: Fiber) { + warnAboutUpdateOnUnmounted = function(fiber: Fiber, isClass: boolean) { // We show the whole stack but dedupe on the top component's name because // the problematic code almost always lies inside that component. - const componentName = getComponentName(fiber.type) || 'ReactClass'; + const componentName = getComponentName(fiber.type) || 'ReactComponent'; if (didWarnStateUpdateForUnmountedComponent[componentName]) { return; } warningWithoutStack( false, - "Can't call setState (or forceUpdate) on an unmounted component. This " + + "Can't perform a React state update on an unmounted component. This " + 'is a no-op, but it indicates a memory leak in your application. To ' + - 'fix, cancel all subscriptions and asynchronous tasks in the ' + - 'componentWillUnmount method.%s', + 'fix, cancel all subscriptions and asynchronous tasks in %s.%s', + isClass + ? 'the componentWillUnmount method' + : 'a useEffect cleanup function', ReactCurrentFiber.getStackByFiberInDevAndProd(fiber), ); didWarnStateUpdateForUnmountedComponent[componentName] = true; @@ -1785,8 +1790,17 @@ function scheduleWorkToRoot(fiber: Fiber, expirationTime): FiberRoot | null { } if (root === null) { - if (__DEV__ && fiber.tag === ClassComponent) { - warnAboutUpdateOnUnmounted(fiber); + if (__DEV__) { + switch (fiber.tag) { + case ClassComponent: + warnAboutUpdateOnUnmounted(fiber, true); + break; + case FunctionComponent: + case ForwardRef: + case PureComponent: + warnAboutUpdateOnUnmounted(fiber, false); + break; + } } return null; } diff --git a/packages/react-reconciler/src/__tests__/ReactHooks-test.internal.js b/packages/react-reconciler/src/__tests__/ReactHooks-test.internal.js index ec57236930aee..ec6eaf59dd92f 100644 --- a/packages/react-reconciler/src/__tests__/ReactHooks-test.internal.js +++ b/packages/react-reconciler/src/__tests__/ReactHooks-test.internal.js @@ -309,6 +309,27 @@ describe('ReactHooks', () => { expect(updaters).toEqual([updaters[0], updaters[0], updaters[0]]); }); + + it('warns on set after unmount', () => { + let _updateCount; + function Counter(props, ref) { + const [, updateCount] = useState(0); + _updateCount = updateCount; + return null; + } + + ReactNoop.render(); + ReactNoop.flush(); + ReactNoop.render(null); + ReactNoop.flush(); + expect(() => _updateCount(1)).toWarnDev( + "Warning: Can't perform a React state update on an unmounted " + + 'component. This is a no-op, but it indicates a memory leak in your ' + + 'application. To fix, cancel all subscriptions and asynchronous ' + + 'tasks in a useEffect cleanup function.\n' + + ' in Counter (at **)', + ); + }); }); describe('updates during the render phase', () => { From b772e0e26bd8f3062e23e9d831045a39072d0372 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Sebastian=20Markb=C3=A5ge?= Date: Tue, 9 Oct 2018 02:20:48 +0100 Subject: [PATCH 07/19] "functional component" -> "function component" in hooks error messages --- .../ReactDOMServerIntegrationHooks-test.internal.js | 2 +- packages/react-dom/src/server/ReactPartialRendererHooks.js | 6 +++--- packages/react-reconciler/src/ReactFiberHooks.js | 6 +++--- .../src/__tests__/ReactHooks-test.internal.js | 6 +++--- .../src/__tests__/ReactNewContext-test.internal.js | 4 ++-- packages/react/src/ReactHooks.js | 2 +- 6 files changed, 13 insertions(+), 13 deletions(-) diff --git a/packages/react-dom/src/__tests__/ReactDOMServerIntegrationHooks-test.internal.js b/packages/react-dom/src/__tests__/ReactDOMServerIntegrationHooks-test.internal.js index 99d6ccbb783d9..8c991c9ebe8c7 100644 --- a/packages/react-dom/src/__tests__/ReactDOMServerIntegrationHooks-test.internal.js +++ b/packages/react-dom/src/__tests__/ReactDOMServerIntegrationHooks-test.internal.js @@ -145,7 +145,7 @@ describe('ReactDOMServerHooks', () => { return render(); }, - 'Hooks can only be called inside the body of a functional component.', + 'Hooks can only be called inside the body of a function component.', ); itRenders('multiple times when an updater is called', async render => { diff --git a/packages/react-dom/src/server/ReactPartialRendererHooks.js b/packages/react-dom/src/server/ReactPartialRendererHooks.js index bc2d34d6d0922..bec21679c1132 100644 --- a/packages/react-dom/src/server/ReactPartialRendererHooks.js +++ b/packages/react-dom/src/server/ReactPartialRendererHooks.js @@ -50,7 +50,7 @@ const RE_RENDER_LIMIT = 25; function resolveCurrentlyRenderingComponent(): Object { invariant( currentlyRenderingComponent !== null, - 'Hooks can only be called inside the body of a functional component.', + 'Hooks can only be called inside the body of a function component.', ); return currentlyRenderingComponent; } @@ -105,7 +105,7 @@ export function finishHooks( children: any, refOrContext: any, ): any { - // This must be called after every functional component to prevent hooks from + // This must be called after every function component to prevent hooks from // being used in classes. while (didScheduleRenderPhaseUpdate) { @@ -321,7 +321,7 @@ function dispatchAction( lastRenderPhaseUpdate.next = update; } } else { - // This means an update has happened after the functional component has + // This means an update has happened after the function component has // returned. On the server this is a no-op. In React Fiber, the update // would be scheduled for a future render. } diff --git a/packages/react-reconciler/src/ReactFiberHooks.js b/packages/react-reconciler/src/ReactFiberHooks.js index 7a2671c80c891..5e843c5e53287 100644 --- a/packages/react-reconciler/src/ReactFiberHooks.js +++ b/packages/react-reconciler/src/ReactFiberHooks.js @@ -121,7 +121,7 @@ const RE_RENDER_LIMIT = 25; function resolveCurrentlyRenderingFiber(): Fiber { invariant( currentlyRenderingFiber !== null, - 'Hooks can only be called inside the body of a functional component.', + 'Hooks can only be called inside the body of a function component.', ); return currentlyRenderingFiber; } @@ -154,7 +154,7 @@ export function finishHooks( children: any, refOrContext: any, ): any { - // This must be called after every functional component to prevent hooks from + // This must be called after every function component to prevent hooks from // being used in classes. while (didScheduleRenderPhaseUpdate) { @@ -325,7 +325,7 @@ export function useContext( context: ReactContext, observedBits: void | number | boolean, ): T { - // Ensure we're in a functional component (class components support only the + // Ensure we're in a function component (class components support only the // .unstable_read() form) resolveCurrentlyRenderingFiber(); return readContext(context, observedBits); diff --git a/packages/react-reconciler/src/__tests__/ReactHooks-test.internal.js b/packages/react-reconciler/src/__tests__/ReactHooks-test.internal.js index ec6eaf59dd92f..6e7480215827c 100644 --- a/packages/react-reconciler/src/__tests__/ReactHooks-test.internal.js +++ b/packages/react-reconciler/src/__tests__/ReactHooks-test.internal.js @@ -121,7 +121,7 @@ describe('ReactHooks', () => { ReactNoop.render(); expect(() => ReactNoop.flush()).toThrow( - 'Hooks can only be called inside the body of a functional component.', + 'Hooks can only be called inside the body of a function component.', ); // Confirm that a subsequent hook works properly. @@ -144,7 +144,7 @@ describe('ReactHooks', () => { } ReactNoop.render(); expect(() => ReactNoop.flush()).toThrow( - 'Hooks can only be called inside the body of a functional component.', + 'Hooks can only be called inside the body of a function component.', ); // Confirm that a subsequent hook works properly. @@ -158,7 +158,7 @@ describe('ReactHooks', () => { it('throws when called outside the render phase', () => { expect(() => useState(0)).toThrow( - 'Hooks can only be called inside the body of a functional component.', + 'Hooks can only be called inside the body of a function component.', ); }); diff --git a/packages/react-reconciler/src/__tests__/ReactNewContext-test.internal.js b/packages/react-reconciler/src/__tests__/ReactNewContext-test.internal.js index 194539e6ec84a..25c3a3a1ad535 100644 --- a/packages/react-reconciler/src/__tests__/ReactNewContext-test.internal.js +++ b/packages/react-reconciler/src/__tests__/ReactNewContext-test.internal.js @@ -48,7 +48,7 @@ describe('ReactNewContext', () => { // a suite of tests for a given context consumer implementation. sharedContextTests('Context.Consumer', Context => Context.Consumer); sharedContextTests( - 'useContext inside functional component', + 'useContext inside function component', Context => function Consumer(props) { const observedBits = props.unstable_observedBits; @@ -1342,7 +1342,7 @@ describe('ReactNewContext', () => { } ReactNoop.render(); expect(ReactNoop.flush).toThrow( - 'Hooks can only be called inside the body of a functional component.', + 'Hooks can only be called inside the body of a function component.', ); }); }); diff --git a/packages/react/src/ReactHooks.js b/packages/react/src/ReactHooks.js index 1c9b7271ea905..e2ccc3a2a8cc7 100644 --- a/packages/react/src/ReactHooks.js +++ b/packages/react/src/ReactHooks.js @@ -16,7 +16,7 @@ function resolveDispatcher() { const dispatcher = ReactCurrentOwner.currentDispatcher; invariant( dispatcher !== null, - 'Hooks can only be called inside the body of a functional component.', + 'Hooks can only be called inside the body of a function component.', ); return dispatcher; } From 30aa4ad5541c6e82de465a5e6234e5bb1329dec1 Mon Sep 17 00:00:00 2001 From: Sophie Alpert Date: Wed, 10 Oct 2018 15:41:46 -0700 Subject: [PATCH 08/19] The Lost Effect, chapter 2 Previously, flushPassiveEffects (called by scheduling work) would overwrite rootWithPendingPassiveEffects before we had a chance to schedule the work. --- .../src/ReactFiberScheduler.js | 11 ++++--- .../src/__tests__/ReactHooks-test.internal.js | 33 +++++++++++++++++++ 2 files changed, 39 insertions(+), 5 deletions(-) diff --git a/packages/react-reconciler/src/ReactFiberScheduler.js b/packages/react-reconciler/src/ReactFiberScheduler.js index 47fda4d9b5a0a..81be6ff00102c 100644 --- a/packages/react-reconciler/src/ReactFiberScheduler.js +++ b/packages/react-reconciler/src/ReactFiberScheduler.js @@ -599,12 +599,13 @@ function flushPassiveEffectsBeforeSchedulingUpdateOnFiber(fiber: Fiber) { } function flushPassiveEffects(root: FiberRoot) { - if (root === rootWithPendingPassiveEffects) { + if ( + passiveEffectCallbackHandle !== null && + root === rootWithPendingPassiveEffects + ) { + Schedule_cancelCallback(passiveEffectCallbackHandle); + passiveEffectCallbackHandle = null; rootWithPendingPassiveEffects = null; - if (passiveEffectCallbackHandle !== null) { - Schedule_cancelCallback(passiveEffectCallbackHandle); - passiveEffectCallbackHandle = null; - } if (firstPassiveEffect !== null) { commitPassiveEffects(root, firstPassiveEffect); } diff --git a/packages/react-reconciler/src/__tests__/ReactHooks-test.internal.js b/packages/react-reconciler/src/__tests__/ReactHooks-test.internal.js index 6e7480215827c..5f6681babd9a0 100644 --- a/packages/react-reconciler/src/__tests__/ReactHooks-test.internal.js +++ b/packages/react-reconciler/src/__tests__/ReactHooks-test.internal.js @@ -657,6 +657,39 @@ describe('ReactHooks', () => { expect(ReactNoop.clearYields()).toEqual(null); }); + it('flushes passive effects even if siblings schedule an update', () => { + function PassiveEffect(props) { + useEffect(() => { + ReactNoop.yield('Passive effect'); + }); + return ; + } + function LayoutEffect(props) { + let [count, setCount] = useState(0); + useLayoutEffect(() => { + // Scheduling work shouldn't interfere with the queued passive effect + if (count === 0) { + setCount(1); + } + ReactNoop.yield('Layout effect ' + count); + }); + return ; + } + ReactNoop.render([, ]); + expect(ReactNoop.flush()).toEqual([ + 'Passive', + 'Layout', + 'Layout effect 0', + 'Passive effect', + 'Layout', + 'Layout effect 1', + ]); + expect(ReactNoop.getChildren()).toEqual([ + span('Passive'), + span('Layout'), + ]); + }); + it( 'flushes effects serially by flushing old effects before flushing ' + "new ones, if they haven't already fired", From 55a4b1f37799d1dd5b009ad22931be4131ca2c69 Mon Sep 17 00:00:00 2001 From: Sophie Alpert Date: Thu, 11 Oct 2018 13:54:50 -0700 Subject: [PATCH 09/19] memo supports Hooks --- .../src/ReactFiberCommitWork.js | 20 +++-- .../src/ReactFiberScheduler.js | 38 ++++----- .../src/__tests__/ReactHooks-test.internal.js | 78 +++++++++++++++++++ 3 files changed, 107 insertions(+), 29 deletions(-) diff --git a/packages/react-reconciler/src/ReactFiberCommitWork.js b/packages/react-reconciler/src/ReactFiberCommitWork.js index d851bb77c221b..3f61066dd9d62 100644 --- a/packages/react-reconciler/src/ReactFiberCommitWork.js +++ b/packages/react-reconciler/src/ReactFiberCommitWork.js @@ -36,6 +36,8 @@ import { Profiler, SuspenseComponent, IncompleteClassComponent, + MemoComponent, + SimpleMemoComponent, } from 'shared/ReactWorkTags'; import { invokeGuardedCallback, @@ -216,7 +218,8 @@ function commitBeforeMutationLifeCycles( ): void { switch (finishedWork.tag) { case FunctionComponent: - case ForwardRef: { + case ForwardRef: + case SimpleMemoComponent: { commitHookEffectList(UnmountSnapshot, NoHookEffect, finishedWork); return; } @@ -313,7 +316,8 @@ function commitLifeCycles( ): void { switch (finishedWork.tag) { case FunctionComponent: - case ForwardRef: { + case ForwardRef: + case SimpleMemoComponent: { commitHookEffectList(UnmountLayout, MountLayout, finishedWork); const newUpdateQueue: FunctionComponentUpdateQueue | null = (finishedWork.updateQueue: any); if (newUpdateQueue !== null) { @@ -588,7 +592,9 @@ function commitUnmount(current: Fiber): void { switch (current.tag) { case FunctionComponent: - case ForwardRef: { + case ForwardRef: + case MemoComponent: + case SimpleMemoComponent: { const updateQueue: FunctionComponentUpdateQueue | null = (current.updateQueue: any); if (updateQueue !== null) { const lastEffect = updateQueue.lastEffect; @@ -981,7 +987,9 @@ function commitWork(current: Fiber | null, finishedWork: Fiber): void { if (!supportsMutation) { switch (finishedWork.tag) { case FunctionComponent: - case ForwardRef: { + case ForwardRef: + case MemoComponent: + case SimpleMemoComponent: { commitHookEffectList(UnmountMutation, MountMutation, finishedWork); return; } @@ -993,7 +1001,9 @@ function commitWork(current: Fiber | null, finishedWork: Fiber): void { switch (finishedWork.tag) { case FunctionComponent: - case ForwardRef: { + case ForwardRef: + case MemoComponent: + case SimpleMemoComponent: { commitHookEffectList(UnmountMutation, MountMutation, finishedWork); return; } diff --git a/packages/react-reconciler/src/ReactFiberScheduler.js b/packages/react-reconciler/src/ReactFiberScheduler.js index 81be6ff00102c..21a919b5c5058 100644 --- a/packages/react-reconciler/src/ReactFiberScheduler.js +++ b/packages/react-reconciler/src/ReactFiberScheduler.js @@ -52,7 +52,8 @@ import { FunctionComponent, HostPortal, HostRoot, - PureComponent, + MemoComponent, + SimpleMemoComponent, } from 'shared/ReactWorkTags'; import { enableSchedulerTracing, @@ -270,8 +271,8 @@ let nextEffect: Fiber | null = null; let isCommitting: boolean = false; let rootWithPendingPassiveEffects: FiberRoot | null = null; -let firstPassiveEffect: Fiber | null = null; let passiveEffectCallbackHandle: * = null; +let passiveEffectCallback: * = null; let legacyErrorBoundariesThatAlreadyFailed: Set | null = null; @@ -521,6 +522,7 @@ function commitAllLifeCycles( function commitPassiveEffects(root: FiberRoot, firstEffect: Fiber): void { rootWithPendingPassiveEffects = null; passiveEffectCallbackHandle = null; + passiveEffectCallback = null; // Set this to true to prevent re-entrancy const previousIsRendering = isRendering; @@ -600,15 +602,13 @@ function flushPassiveEffectsBeforeSchedulingUpdateOnFiber(fiber: Fiber) { function flushPassiveEffects(root: FiberRoot) { if ( - passiveEffectCallbackHandle !== null && + passiveEffectCallback !== null && root === rootWithPendingPassiveEffects ) { Schedule_cancelCallback(passiveEffectCallbackHandle); - passiveEffectCallbackHandle = null; - rootWithPendingPassiveEffects = null; - if (firstPassiveEffect !== null) { - commitPassiveEffects(root, firstPassiveEffect); - } + // We call the scheduled callback instead of commitPassiveEffects directly + // to ensure tracing works correctly. + passiveEffectCallback(); } } @@ -807,26 +807,15 @@ function commitRoot(root: FiberRoot, finishedWork: Fiber): void { // after the next paint. Schedule an callback to fire them in an async // event. To ensure serial execution, the callback will be flushed early if // we enter rootWithPendingPassiveEffects commit phase before then. - const resolvedFirstEffect = (firstPassiveEffect = firstEffect); - - let passiveEffectCallback; + let callback = commitPassiveEffects.bind(null, root, firstEffect); if (enableSchedulerTracing) { // TODO: Avoid this extra callback by mutating the tracing ref directly, // like we do at the beginning of commitRoot. I've opted not to do that // here because that code is still in flux. - passiveEffectCallback = Schedule_tracing_wrap(() => { - commitPassiveEffects(root, resolvedFirstEffect); - }); - } else { - passiveEffectCallback = commitPassiveEffects.bind( - null, - root, - resolvedFirstEffect, - ); + callback = Schedule_tracing_wrap(callback); } - passiveEffectCallbackHandle = Schedule_scheduleCallback( - passiveEffectCallback, - ); + passiveEffectCallbackHandle = Schedule_scheduleCallback(callback); + passiveEffectCallback = callback; } isCommitting = false; @@ -1798,7 +1787,8 @@ function scheduleWorkToRoot(fiber: Fiber, expirationTime): FiberRoot | null { break; case FunctionComponent: case ForwardRef: - case PureComponent: + case MemoComponent: + case SimpleMemoComponent: warnAboutUpdateOnUnmounted(fiber, false); break; } diff --git a/packages/react-reconciler/src/__tests__/ReactHooks-test.internal.js b/packages/react-reconciler/src/__tests__/ReactHooks-test.internal.js index 5f6681babd9a0..22ce1bc4e4e93 100644 --- a/packages/react-reconciler/src/__tests__/ReactHooks-test.internal.js +++ b/packages/react-reconciler/src/__tests__/ReactHooks-test.internal.js @@ -15,6 +15,7 @@ let React; let ReactFeatureFlags; let ReactNoop; +let SchedulerTracing; let useState; let useReducer; let useEffect; @@ -26,6 +27,7 @@ let useRef; let useAPI; let forwardRef; let flushPassiveEffects; +let memo; describe('ReactHooks', () => { beforeEach(() => { @@ -56,8 +58,10 @@ describe('ReactHooks', () => { ReactFeatureFlags = require('shared/ReactFeatureFlags'); ReactFeatureFlags.debugRenderPhaseSideEffectsForStrictMode = false; ReactFeatureFlags.enableHooks = true; + ReactFeatureFlags.enableSchedulerTracing = true; React = require('react'); ReactNoop = require('react-noop-renderer'); + SchedulerTracing = require('scheduler/tracing'); useState = React.useState; useReducer = React.useReducer; useEffect = React.useEffect; @@ -68,6 +72,7 @@ describe('ReactHooks', () => { useRef = React.useRef; useAPI = React.useAPI; forwardRef = React.forwardRef; + memo = React.memo; }); function span(prop) { @@ -330,6 +335,28 @@ describe('ReactHooks', () => { ' in Counter (at **)', ); }); + + it('works with memo', () => { + let _updateCount; + function Counter(props) { + const [count, updateCount] = useState(0); + _updateCount = updateCount; + return ; + } + Counter = memo(Counter); + + ReactNoop.render(); + expect(ReactNoop.flush()).toEqual(['Count: 0']); + expect(ReactNoop.getChildren()).toEqual([span('Count: 0')]); + + ReactNoop.render(); + expect(ReactNoop.flush()).toEqual([]); + expect(ReactNoop.getChildren()).toEqual([span('Count: 0')]); + + _updateCount(1); + expect(ReactNoop.flush()).toEqual(['Count: 1']); + expect(ReactNoop.getChildren()).toEqual([span('Count: 1')]); + }); }); describe('updates during the render phase', () => { @@ -797,6 +824,7 @@ describe('ReactHooks', () => { }, []); return ; } + ReactNoop.render(); expect(ReactNoop.flush()).toEqual(['Count: 0']); expect(ReactNoop.getChildren()).toEqual([span('Count: 0')]); @@ -808,6 +836,56 @@ describe('ReactHooks', () => { expect(ReactNoop.getChildren()).toEqual([span('Count: 2')]); }); + it('flushes serial effects before enqueueing work (with tracing)', () => { + const onInteractionScheduledWorkCompleted = jest.fn(); + const onWorkCanceled = jest.fn(); + SchedulerTracing.unstable_subscribe({ + onInteractionScheduledWorkCompleted, + onInteractionTraced: jest.fn(), + onWorkCanceled, + onWorkScheduled: jest.fn(), + onWorkStarted: jest.fn(), + onWorkStopped: jest.fn(), + }); + + let _updateCount; + function Counter(props) { + const [count, updateCount] = useState(0); + _updateCount = updateCount; + useEffect(() => { + expect(SchedulerTracing.unstable_getCurrent()).toMatchInteractions([ + tracingEvent, + ]); + ReactNoop.yield(`Will set count to 1`); + updateCount(1); + }, []); + return ; + } + + const tracingEvent = {id: 0, name: 'hello', timestamp: 0}; + SchedulerTracing.unstable_trace( + tracingEvent.name, + tracingEvent.timestamp, + () => { + ReactNoop.render(); + }, + ); + expect(ReactNoop.flush()).toEqual(['Count: 0']); + expect(ReactNoop.getChildren()).toEqual([span('Count: 0')]); + + expect(onInteractionScheduledWorkCompleted).toHaveBeenCalledTimes(0); + + // Enqueuing this update forces the passive effect to be flushed -- + // updateCount(1) happens first, so 2 wins. + _updateCount(2); + expect(ReactNoop.flush()).toEqual(['Will set count to 1', 'Count: 2']); + expect(ReactNoop.getChildren()).toEqual([span('Count: 2')]); + + flushPassiveEffects(); + expect(onInteractionScheduledWorkCompleted).toHaveBeenCalledTimes(1); + expect(onWorkCanceled).toHaveBeenCalledTimes(0); + }); + it( 'in sync mode, useEffect is deferred and updates finish synchronously ' + '(in a single batch)', From 75a1c2e72a8ebff2ec5e9b89e3c0e04f576731b7 Mon Sep 17 00:00:00 2001 From: Sophie Alpert Date: Fri, 12 Oct 2018 18:04:49 -0700 Subject: [PATCH 10/19] The Lost Effect, chapter 3 wow, writing code is hard --- .../src/ReactFiberClassComponent.js | 8 ++--- .../src/ReactFiberCommitWork.js | 4 +-- .../react-reconciler/src/ReactFiberHooks.js | 4 +-- .../src/ReactFiberReconciler.js | 4 +-- .../src/ReactFiberScheduler.js | 33 +++---------------- .../src/__tests__/ReactHooks-test.internal.js | 29 ++++++++++++++++ 6 files changed, 43 insertions(+), 39 deletions(-) diff --git a/packages/react-reconciler/src/ReactFiberClassComponent.js b/packages/react-reconciler/src/ReactFiberClassComponent.js index b2b50a6a52567..9a167e6f4c8ca 100644 --- a/packages/react-reconciler/src/ReactFiberClassComponent.js +++ b/packages/react-reconciler/src/ReactFiberClassComponent.js @@ -51,7 +51,7 @@ import { requestCurrentTime, computeExpirationForFiber, scheduleWork, - flushPassiveEffectsBeforeSchedulingUpdateOnFiber, + flushPassiveEffects, } from './ReactFiberScheduler'; const ReactCurrentOwner = ReactSharedInternals.ReactCurrentOwner; @@ -200,7 +200,7 @@ const classComponentUpdater = { update.callback = callback; } - flushPassiveEffectsBeforeSchedulingUpdateOnFiber(fiber); + flushPassiveEffects(); enqueueUpdate(fiber, update); scheduleWork(fiber, expirationTime); }, @@ -220,7 +220,7 @@ const classComponentUpdater = { update.callback = callback; } - flushPassiveEffectsBeforeSchedulingUpdateOnFiber(fiber); + flushPassiveEffects(); enqueueUpdate(fiber, update); scheduleWork(fiber, expirationTime); }, @@ -239,7 +239,7 @@ const classComponentUpdater = { update.callback = callback; } - flushPassiveEffectsBeforeSchedulingUpdateOnFiber(fiber); + flushPassiveEffects(); enqueueUpdate(fiber, update); scheduleWork(fiber, expirationTime); }, diff --git a/packages/react-reconciler/src/ReactFiberCommitWork.js b/packages/react-reconciler/src/ReactFiberCommitWork.js index 3f61066dd9d62..2f76b6ff50ebf 100644 --- a/packages/react-reconciler/src/ReactFiberCommitWork.js +++ b/packages/react-reconciler/src/ReactFiberCommitWork.js @@ -85,7 +85,7 @@ import { } from './ReactFiberHostConfig'; import { captureCommitPhaseError, - flushPassiveEffectsBeforeSchedulingUpdateOnFiber, + flushPassiveEffects, requestCurrentTime, scheduleWork, } from './ReactFiberScheduler'; @@ -457,7 +457,7 @@ function commitLifeCycles( timedOutAt: NoWork, }; finishedWork.memoizedState = newState; - flushPassiveEffectsBeforeSchedulingUpdateOnFiber(finishedWork); + flushPassiveEffects(); scheduleWork(finishedWork, Sync); return; } diff --git a/packages/react-reconciler/src/ReactFiberHooks.js b/packages/react-reconciler/src/ReactFiberHooks.js index 5e843c5e53287..eef5c602b07be 100644 --- a/packages/react-reconciler/src/ReactFiberHooks.js +++ b/packages/react-reconciler/src/ReactFiberHooks.js @@ -32,7 +32,7 @@ import { import { scheduleWork, computeExpirationForFiber, - flushPassiveEffectsBeforeSchedulingUpdateOnFiber, + flushPassiveEffects, requestCurrentTime, } from './ReactFiberScheduler'; @@ -725,7 +725,7 @@ function dispatchAction( callback: callback !== undefined ? callback : null, next: null, }; - flushPassiveEffectsBeforeSchedulingUpdateOnFiber(fiber); + flushPassiveEffects(); // Append the update to the end of the list. const last = queue.last; if (last === null) { diff --git a/packages/react-reconciler/src/ReactFiberReconciler.js b/packages/react-reconciler/src/ReactFiberReconciler.js index b06922c94f80b..a61c1a3304905 100644 --- a/packages/react-reconciler/src/ReactFiberReconciler.js +++ b/packages/react-reconciler/src/ReactFiberReconciler.js @@ -52,7 +52,7 @@ import { syncUpdates, interactiveUpdates, flushInteractiveUpdates, - flushPassiveEffectsBeforeSchedulingUpdateOnFiber, + flushPassiveEffects, } from './ReactFiberScheduler'; import {createUpdate, enqueueUpdate} from './ReactUpdateQueue'; import ReactFiberInstrumentation from './ReactFiberInstrumentation'; @@ -147,7 +147,7 @@ function scheduleRootUpdate( update.callback = callback; } - flushPassiveEffectsBeforeSchedulingUpdateOnFiber(current); + flushPassiveEffects(); enqueueUpdate(current, update); scheduleWork(current, expirationTime); diff --git a/packages/react-reconciler/src/ReactFiberScheduler.js b/packages/react-reconciler/src/ReactFiberScheduler.js index 21a919b5c5058..b49715971f891 100644 --- a/packages/react-reconciler/src/ReactFiberScheduler.js +++ b/packages/react-reconciler/src/ReactFiberScheduler.js @@ -578,33 +578,8 @@ function markLegacyErrorBoundaryAsFailed(instance: mixed) { } } -function flushPassiveEffectsBeforeSchedulingUpdateOnFiber(fiber: Fiber) { - if (rootWithPendingPassiveEffects !== null) { - // TODO: This is an unfortunate extra loop. We end up traversing to the root - // again in scheduleWorkToRoot. But we have to do this one first because it - // needs to happen before adding an update to the queue, and - // scheduleWorkToRoot may perform a synchronous re-render. Maybe we can - // solve this with batchedUpdates, or with the equivalent in the Scheduler - // package. - let node = fiber; - do { - switch (node.tag) { - case HostRoot: { - const root: FiberRoot = node.stateNode; - flushPassiveEffects(root); - return; - } - } - node = node.return; - } while (node !== null); - } -} - -function flushPassiveEffects(root: FiberRoot) { - if ( - passiveEffectCallback !== null && - root === rootWithPendingPassiveEffects - ) { +function flushPassiveEffects() { + if (passiveEffectCallback !== null) { Schedule_cancelCallback(passiveEffectCallbackHandle); // We call the scheduled callback instead of commitPassiveEffects directly // to ensure tracing works correctly. @@ -1248,7 +1223,7 @@ function renderRoot( 'by a bug in React. Please file an issue.', ); - flushPassiveEffects(root); + flushPassiveEffects(); isWorking = true; ReactCurrentOwner.currentDispatcher = Dispatcher; @@ -2613,5 +2588,5 @@ export { interactiveUpdates, flushInteractiveUpdates, computeUniqueAsyncExpiration, - flushPassiveEffectsBeforeSchedulingUpdateOnFiber, + flushPassiveEffects, }; diff --git a/packages/react-reconciler/src/__tests__/ReactHooks-test.internal.js b/packages/react-reconciler/src/__tests__/ReactHooks-test.internal.js index 22ce1bc4e4e93..0c38e6155944d 100644 --- a/packages/react-reconciler/src/__tests__/ReactHooks-test.internal.js +++ b/packages/react-reconciler/src/__tests__/ReactHooks-test.internal.js @@ -717,6 +717,35 @@ describe('ReactHooks', () => { ]); }); + it('flushes passive effects even if siblings schedule a new root', () => { + function PassiveEffect(props) { + useEffect(() => { + ReactNoop.yield('Passive effect'); + }, []); + return ; + } + function LayoutEffect(props) { + useLayoutEffect(() => { + ReactNoop.yield('Layout effect'); + // Scheduling work shouldn't interfere with the queued passive effect + ReactNoop.renderToRootWithID(, 'root2'); + }); + return ; + } + ReactNoop.render([, ]); + expect(ReactNoop.flush()).toEqual([ + 'Passive', + 'Layout', + 'Layout effect', + 'Passive effect', + 'New Root', + ]); + expect(ReactNoop.getChildren()).toEqual([ + span('Passive'), + span('Layout'), + ]); + }); + it( 'flushes effects serially by flushing old effects before flushing ' + "new ones, if they haven't already fired", From 3a7c6da8d46763991d47ebf2198bc4e2fbfa0d9a Mon Sep 17 00:00:00 2001 From: Sophie Alpert Date: Sat, 13 Oct 2018 16:02:38 -0700 Subject: [PATCH 11/19] Make effects actually work with memo Bug fix. --- .../src/__tests__/ReactHooks-test.internal.js | 23 +++++++++++++++++++ 1 file changed, 23 insertions(+) diff --git a/packages/react-reconciler/src/__tests__/ReactHooks-test.internal.js b/packages/react-reconciler/src/__tests__/ReactHooks-test.internal.js index 0c38e6155944d..9ee5ef633cc25 100644 --- a/packages/react-reconciler/src/__tests__/ReactHooks-test.internal.js +++ b/packages/react-reconciler/src/__tests__/ReactHooks-test.internal.js @@ -1270,6 +1270,29 @@ describe('ReactHooks', () => { ]); expect(ReactNoop.getChildren()).toEqual([]); }); + + it('works with memo', () => { + function Counter({count}) { + useLayoutEffect(() => { + ReactNoop.yield('Mount: ' + count); + return () => ReactNoop.yield('Unmount: ' + count); + }); + return ; + } + Counter = memo(Counter); + + ReactNoop.render(); + expect(ReactNoop.flush()).toEqual(['Count: 0', 'Mount: 0']); + expect(ReactNoop.getChildren()).toEqual([span('Count: 0')]); + + ReactNoop.render(); + expect(ReactNoop.flush()).toEqual(['Count: 1', 'Unmount: 0', 'Mount: 1']); + expect(ReactNoop.getChildren()).toEqual([span('Count: 1')]); + + ReactNoop.render(null); + expect(ReactNoop.flush()).toEqual(['Unmount: 1']); + expect(ReactNoop.getChildren()).toEqual([]); + }); }); describe('useMutationEffect and useLayoutEffect', () => { From 63cc7d2b31b4e5ad17c07b383a328c598c91276e Mon Sep 17 00:00:00 2001 From: Dan Abramov Date: Mon, 15 Oct 2018 17:20:05 -0400 Subject: [PATCH 12/19] Test useContext in pure, forwardRef, and PureComponent --- .../ReactNewContext-test.internal.js | 97 +++++++++++-------- 1 file changed, 59 insertions(+), 38 deletions(-) diff --git a/packages/react-reconciler/src/__tests__/ReactNewContext-test.internal.js b/packages/react-reconciler/src/__tests__/ReactNewContext-test.internal.js index 25c3a3a1ad535..4ebcd1f84d6c7 100644 --- a/packages/react-reconciler/src/__tests__/ReactNewContext-test.internal.js +++ b/packages/react-reconciler/src/__tests__/ReactNewContext-test.internal.js @@ -57,6 +57,25 @@ describe('ReactNewContext', () => { return render(contextValue); }, ); + sharedContextTests('useContext inside forwardRef component', Context => + React.forwardRef(function Consumer(props, ref) { + const observedBits = props.unstable_observedBits; + const contextValue = useContext(Context, observedBits); + const render = props.children; + return render(contextValue); + }), + ); + sharedContextTests( + 'useContext inside memoized function component', + Context => + React.memo(function Consumer(props) { + const observedBits = props.unstable_observedBits; + const contextValue = useContext(Context, observedBits); + const render = props.children; + return render(contextValue); + }), + true, + ); sharedContextTests( 'readContext(Context) inside class component', Context => @@ -70,7 +89,7 @@ describe('ReactNewContext', () => { }, ); - function sharedContextTests(label, getConsumer) { + function sharedContextTests(label, getConsumer, isPure) { describe(`reading context with ${label}`, () => { it('simple mount and update', () => { const Context = React.createContext(1); @@ -855,46 +874,48 @@ describe('ReactNewContext', () => { expect(ReactNoop.getChildren()).toEqual([span(2), span(2)]); }); - // Context consumer bails out on propagating "deep" updates when `value` hasn't changed. - // However, it doesn't bail out from rendering if the component above it re-rendered anyway. - // If we bailed out on referential equality, it would be confusing that you - // can call this.setState(), but an autobound render callback "blocked" the update. - // https://github.com/facebook/react/pull/12470#issuecomment-376917711 - it('consumer does not bail out if there were no bailouts above it', () => { - const Context = React.createContext(0); - const Consumer = getConsumer(Context); - - class App extends React.Component { - state = { - text: 'hello', - }; - - renderConsumer = context => { - ReactNoop.yield('App#renderConsumer'); - return ; - }; - - render() { - ReactNoop.yield('App'); - return ( - - {this.renderConsumer} - - ); + if (!isPure) { + // Context consumer bails out on propagating "deep" updates when `value` hasn't changed. + // However, it doesn't bail out from rendering if the component above it re-rendered anyway. + // If we bailed out on referential equality, it would be confusing that you + // can call this.setState(), but an autobound render callback "blocked" the update. + // https://github.com/facebook/react/pull/12470#issuecomment-376917711 + it('consumer does not bail out if there were no bailouts above it', () => { + const Context = React.createContext(0); + const Consumer = getConsumer(Context); + + class App extends React.Component { + state = { + text: 'hello', + }; + + renderConsumer = context => { + ReactNoop.yield('App#renderConsumer'); + return ; + }; + + render() { + ReactNoop.yield('App'); + return ( + + {this.renderConsumer} + + ); + } } - } - // Initial mount - let inst; - ReactNoop.render( (inst = ref)} />); - expect(ReactNoop.flush()).toEqual(['App', 'App#renderConsumer']); - expect(ReactNoop.getChildren()).toEqual([span('hello')]); + // Initial mount + let inst; + ReactNoop.render( (inst = ref)} />); + expect(ReactNoop.flush()).toEqual(['App', 'App#renderConsumer']); + expect(ReactNoop.getChildren()).toEqual([span('hello')]); - // Update - inst.setState({text: 'goodbye'}); - expect(ReactNoop.flush()).toEqual(['App', 'App#renderConsumer']); - expect(ReactNoop.getChildren()).toEqual([span('goodbye')]); - }); + // Update + inst.setState({text: 'goodbye'}); + expect(ReactNoop.flush()).toEqual(['App', 'App#renderConsumer']); + expect(ReactNoop.getChildren()).toEqual([span('goodbye')]); + }); + } // This is a regression case for https://github.com/facebook/react/issues/12389. it('does not run into an infinite loop', () => { From f7cb9d2b2291b2b4725786105ddd89316711161f Mon Sep 17 00:00:00 2001 From: Dan Abramov Date: Mon, 15 Oct 2018 18:31:14 -0400 Subject: [PATCH 13/19] Warn about useContext(Consumer|Provider) --- .../ReactNewContext-test.internal.js | 235 ++++++++++++++---- packages/react/src/ReactHooks.js | 22 ++ 2 files changed, 203 insertions(+), 54 deletions(-) diff --git a/packages/react-reconciler/src/__tests__/ReactNewContext-test.internal.js b/packages/react-reconciler/src/__tests__/ReactNewContext-test.internal.js index 4ebcd1f84d6c7..4cc8b4b15f9a9 100644 --- a/packages/react-reconciler/src/__tests__/ReactNewContext-test.internal.js +++ b/packages/react-reconciler/src/__tests__/ReactNewContext-test.internal.js @@ -65,16 +65,13 @@ describe('ReactNewContext', () => { return render(contextValue); }), ); - sharedContextTests( - 'useContext inside memoized function component', - Context => - React.memo(function Consumer(props) { - const observedBits = props.unstable_observedBits; - const contextValue = useContext(Context, observedBits); - const render = props.children; - return render(contextValue); - }), - true, + sharedContextTests('useContext inside memoized function component', Context => + React.memo(function Consumer(props) { + const observedBits = props.unstable_observedBits; + const contextValue = useContext(Context, observedBits); + const render = props.children; + return render(contextValue); + }), ); sharedContextTests( 'readContext(Context) inside class component', @@ -88,8 +85,20 @@ describe('ReactNewContext', () => { } }, ); + sharedContextTests( + 'readContext(Context) inside pure class component', + Context => + class Consumer extends React.PureComponent { + render() { + const observedBits = this.props.unstable_observedBits; + const contextValue = readContext(Context, observedBits); + const render = this.props.children; + return render(contextValue); + } + }, + ); - function sharedContextTests(label, getConsumer, isPure) { + function sharedContextTests(label, getConsumer) { describe(`reading context with ${label}`, () => { it('simple mount and update', () => { const Context = React.createContext(1); @@ -874,49 +883,6 @@ describe('ReactNewContext', () => { expect(ReactNoop.getChildren()).toEqual([span(2), span(2)]); }); - if (!isPure) { - // Context consumer bails out on propagating "deep" updates when `value` hasn't changed. - // However, it doesn't bail out from rendering if the component above it re-rendered anyway. - // If we bailed out on referential equality, it would be confusing that you - // can call this.setState(), but an autobound render callback "blocked" the update. - // https://github.com/facebook/react/pull/12470#issuecomment-376917711 - it('consumer does not bail out if there were no bailouts above it', () => { - const Context = React.createContext(0); - const Consumer = getConsumer(Context); - - class App extends React.Component { - state = { - text: 'hello', - }; - - renderConsumer = context => { - ReactNoop.yield('App#renderConsumer'); - return ; - }; - - render() { - ReactNoop.yield('App'); - return ( - - {this.renderConsumer} - - ); - } - } - - // Initial mount - let inst; - ReactNoop.render( (inst = ref)} />); - expect(ReactNoop.flush()).toEqual(['App', 'App#renderConsumer']); - expect(ReactNoop.getChildren()).toEqual([span('hello')]); - - // Update - inst.setState({text: 'goodbye'}); - expect(ReactNoop.flush()).toEqual(['App', 'App#renderConsumer']); - expect(ReactNoop.getChildren()).toEqual([span('goodbye')]); - }); - } - // This is a regression case for https://github.com/facebook/react/issues/12389. it('does not run into an infinite loop', () => { const Context = React.createContext(null); @@ -1260,6 +1226,96 @@ describe('ReactNewContext', () => { expect(ReactNoop.flush()).toEqual(['Foo: 2, Bar: 2']); expect(ReactNoop.getChildren()).toEqual([span('Foo: 2, Bar: 2')]); }); + + // Context consumer bails out on propagating "deep" updates when `value` hasn't changed. + // However, it doesn't bail out from rendering if the component above it re-rendered anyway. + // If we bailed out on referential equality, it would be confusing that you + // can call this.setState(), but an autobound render callback "blocked" the update. + // https://github.com/facebook/react/pull/12470#issuecomment-376917711 + it('consumer does not bail out if there were no bailouts above it', () => { + const Context = React.createContext(0); + const Consumer = Context.Consumer; + + class App extends React.Component { + state = { + text: 'hello', + }; + + renderConsumer = context => { + ReactNoop.yield('App#renderConsumer'); + return ; + }; + + render() { + ReactNoop.yield('App'); + return ( + + {this.renderConsumer} + + ); + } + } + + // Initial mount + let inst; + ReactNoop.render( (inst = ref)} />); + expect(ReactNoop.flush()).toEqual(['App', 'App#renderConsumer']); + expect(ReactNoop.getChildren()).toEqual([span('hello')]); + + // Update + inst.setState({text: 'goodbye'}); + expect(ReactNoop.flush()).toEqual(['App', 'App#renderConsumer']); + expect(ReactNoop.getChildren()).toEqual([span('goodbye')]); + }); + }); + + describe('readContext', () => { + // Context consumer bails out on propagating "deep" updates when `value` hasn't changed. + // However, it doesn't bail out from rendering if the component above it re-rendered anyway. + // If we bailed out on referential equality, it would be confusing that you + // can call this.setState(), but an autobound render callback "blocked" the update. + // https://github.com/facebook/react/pull/12470#issuecomment-376917711 + it('does not bail out if there were no bailouts above it', () => { + const Context = React.createContext(0); + + class Consumer extends React.Component { + render() { + const contextValue = readContext(Context); + return this.props.children(contextValue); + } + } + + class App extends React.Component { + state = { + text: 'hello', + }; + + renderConsumer = context => { + ReactNoop.yield('App#renderConsumer'); + return ; + }; + + render() { + ReactNoop.yield('App'); + return ( + + {this.renderConsumer} + + ); + } + } + + // Initial mount + let inst; + ReactNoop.render( (inst = ref)} />); + expect(ReactNoop.flush()).toEqual(['App', 'App#renderConsumer']); + expect(ReactNoop.getChildren()).toEqual([span('hello')]); + + // Update + inst.setState({text: 'goodbye'}); + expect(ReactNoop.flush()).toEqual(['App', 'App#renderConsumer']); + expect(ReactNoop.getChildren()).toEqual([span('goodbye')]); + }); }); describe('useContext', () => { @@ -1366,6 +1422,77 @@ describe('ReactNewContext', () => { 'Hooks can only be called inside the body of a function component.', ); }); + + it('warns when passed a consumer', () => { + const Context = React.createContext(0); + function Foo() { + return useContext(Context.Consumer); + } + ReactNoop.render(); + expect(ReactNoop.flush).toWarnDev( + 'Calling useContext(Context.Consumer) is not supported, may cause bugs, ' + + 'and will be removed in a future major release. ' + + 'Did you mean to call useContext(Context) instead?', + ); + }); + + it('warns when passed a provider', () => { + const Context = React.createContext(0); + function Foo() { + useContext(Context.Provider); + return null; + } + ReactNoop.render(); + expect(ReactNoop.flush).toWarnDev( + 'Calling useContext(Context.Provider) is not supported. ' + + 'Did you mean to call useContext(Context) instead?', + ); + }); + + // Context consumer bails out on propagating "deep" updates when `value` hasn't changed. + // However, it doesn't bail out from rendering if the component above it re-rendered anyway. + // If we bailed out on referential equality, it would be confusing that you + // can call this.setState(), but an autobound render callback "blocked" the update. + // https://github.com/facebook/react/pull/12470#issuecomment-376917711 + it('does not bail out if there were no bailouts above it', () => { + const Context = React.createContext(0); + + function Consumer({children}) { + const contextValue = useContext(Context); + return children(contextValue); + } + + class App extends React.Component { + state = { + text: 'hello', + }; + + renderConsumer = context => { + ReactNoop.yield('App#renderConsumer'); + return ; + }; + + render() { + ReactNoop.yield('App'); + return ( + + {this.renderConsumer} + + ); + } + } + + // Initial mount + let inst; + ReactNoop.render( (inst = ref)} />); + expect(ReactNoop.flush()).toEqual(['App', 'App#renderConsumer']); + expect(ReactNoop.getChildren()).toEqual([span('hello')]); + + // Update + inst.setState({text: 'goodbye'}); + expect(ReactNoop.flush()).toEqual(['App', 'App#renderConsumer']); + expect(ReactNoop.getChildren()).toEqual([span('goodbye')]); + }); }); it('unwinds after errors in complete phase', () => { diff --git a/packages/react/src/ReactHooks.js b/packages/react/src/ReactHooks.js index e2ccc3a2a8cc7..1cc8b79ac57f8 100644 --- a/packages/react/src/ReactHooks.js +++ b/packages/react/src/ReactHooks.js @@ -9,6 +9,7 @@ import type {ReactContext} from 'shared/ReactTypes'; import invariant from 'shared/invariant'; +import warning from 'shared/warning'; import ReactCurrentOwner from './ReactCurrentOwner'; @@ -26,6 +27,27 @@ export function useContext( observedBits: number | boolean | void, ) { const dispatcher = resolveDispatcher(); + if (__DEV__) { + // TODO: add a more generic warning for invalid values. + if ((Context: any)._context !== undefined) { + const realContext = (Context: any)._context; + // Don't deduplicate because this legitimately causes bugs + // and nobody should be using this in existing code. + if (realContext.Consumer === Context) { + warning( + false, + 'Calling useContext(Context.Consumer) is not supported, may cause bugs, and will be ' + + 'removed in a future major release. Did you mean to call useContext(Context) instead?', + ); + } else if (realContext.Provider === Context) { + warning( + false, + 'Calling useContext(Context.Provider) is not supported. ' + + 'Did you mean to call useContext(Context) instead?', + ); + } + } + } return dispatcher.useContext(Context, observedBits); } From 9f34eb79a3f7df71cd89b247de2e12b41b98f55a Mon Sep 17 00:00:00 2001 From: Alex Taylor Date: Thu, 11 Oct 2018 10:17:39 -0700 Subject: [PATCH 14/19] Add readContext to ReactPartialRendererHooks --- ...actDOMServerIntegrationHooks-test.internal.js | 16 ++++++++++++++++ .../src/server/ReactPartialRendererHooks.js | 10 +++++++++- 2 files changed, 25 insertions(+), 1 deletion(-) diff --git a/packages/react-dom/src/__tests__/ReactDOMServerIntegrationHooks-test.internal.js b/packages/react-dom/src/__tests__/ReactDOMServerIntegrationHooks-test.internal.js index 8c991c9ebe8c7..a616ff395f5a3 100644 --- a/packages/react-dom/src/__tests__/ReactDOMServerIntegrationHooks-test.internal.js +++ b/packages/react-dom/src/__tests__/ReactDOMServerIntegrationHooks-test.internal.js @@ -646,5 +646,21 @@ describe('ReactDOMServerHooks', () => { expect(domNode.lastChild.textContent).toEqual('Baz: 5'); }, ); + + itThrowsWhenRendering( + 'if used inside a class component', + async render => { + const Context = React.createContext({}, () => {}); + class Counter extends React.Component { + render() { + let [count] = useContext(Context); + return ; + } + } + + return render(); + }, + 'Hooks can only be called inside the body of a function component.', + ); }); }); diff --git a/packages/react-dom/src/server/ReactPartialRendererHooks.js b/packages/react-dom/src/server/ReactPartialRendererHooks.js index bec21679c1132..1bebbc744462f 100644 --- a/packages/react-dom/src/server/ReactPartialRendererHooks.js +++ b/packages/react-dom/src/server/ReactPartialRendererHooks.js @@ -138,10 +138,18 @@ export function finishHooks( return children; } +function readContext( + context: ReactContext, + observedBits: void | number | boolean, +): T { + return context._currentValue; +} + function useContext( context: ReactContext, observedBits: void | number | boolean, ): T { + resolveCurrentlyRenderingComponent(); return context._currentValue; } @@ -349,7 +357,7 @@ function inputsAreEqual(arr1, arr2) { function noop(): void {} export const Dispatcher = { - readContext: useContext, + readContext, useContext, useMemo, useReducer, From 5fc84efacce66272928815e8a6c83a341af9160e Mon Sep 17 00:00:00 2001 From: Sophie Alpert Date: Tue, 16 Oct 2018 13:02:02 -0700 Subject: [PATCH 15/19] Skip updating effect tag when skipping effect For example, if you have `useEffect(..., [])`, there's no need to set .effectTag to `Update | Passive` on updates. --- ...DOMServerIntegrationHooks-test.internal.js | 8 +-- .../src/server/ReactPartialRendererHooks.js | 4 +- .../src/ReactFiberDispatcher.js | 4 +- .../react-reconciler/src/ReactFiberHooks.js | 29 ++++++----- .../src/__tests__/ReactHooks-test.internal.js | 50 ++++++++++++++----- packages/react/src/React.js | 4 +- packages/react/src/ReactHooks.js | 4 +- 7 files changed, 64 insertions(+), 39 deletions(-) diff --git a/packages/react-dom/src/__tests__/ReactDOMServerIntegrationHooks-test.internal.js b/packages/react-dom/src/__tests__/ReactDOMServerIntegrationHooks-test.internal.js index a616ff395f5a3..37315929e42fb 100644 --- a/packages/react-dom/src/__tests__/ReactDOMServerIntegrationHooks-test.internal.js +++ b/packages/react-dom/src/__tests__/ReactDOMServerIntegrationHooks-test.internal.js @@ -24,7 +24,7 @@ let useContext; let useCallback; let useMemo; let useRef; -let useAPI; +let useImperativeMethods; let useMutationEffect; let useLayoutEffect; let forwardRef; @@ -49,7 +49,7 @@ function initModules() { useCallback = React.useCallback; useMemo = React.useMemo; useRef = React.useRef; - useAPI = React.useAPI; + useImperativeMethods = React.useImperativeMethods; useMutationEffect = React.useMutationEffect; useLayoutEffect = React.useLayoutEffect; forwardRef = React.forwardRef; @@ -518,10 +518,10 @@ describe('ReactDOMServerHooks', () => { }); }); - describe('useAPI', () => { + describe('useImperativeMethods', () => { it('should not be invoked on the server', async () => { function Counter(props, ref) { - useAPI(ref, () => { + useImperativeMethods(ref, () => { throw new Error('should not be invoked'); }); return ; diff --git a/packages/react-dom/src/server/ReactPartialRendererHooks.js b/packages/react-dom/src/server/ReactPartialRendererHooks.js index 1bebbc744462f..b859e6dfcaaae 100644 --- a/packages/react-dom/src/server/ReactPartialRendererHooks.js +++ b/packages/react-dom/src/server/ReactPartialRendererHooks.js @@ -365,8 +365,8 @@ export const Dispatcher = { useState, useMutationEffect, useLayoutEffect, - // useAPI is not run in the server environment - useAPI: noop, + // useImperativeMethods is not run in the server environment + useImperativeMethods: noop, // Callbacks are not run in the server environment. useCallback: noop, // Effects are not run in the server environment. diff --git a/packages/react-reconciler/src/ReactFiberDispatcher.js b/packages/react-reconciler/src/ReactFiberDispatcher.js index 10c0dee649a64..b5f27df334b7f 100644 --- a/packages/react-reconciler/src/ReactFiberDispatcher.js +++ b/packages/react-reconciler/src/ReactFiberDispatcher.js @@ -9,10 +9,10 @@ import {readContext} from './ReactFiberNewContext'; import { - useAPI, useCallback, useContext, useEffect, + useImperativeMethods, useLayoutEffect, useMemo, useMutationEffect, @@ -23,10 +23,10 @@ import { export const Dispatcher = { readContext, - useAPI, useCallback, useContext, useEffect, + useImperativeMethods, useLayoutEffect, useMemo, useMutationEffect, diff --git a/packages/react-reconciler/src/ReactFiberHooks.js b/packages/react-reconciler/src/ReactFiberHooks.js index eef5c602b07be..35ad699d20253 100644 --- a/packages/react-reconciler/src/ReactFiberHooks.js +++ b/packages/react-reconciler/src/ReactFiberHooks.js @@ -179,9 +179,7 @@ export function finishHooks( renderedWork.memoizedState = firstWorkInProgressHook; renderedWork.expirationTime = remainingExpirationTime; - if (componentUpdateQueue !== null) { - renderedWork.updateQueue = (componentUpdateQueue: any); - } + renderedWork.updateQueue = (componentUpdateQueue: any); const didRenderTooFewHooks = currentHook !== null && currentHook.next !== null; @@ -576,26 +574,27 @@ function useEffectImpl(fiberEffectTag, hookEffectTag, create, inputs): void { currentlyRenderingFiber = resolveCurrentlyRenderingFiber(); workInProgressHook = createWorkInProgressHook(); - let nextEffect; let nextInputs = inputs !== undefined && inputs !== null ? inputs : [create]; + let destroy = null; if (currentHook !== null) { const prevEffect = currentHook.memoizedState; - const prevInputs = prevEffect.inputs; - nextEffect = pushEffect( - inputsAreEqual(nextInputs, prevInputs) ? NoHookEffect : hookEffectTag, - create, - prevEffect.destroy, - nextInputs, - ); - } else { - nextEffect = pushEffect(hookEffectTag, create, null, nextInputs); + destroy = prevEffect.destroy; + if (inputsAreEqual(nextInputs, prevEffect.inputs)) { + pushEffect(NoHookEffect, create, destroy, nextInputs); + return; + } } - workInProgressHook.memoizedState = nextEffect; currentlyRenderingFiber.effectTag |= fiberEffectTag; + workInProgressHook.memoizedState = pushEffect( + hookEffectTag, + create, + destroy, + nextInputs, + ); } -export function useAPI( +export function useImperativeMethods( ref: {current: T | null} | ((inst: T | null) => mixed) | null | void, create: () => T, inputs: Array | void | null, diff --git a/packages/react-reconciler/src/__tests__/ReactHooks-test.internal.js b/packages/react-reconciler/src/__tests__/ReactHooks-test.internal.js index 9ee5ef633cc25..c6844256d6d7e 100644 --- a/packages/react-reconciler/src/__tests__/ReactHooks-test.internal.js +++ b/packages/react-reconciler/src/__tests__/ReactHooks-test.internal.js @@ -24,7 +24,7 @@ let useLayoutEffect; let useCallback; let useMemo; let useRef; -let useAPI; +let useImperativeMethods; let forwardRef; let flushPassiveEffects; let memo; @@ -70,7 +70,7 @@ describe('ReactHooks', () => { useCallback = React.useCallback; useMemo = React.useMemo; useRef = React.useRef; - useAPI = React.useAPI; + useImperativeMethods = React.useImperativeMethods; forwardRef = React.forwardRef; memo = React.memo; }); @@ -87,7 +87,7 @@ describe('ReactHooks', () => { it('resumes after an interruption', () => { function Counter(props, ref) { const [count, updateCount] = useState(0); - useAPI(ref, () => ({updateCount})); + useImperativeMethods(ref, () => ({updateCount})); return ; } Counter = forwardRef(Counter); @@ -171,7 +171,7 @@ describe('ReactHooks', () => { it('simple mount and update', () => { function Counter(props, ref) { const [count, updateCount] = useState(0); - useAPI(ref, () => ({updateCount})); + useImperativeMethods(ref, () => ({updateCount})); return ; } Counter = forwardRef(Counter); @@ -195,7 +195,7 @@ describe('ReactHooks', () => { ReactNoop.yield('getInitialState'); return props.initialState; }); - useAPI(ref, () => ({updateCount})); + useImperativeMethods(ref, () => ({updateCount})); return ; } Counter = forwardRef(Counter); @@ -213,7 +213,7 @@ describe('ReactHooks', () => { function Counter(props, ref) { const [count, updateCount] = useState(0); const [label, updateLabel] = useState('Count'); - useAPI(ref, () => ({updateCount, updateLabel})); + useImperativeMethods(ref, () => ({updateCount, updateLabel})); return ; } Counter = forwardRef(Counter); @@ -232,7 +232,7 @@ describe('ReactHooks', () => { it('callbacks', () => { function Counter(props, ref) { const [count, updateCount] = useState(0); - useAPI(ref, () => ({updateCount})); + useImperativeMethods(ref, () => ({updateCount})); return ; } Counter = forwardRef(Counter); @@ -264,7 +264,7 @@ describe('ReactHooks', () => { it('does not fire callbacks more than once when rebasing', () => { function Counter(props, ref) { const [count, updateCount] = useState(0); - useAPI(ref, () => ({updateCount})); + useImperativeMethods(ref, () => ({updateCount})); return ; } Counter = forwardRef(Counter); @@ -507,7 +507,7 @@ describe('ReactHooks', () => { function Counter({row: newRow}, ref) { let [reducer, setReducer] = useState(() => reducerA); let [count, dispatch] = useReducer(reducer, 0); - useAPI(ref, () => ({dispatch})); + useImperativeMethods(ref, () => ({dispatch})); if (count < 20) { dispatch('increment'); // Swap reducers each time we increment @@ -568,7 +568,7 @@ describe('ReactHooks', () => { function Counter(props, ref) { const [count, dispatch] = useReducer(reducer, 0); - useAPI(ref, () => ({dispatch})); + useImperativeMethods(ref, () => ({dispatch})); return ; } Counter = forwardRef(Counter); @@ -609,7 +609,7 @@ describe('ReactHooks', () => { function Counter(props, ref) { const [count, dispatch] = useReducer(reducer, 0, initialAction); - useAPI(ref, () => ({dispatch})); + useImperativeMethods(ref, () => ({dispatch})); return ; } Counter = forwardRef(Counter); @@ -910,7 +910,6 @@ describe('ReactHooks', () => { expect(ReactNoop.flush()).toEqual(['Will set count to 1', 'Count: 2']); expect(ReactNoop.getChildren()).toEqual([span('Count: 2')]); - flushPassiveEffects(); expect(onInteractionScheduledWorkCompleted).toHaveBeenCalledTimes(1); expect(onWorkCanceled).toHaveBeenCalledTimes(0); }); @@ -1019,6 +1018,33 @@ describe('ReactHooks', () => { expect(ReactNoop.getChildren()).toEqual([]); }); + it('unmounts on deletion after skipped effect', () => { + function Counter(props) { + useEffect(() => { + ReactNoop.yield(`Did create [${props.count}]`); + return () => { + ReactNoop.yield(`Did destroy [${props.count}]`); + }; + }, []); + return ; + } + ReactNoop.render(); + expect(ReactNoop.flush()).toEqual(['Count: 0']); + expect(ReactNoop.getChildren()).toEqual([span('Count: 0')]); + flushPassiveEffects(); + expect(ReactNoop.clearYields()).toEqual(['Did create [0]']); + + ReactNoop.render(); + expect(ReactNoop.flush()).toEqual(['Count: 1']); + expect(ReactNoop.getChildren()).toEqual([span('Count: 1')]); + flushPassiveEffects(); + expect(ReactNoop.clearYields()).toEqual(null); + + ReactNoop.render(null); + expect(ReactNoop.flush()).toEqual(['Did destroy [0]']); + expect(ReactNoop.getChildren()).toEqual([]); + }); + it('skips effect if constructor has not changed', () => { function effect() { ReactNoop.yield(`Did mount`); diff --git a/packages/react/src/React.js b/packages/react/src/React.js index b2bbf2e8d2fa9..04f3592cedf90 100644 --- a/packages/react/src/React.js +++ b/packages/react/src/React.js @@ -29,10 +29,10 @@ import {lazy} from './ReactLazy'; import forwardRef from './forwardRef'; import memo from './memo'; import { - useAPI, useCallback, useContext, useEffect, + useImperativeMethods, useLayoutEffect, useMemo, useMutationEffect, @@ -89,10 +89,10 @@ if (enableStableConcurrentModeAPIs) { } if (enableHooks) { - React.useAPI = useAPI; React.useCallback = useCallback; React.useContext = useContext; React.useEffect = useEffect; + React.useImperativeMethods = useImperativeMethods; React.useLayoutEffect = useLayoutEffect; React.useMemo = useMemo; React.useMutationEffect = useMutationEffect; diff --git a/packages/react/src/ReactHooks.js b/packages/react/src/ReactHooks.js index 1cc8b79ac57f8..0e8f46dddbf67 100644 --- a/packages/react/src/ReactHooks.js +++ b/packages/react/src/ReactHooks.js @@ -110,11 +110,11 @@ export function useMemo( return dispatcher.useMemo(create, inputs); } -export function useAPI( +export function useImperativeMethods( ref: {current: T | null} | ((inst: T | null) => mixed) | null | void, create: () => T, inputs: Array | void | null, ): void { const dispatcher = resolveDispatcher(); - return dispatcher.useAPI(ref, create, inputs); + return dispatcher.useImperativeMethods(ref, create, inputs); } From 933b64710a48b2f86f71b1507901038025e79e3f Mon Sep 17 00:00:00 2001 From: Andrew Clark Date: Sat, 20 Oct 2018 15:01:40 -0700 Subject: [PATCH 16/19] Disable hook update callback (2nd arg to setState/dispatch) I put the feature behind a feature flag, along with a warning, so we can phase it out in www. --- .../react-reconciler/src/ReactFiberHooks.js | 16 +++++ .../src/__tests__/ReactHooks-test.internal.js | 65 ++++++++++++++----- packages/shared/ReactFeatureFlags.js | 1 + .../ReactFeatureFlags.native-fabric-fb.js | 1 + .../ReactFeatureFlags.native-fabric-oss.js | 1 + .../forks/ReactFeatureFlags.native-fb.js | 1 + .../forks/ReactFeatureFlags.native-oss.js | 1 + .../forks/ReactFeatureFlags.persistent.js | 1 + .../forks/ReactFeatureFlags.test-renderer.js | 1 + .../ReactFeatureFlags.test-renderer.www.js | 1 + .../shared/forks/ReactFeatureFlags.www.js | 1 + 11 files changed, 75 insertions(+), 15 deletions(-) diff --git a/packages/react-reconciler/src/ReactFiberHooks.js b/packages/react-reconciler/src/ReactFiberHooks.js index 35ad699d20253..bb7a0148ef09f 100644 --- a/packages/react-reconciler/src/ReactFiberHooks.js +++ b/packages/react-reconciler/src/ReactFiberHooks.js @@ -37,6 +37,8 @@ import { } from './ReactFiberScheduler'; import invariant from 'shared/invariant'; +import warningWithoutStack from 'shared/warningWithoutStack'; +import {enableDispatchCallback_DEPRECATED} from 'shared/ReactFeatureFlags'; type Update = { expirationTime: ExpirationTime, @@ -680,6 +682,20 @@ function dispatchAction( action: A, callback: void | null | (S => mixed), ) { + if (enableDispatchCallback_DEPRECATED) { + if (__DEV__) { + if (typeof callback === 'function') { + warningWithoutStack( + false, + 'Update callbacks (the second argument to dispatch/setState) are ' + + 'deprecated. Try useEffect instead.', + ); + } + } + } else { + callback = null; + } + invariant( numberOfReRenders < RE_RENDER_LIMIT, 'Too many re-renders. React limits the number of renders to prevent ' + diff --git a/packages/react-reconciler/src/__tests__/ReactHooks-test.internal.js b/packages/react-reconciler/src/__tests__/ReactHooks-test.internal.js index c6844256d6d7e..a505edf7ee17c 100644 --- a/packages/react-reconciler/src/__tests__/ReactHooks-test.internal.js +++ b/packages/react-reconciler/src/__tests__/ReactHooks-test.internal.js @@ -58,6 +58,7 @@ describe('ReactHooks', () => { ReactFeatureFlags = require('shared/ReactFeatureFlags'); ReactFeatureFlags.debugRenderPhaseSideEffectsForStrictMode = false; ReactFeatureFlags.enableHooks = true; + ReactFeatureFlags.enableDispatchCallback_DEPRECATED = true; ReactFeatureFlags.enableSchedulerTracing = true; React = require('react'); ReactNoop = require('react-noop-renderer'); @@ -241,18 +242,39 @@ describe('ReactHooks', () => { ReactNoop.flush(); expect(ReactNoop.getChildren()).toEqual([span('Count: 0')]); - counter.current.updateCount(7, count => { - ReactNoop.yield(`Did update count`); - }); + expect(() => { + counter.current.updateCount(7, count => { + ReactNoop.yield(`Did update count`); + }); + }).toWarnDev( + 'Warning: Update callbacks (the second argument to ' + + 'dispatch/setState) are deprecated. Try useEffect instead.', + {withoutStack: true}, + ); + expect(ReactNoop.flush()).toEqual(['Count: 7', 'Did update count']); // Update twice in the same batch - counter.current.updateCount(1, () => { - ReactNoop.yield(`Did update count (first callback)`); - }); - counter.current.updateCount(2, () => { - ReactNoop.yield(`Did update count (second callback)`); - }); + expect(() => { + counter.current.updateCount(1, () => { + ReactNoop.yield(`Did update count (first callback)`); + }); + }).toWarnDev( + 'Warning: Update callbacks (the second argument to ' + + 'dispatch/setState) are deprecated. Try useEffect instead.', + {withoutStack: true}, + ); + + expect(() => { + counter.current.updateCount(2, () => { + ReactNoop.yield(`Did update count (second callback)`); + }); + }).toWarnDev( + 'Warning: Update callbacks (the second argument to ' + + 'dispatch/setState) are deprecated. Try useEffect instead.', + {withoutStack: true}, + ); + expect(ReactNoop.flush()).toEqual([ // Component only renders once 'Count: 2', @@ -273,14 +295,27 @@ describe('ReactHooks', () => { ReactNoop.flush(); expect(ReactNoop.getChildren()).toEqual([span('Count: 0')]); - counter.current.updateCount(1, count => { - ReactNoop.yield(`Did update count (low pri)`); - }); - ReactNoop.flushSync(() => { - counter.current.updateCount(2, count => { - ReactNoop.yield(`Did update count (high pri)`); + expect(() => { + counter.current.updateCount(1, count => { + ReactNoop.yield(`Did update count (low pri)`); }); + }).toWarnDev( + 'Warning: Update callbacks (the second argument to ' + + 'dispatch/setState) are deprecated. Try useEffect instead.', + {withoutStack: true}, + ); + ReactNoop.flushSync(() => { + expect(() => { + counter.current.updateCount(2, count => { + ReactNoop.yield(`Did update count (high pri)`); + }); + }).toWarnDev( + 'Warning: Update callbacks (the second argument to ' + + 'dispatch/setState) are deprecated. Try useEffect instead.', + {withoutStack: true}, + ); }); + expect(ReactNoop.clearYields()).toEqual([ 'Count: 2', 'Did update count (high pri)', diff --git a/packages/shared/ReactFeatureFlags.js b/packages/shared/ReactFeatureFlags.js index 028b5a8e32d96..e41d9c89961ad 100644 --- a/packages/shared/ReactFeatureFlags.js +++ b/packages/shared/ReactFeatureFlags.js @@ -10,6 +10,7 @@ export const enableUserTimingAPI = __DEV__; export const enableHooks = false; +export const enableDispatchCallback_DEPRECATED = false; // Helps identify side effects in begin-phase lifecycle hooks and setState reducers: export const debugRenderPhaseSideEffects = false; diff --git a/packages/shared/forks/ReactFeatureFlags.native-fabric-fb.js b/packages/shared/forks/ReactFeatureFlags.native-fabric-fb.js index 971f6bcbd3f9b..7449853c3c769 100644 --- a/packages/shared/forks/ReactFeatureFlags.native-fabric-fb.js +++ b/packages/shared/forks/ReactFeatureFlags.native-fabric-fb.js @@ -16,6 +16,7 @@ export const debugRenderPhaseSideEffects = false; export const debugRenderPhaseSideEffectsForStrictMode = false; export const enableUserTimingAPI = __DEV__; export const enableHooks = false; +export const enableDispatchCallback_DEPRECATED = false; export const warnAboutDeprecatedLifecycles = false; export const replayFailedUnitOfWorkWithInvokeGuardedCallback = __DEV__; export const enableProfilerTimer = __PROFILE__; diff --git a/packages/shared/forks/ReactFeatureFlags.native-fabric-oss.js b/packages/shared/forks/ReactFeatureFlags.native-fabric-oss.js index 3f647674e34d8..05a5d9fb1579e 100644 --- a/packages/shared/forks/ReactFeatureFlags.native-fabric-oss.js +++ b/packages/shared/forks/ReactFeatureFlags.native-fabric-oss.js @@ -16,6 +16,7 @@ export const debugRenderPhaseSideEffects = false; export const debugRenderPhaseSideEffectsForStrictMode = false; export const enableUserTimingAPI = __DEV__; export const enableHooks = false; +export const enableDispatchCallback_DEPRECATED = false; export const warnAboutDeprecatedLifecycles = false; export const replayFailedUnitOfWorkWithInvokeGuardedCallback = __DEV__; export const enableProfilerTimer = __PROFILE__; diff --git a/packages/shared/forks/ReactFeatureFlags.native-fb.js b/packages/shared/forks/ReactFeatureFlags.native-fb.js index 6e9d290359c60..d9dcb280b9aff 100644 --- a/packages/shared/forks/ReactFeatureFlags.native-fb.js +++ b/packages/shared/forks/ReactFeatureFlags.native-fb.js @@ -15,6 +15,7 @@ import typeof * as FeatureFlagsShimType from './ReactFeatureFlags.native-fb'; // Re-export dynamic flags from the fbsource version. export const { enableHooks, + enableDispatchCallback_DEPRECATED, debugRenderPhaseSideEffects, debugRenderPhaseSideEffectsForStrictMode, warnAboutDeprecatedLifecycles, diff --git a/packages/shared/forks/ReactFeatureFlags.native-oss.js b/packages/shared/forks/ReactFeatureFlags.native-oss.js index 96f7607431a1b..88026e476710c 100644 --- a/packages/shared/forks/ReactFeatureFlags.native-oss.js +++ b/packages/shared/forks/ReactFeatureFlags.native-oss.js @@ -15,6 +15,7 @@ import typeof * as FeatureFlagsShimType from './ReactFeatureFlags.native-oss'; export const debugRenderPhaseSideEffects = false; export const debugRenderPhaseSideEffectsForStrictMode = false; export const enableHooks = false; +export const enableDispatchCallback_DEPRECATED = false; export const enableUserTimingAPI = __DEV__; export const replayFailedUnitOfWorkWithInvokeGuardedCallback = __DEV__; export const warnAboutDeprecatedLifecycles = false; diff --git a/packages/shared/forks/ReactFeatureFlags.persistent.js b/packages/shared/forks/ReactFeatureFlags.persistent.js index bcb561d62b21e..a9903783147c2 100644 --- a/packages/shared/forks/ReactFeatureFlags.persistent.js +++ b/packages/shared/forks/ReactFeatureFlags.persistent.js @@ -16,6 +16,7 @@ export const debugRenderPhaseSideEffects = false; export const debugRenderPhaseSideEffectsForStrictMode = false; export const enableUserTimingAPI = __DEV__; export const enableHooks = false; +export const enableDispatchCallback_DEPRECATED = false; export const warnAboutDeprecatedLifecycles = false; export const replayFailedUnitOfWorkWithInvokeGuardedCallback = __DEV__; export const enableProfilerTimer = __PROFILE__; diff --git a/packages/shared/forks/ReactFeatureFlags.test-renderer.js b/packages/shared/forks/ReactFeatureFlags.test-renderer.js index 9431ff5623d7e..93e5580665702 100644 --- a/packages/shared/forks/ReactFeatureFlags.test-renderer.js +++ b/packages/shared/forks/ReactFeatureFlags.test-renderer.js @@ -16,6 +16,7 @@ export const debugRenderPhaseSideEffects = false; export const debugRenderPhaseSideEffectsForStrictMode = false; export const enableUserTimingAPI = __DEV__; export const enableHooks = false; +export const enableDispatchCallback_DEPRECATED = false; export const warnAboutDeprecatedLifecycles = false; export const replayFailedUnitOfWorkWithInvokeGuardedCallback = false; export const enableProfilerTimer = false; diff --git a/packages/shared/forks/ReactFeatureFlags.test-renderer.www.js b/packages/shared/forks/ReactFeatureFlags.test-renderer.www.js index fb1c9bd817b9e..d1e14b3531cb8 100644 --- a/packages/shared/forks/ReactFeatureFlags.test-renderer.www.js +++ b/packages/shared/forks/ReactFeatureFlags.test-renderer.www.js @@ -16,6 +16,7 @@ export const debugRenderPhaseSideEffects = false; export const debugRenderPhaseSideEffectsForStrictMode = false; export const enableUserTimingAPI = __DEV__; export const enableHooks = false; +export const enableDispatchCallback_DEPRECATED = false; export const warnAboutDeprecatedLifecycles = false; export const replayFailedUnitOfWorkWithInvokeGuardedCallback = false; export const enableProfilerTimer = false; diff --git a/packages/shared/forks/ReactFeatureFlags.www.js b/packages/shared/forks/ReactFeatureFlags.www.js index bf0057ef3620c..7bc4628a62c5f 100644 --- a/packages/shared/forks/ReactFeatureFlags.www.js +++ b/packages/shared/forks/ReactFeatureFlags.www.js @@ -18,6 +18,7 @@ export const { replayFailedUnitOfWorkWithInvokeGuardedCallback, warnAboutDeprecatedLifecycles, disableInputAttributeSyncing, + enableDispatchCallback_DEPRECATED, } = require('ReactFeatureFlags'); // The rest of the flags are static for better dead code elimination. From acb48996373ee0cbf1b2462972cd7644862d770e Mon Sep 17 00:00:00 2001 From: Andrew Clark Date: Tue, 23 Oct 2018 00:41:22 -0700 Subject: [PATCH 17/19] Clear effect tags from a fiber that suspends in non-concurrent mode Even though we commit the fiber in an incomplete state, we shouldn't fire any lifecycles or effects. We already did this for classes, but now with useEffect, the same is needed for other types of work, too. --- .../src/ReactFiberUnwindWork.js | 11 +++-- .../__tests__/ReactSuspense-test.internal.js | 46 +++++++++++++++++++ 2 files changed, 52 insertions(+), 5 deletions(-) diff --git a/packages/react-reconciler/src/ReactFiberUnwindWork.js b/packages/react-reconciler/src/ReactFiberUnwindWork.js index 40c814a37d7e1..26ab5bea0f11a 100644 --- a/packages/react-reconciler/src/ReactFiberUnwindWork.js +++ b/packages/react-reconciler/src/ReactFiberUnwindWork.js @@ -248,15 +248,16 @@ function throwException( ); sourceFiber.effectTag &= ~Incomplete; + // We're going to commit this fiber even though it didn't complete. + // But we shouldn't call any lifecycle methods or callbacks. Remove + // all lifecycle effect tags. + sourceFiber.effectTag &= ~LifecycleEffectMask; + if (sourceFiber.tag === ClassComponent) { - // We're going to commit this fiber even though it didn't complete. - // But we shouldn't call any lifecycle methods or callbacks. Remove - // all lifecycle effect tags. - sourceFiber.effectTag &= ~LifecycleEffectMask; const current = sourceFiber.alternate; if (current === null) { // This is a new mount. Change the tag so it's not mistaken for a - // completed component. For example, we should not call + // completed class component. For example, we should not call // componentWillUnmount if it is deleted. sourceFiber.tag = IncompleteClassComponent; } diff --git a/packages/react-reconciler/src/__tests__/ReactSuspense-test.internal.js b/packages/react-reconciler/src/__tests__/ReactSuspense-test.internal.js index 693c5d301e393..c0803a610d3c7 100644 --- a/packages/react-reconciler/src/__tests__/ReactSuspense-test.internal.js +++ b/packages/react-reconciler/src/__tests__/ReactSuspense-test.internal.js @@ -17,6 +17,7 @@ describe('ReactSuspense', () => { ReactFeatureFlags = require('shared/ReactFeatureFlags'); ReactFeatureFlags.debugRenderPhaseSideEffectsForStrictMode = false; ReactFeatureFlags.replayFailedUnitOfWorkWithInvokeGuardedCallback = false; + ReactFeatureFlags.enableHooks = true; React = require('react'); ReactTestRenderer = require('react-test-renderer'); // JestReact = require('jest-react'); @@ -526,6 +527,51 @@ describe('ReactSuspense', () => { expect(root).toMatchRenderedOutput('B'); }); + it('suspends in a component that also contains useEffect', () => { + const {useLayoutEffect} = React; + + function AsyncTextWithEffect(props) { + const text = props.text; + + useLayoutEffect( + () => { + ReactTestRenderer.unstable_yield('Did commit: ' + text); + }, + [text], + ); + + try { + TextResource.read([props.text, props.ms]); + ReactTestRenderer.unstable_yield(text); + return text; + } catch (promise) { + if (typeof promise.then === 'function') { + ReactTestRenderer.unstable_yield(`Suspend! [${text}]`); + } else { + ReactTestRenderer.unstable_yield(`Error! [${text}]`); + } + throw promise; + } + } + + function App({text}) { + return ( + }> + + + ); + } + + ReactTestRenderer.create(); + expect(ReactTestRenderer).toHaveYielded(['Suspend! [A]', 'Loading...']); + jest.advanceTimersByTime(500); + expect(ReactTestRenderer).toHaveYielded([ + 'Promise resolved [A]', + 'A', + 'Did commit: A', + ]); + }); + it('retries when an update is scheduled on a timed out tree', () => { let instance; class Stateful extends React.Component { From ddbfe2ed50c7a3476ceff20f5924011ac1ad6428 Mon Sep 17 00:00:00 2001 From: Caleb Meredith Date: Tue, 23 Oct 2018 12:35:56 -0700 Subject: [PATCH 18/19] Add ESLint rule for React Hooks --- packages/eslint-plugin-react-hooks/README.md | 48 ++ .../__tests__/ESLintRulesOfHooks-test.js | 632 ++++++++++++++++++ packages/eslint-plugin-react-hooks/index.js | 10 + .../eslint-plugin-react-hooks/npm/index.js | 9 + .../eslint-plugin-react-hooks/package.json | 29 + .../src/RulesOfHooks.js | 545 +++++++++++++++ .../eslint-plugin-react-hooks/src/index.js | 14 + scripts/rollup/bundles.js | 15 + 8 files changed, 1302 insertions(+) create mode 100644 packages/eslint-plugin-react-hooks/README.md create mode 100644 packages/eslint-plugin-react-hooks/__tests__/ESLintRulesOfHooks-test.js create mode 100644 packages/eslint-plugin-react-hooks/index.js create mode 100644 packages/eslint-plugin-react-hooks/npm/index.js create mode 100644 packages/eslint-plugin-react-hooks/package.json create mode 100644 packages/eslint-plugin-react-hooks/src/RulesOfHooks.js create mode 100644 packages/eslint-plugin-react-hooks/src/index.js diff --git a/packages/eslint-plugin-react-hooks/README.md b/packages/eslint-plugin-react-hooks/README.md new file mode 100644 index 0000000000000..156f09718b35d --- /dev/null +++ b/packages/eslint-plugin-react-hooks/README.md @@ -0,0 +1,48 @@ +# `eslint-plugin-react-hooks` + +This ESLint plugin enforces the [Rules of Hooks](https://reactjs.org/docs/hooks-rules.html). + +It is a part of the [Hooks proposal](https://reactjs.org/docs/hooks-intro.html) for React. + +## Experimental Status + +This is an experimental release and is intended to be used for testing the Hooks proposal with React 16.7 alpha. The exact heuristics it uses may be adjusted. + +The [Rules of Hooks](https://reactjs.org/docs/hooks-rules.html) documentation contains a link to the technical RFC. Please leave a comment on the RFC if you have concerns or ideas about how this plugin should work. + +## Installation + +**Note: If you're using Create React App, please wait for a corresponding experimental release of `react-scripts` that includes this rule instead of adding it directly.** + +Assuming you already have ESLint installed, run: + +```sh +# npm +npm install eslint-plugin-react-hooks@next --save-dev + +# yarn +yarn add eslint-plugin-react-hooks@next --dev +``` + +Then add it to your ESLint configuration: + +```js +{ + "plugins": [ + // ... + "react-hooks" + ], + "rules": { + // ... + "react-hooks/rules-of-hooks": "error" + } +} +``` + +## Valid and Invalid Examples + +Please refer to the [Rules of Hooks](https://reactjs.org/docs/hooks-rules.html) documentation and the [Hooks FAQ](https://reactjs.org/docs/hooks-faq.html#what-exactly-do-the-lint-rules-enforce) to learn more about this rule. + +## License + +MIT diff --git a/packages/eslint-plugin-react-hooks/__tests__/ESLintRulesOfHooks-test.js b/packages/eslint-plugin-react-hooks/__tests__/ESLintRulesOfHooks-test.js new file mode 100644 index 0000000000000..b8807db3c56f4 --- /dev/null +++ b/packages/eslint-plugin-react-hooks/__tests__/ESLintRulesOfHooks-test.js @@ -0,0 +1,632 @@ +/** + * Copyright (c) Facebook, Inc. and its affiliates. + * + * This source code is licensed under the MIT license found in the + * LICENSE file in the root directory of this source tree. + */ + +'use strict'; + +const ESLintTester = require('eslint').RuleTester; +const ReactHooksESLintPlugin = require('eslint-plugin-react-hooks'); +const ReactHooksESLintRule = ReactHooksESLintPlugin.rules['rules-of-hooks']; + +ESLintTester.setDefaultConfig({ + parser: 'babel-eslint', + parserOptions: { + ecmaVersion: 6, + sourceType: 'module', + }, +}); + +const eslintTester = new ESLintTester(); +eslintTester.run('react-hooks', ReactHooksESLintRule, { + valid: [ + ` + // Valid because components can use hooks. + function ComponentWithHook() { + useHook(); + } + `, + ` + // Valid because components can use hooks. + function createComponentWithHook() { + return function ComponentWithHook() { + useHook(); + }; + } + `, + ` + // Valid because hooks can use hooks. + function useHookWithHook() { + useHook(); + } + `, + ` + // Valid because hooks can use hooks. + function createHook() { + return function useHookWithHook() { + useHook(); + } + } + `, + ` + // Valid because components can call functions. + function ComponentWithNormalFunction() { + doSomething(); + } + `, + ` + // Valid because functions can call functions. + function normalFunctionWithNormalFunction() { + doSomething(); + } + `, + ` + // Valid because functions can call functions. + function normalFunctionWithConditionalFunction() { + if (cond) { + doSomething(); + } + } + `, + ` + // Valid because functions can call functions. + function functionThatStartsWithUseButIsntAHook() { + if (cond) { + userFetch(); + } + } + `, + ` + // Valid although unconditional return doesn't make sense and would fail other rules. + // We could make it invalid but it doesn't matter. + function useUnreachable() { + return; + useHook(); + } + `, + ` + // Valid because hooks can call hooks. + function useHook() { useState(); } + const whatever = function useHook() { useState(); }; + const useHook1 = () => { useState(); }; + let useHook2 = () => useState(); + useHook2 = () => { useState(); }; + ({useHook: () => { useState(); }}); + ({useHook() { useState(); }}); + const {useHook = () => { useState(); }} = {}; + ({useHook = () => { useState(); }} = {}); + `, + ` + // Valid because hooks can call hooks. + function useHook() { + useHook1(); + useHook2(); + } + `, + ` + // Valid because hooks can call hooks. + function createHook() { + return function useHook() { + useHook1(); + useHook2(); + }; + } + `, + ` + // Valid because hooks can call hooks. + function useHook() { + useState() && a; + } + `, + ` + // Valid because hooks can call hooks. + function useHook() { + return useHook1() + useHook2(); + } + `, + ` + // Valid because hooks can call hooks. + function useHook() { + return useHook1(useHook2()); + } + `, + ` + // Valid because classes can call functions. + // We don't consider these to be hooks. + class C { + m() { + this.useHook(); + super.useHook(); + } + } + `, + ` + // Currently valid. + // We *could* make this invalid if we want, but it creates false positives + // (see the FooStore case). + class C { + m() { + This.useHook(); + Super.useHook(); + } + } + `, + ` + // Valid although we *could* consider these invalid. + // But it doesn't bring much benefit since it's an immediate runtime error anyway. + // So might as well allow it. + Hook.use(); + Hook._use(); + Hook.useState(); + Hook._useState(); + Hook.use42(); + Hook.useHook(); + Hook.use_hook(); + `, + ` + // Valid -- this is a regression test. + jest.useFakeTimers(); + beforeEach(() => { + jest.useRealTimers(); + }) + `, + ` + // Valid because that's a false positive we've seen quite a bit. + // This is a regression test. + class Foo extends Component { + render() { + if (cond) { + FooStore.useFeatureFlag(); + } + } + } + `, + ` + // Currently valid because we found this to be a common pattern + // for feature flag checks in existing components. + // We *could* make it invalid but that produces quite a few false positives. + // Why does it make sense to ignore it? Firstly, because using + // hooks in a class would cause a runtime error anyway. + // But why don't we care about the same kind of false positive in a functional + // component? Because even if it was a false positive, it would be confusing + // anyway. So it might make sense to rename a feature flag check in that case. + class ClassComponentWithFeatureFlag extends React.Component { + render() { + if (foo) { + useFeatureFlag(); + } + } + } + `, + ` + // Currently valid because we don't check for hooks in classes. + // See ClassComponentWithFeatureFlag for rationale. + // We *could* make it invalid if we don't regress that false positive. + class ClassComponentWithHook extends React.Component { + render() { + React.useState(); + } + } + `, + ` + // Currently valid. + // These are variations capturing the current heuristic-- + // we only allow hooks in PascalCase, useFoo functions, + // or classes (due to common false positives and because they error anyway). + // We *could* make some of these invalid. + // They probably don't matter much. + (class {useHook = () => { useState(); }}); + (class {useHook() { useState(); }}); + (class {h = () => { useState(); }}); + (class {i() { useState(); }}); + `, + ` + // Currently valid although we *could* consider these invalid. + // It doesn't make a lot of difference because it would crash early. + use(); + _use(); + useState(); + _useState(); + use42(); + useHook(); + use_hook(); + React.useState(); + `, + ` + // Regression test for the popular "history" library + const {createHistory, useBasename} = require('history-2.1.2'); + const browserHistory = useBasename(createHistory)({ + basename: '/', + }); + `, + ` + // Regression test for some internal code. + // This shows how the "callback rule" is more relaxed, + // and doesn't kick in unless we're confident we're in + // a component or a hook. + function makeListener(instance) { + each(pixelsWithInferredEvents, pixel => { + if (useExtendedSelector(pixel.id) && extendedButton) { + foo(); + } + }); + } + `, + ` + // Regression test for incorrectly flagged valid code. + function RegressionTest() { + const foo = cond ? a : b; + useState(); + } + `, + ], + invalid: [ + { + code: ` + // Invalid because it's dangerous and might not warn otherwise. + // This *must* be invalid. + function ComponentWithConditionalHook() { + if (cond) { + useConditionalHook(); + } + } + `, + errors: [conditionalError('useConditionalHook')], + }, + { + code: ` + // Invalid because it's dangerous and might not warn otherwise. + // This *must* be invalid. + function createComponent() { + return function ComponentWithConditionalHook() { + if (cond) { + useConditionalHook(); + } + } + } + `, + errors: [conditionalError('useConditionalHook')], + }, + { + code: ` + // Invalid because it's dangerous and might not warn otherwise. + // This *must* be invalid. + function useHookWithConditionalHook() { + if (cond) { + useConditionalHook(); + } + } + `, + errors: [conditionalError('useConditionalHook')], + }, + { + code: ` + // Invalid because it's dangerous and might not warn otherwise. + // This *must* be invalid. + function createHook() { + return function useHookWithConditionalHook() { + if (cond) { + useConditionalHook(); + } + } + } + `, + errors: [conditionalError('useConditionalHook')], + }, + { + code: ` + // Invalid because it's dangerous and might not warn otherwise. + // This *must* be invalid. + function ComponentWithTernaryHook() { + cond ? useTernaryHook() : null; + } + `, + errors: [conditionalError('useTernaryHook')], + }, + { + code: ` + // Invalid because it's a common misunderstanding. + // We *could* make it valid but the runtime error could be confusing. + function ComponentWithHookInsideCallback() { + useEffect(() => { + useHookInsideCallback(); + }); + } + `, + errors: [genericError('useHookInsideCallback')], + }, + { + code: ` + // Invalid because it's a common misunderstanding. + // We *could* make it valid but the runtime error could be confusing. + function createComponent() { + return function ComponentWithHookInsideCallback() { + useEffect(() => { + useHookInsideCallback(); + }); + } + } + `, + errors: [genericError('useHookInsideCallback')], + }, + { + code: ` + // Invalid because it's a common misunderstanding. + // We *could* make it valid but the runtime error could be confusing. + function ComponentWithHookInsideCallback() { + function handleClick() { + useState(); + } + } + `, + errors: [functionError('useState', 'handleClick')], + }, + { + code: ` + // Invalid because it's a common misunderstanding. + // We *could* make it valid but the runtime error could be confusing. + function createComponent() { + return function ComponentWithHookInsideCallback() { + function handleClick() { + useState(); + } + } + } + `, + errors: [functionError('useState', 'handleClick')], + }, + { + code: ` + // Invalid because it's dangerous and might not warn otherwise. + // This *must* be invalid. + function ComponentWithHookInsideLoop() { + while (cond) { + useHookInsideLoop(); + } + } + `, + errors: [loopError('useHookInsideLoop')], + }, + { + code: ` + // Invalid because it's dangerous and might not warn otherwise. + // This *must* be invalid. + function renderItem() { + useState(); + } + + function List(props) { + return props.items.map(renderItem); + } + `, + errors: [functionError('useState', 'renderItem')], + }, + { + code: ` + // Currently invalid because it violates the convention and removes the "taint" + // from a hook. We *could* make it valid to avoid some false positives but let's + // ensure that we don't break the "renderItem" and "normalFunctionWithConditionalHook" + // cases which must remain invalid. + function normalFunctionWithHook() { + useHookInsideNormalFunction(); + } + `, + errors: [ + functionError('useHookInsideNormalFunction', 'normalFunctionWithHook'), + ], + }, + { + code: ` + // Invalid because it's dangerous and might not warn otherwise. + // This *must* be invalid. + function normalFunctionWithConditionalHook() { + if (cond) { + useHookInsideNormalFunction(); + } + } + `, + errors: [ + functionError( + 'useHookInsideNormalFunction', + 'normalFunctionWithConditionalHook' + ), + ], + }, + { + code: ` + // Invalid because it's dangerous and might not warn otherwise. + // This *must* be invalid. + function useHookInLoops() { + while (a) { + useHook1(); + if (b) return; + useHook2(); + } + while (c) { + useHook3(); + if (d) return; + useHook4(); + } + } + `, + errors: [ + loopError('useHook1'), + loopError('useHook2'), + loopError('useHook3'), + loopError('useHook4'), + ], + }, + { + code: ` + // Invalid because it's dangerous and might not warn otherwise. + // This *must* be invalid. + function useHookInLoops() { + while (a) { + useHook1(); + if (b) continue; + useHook2(); + } + } + `, + errors: [ + loopError('useHook1'), + + // NOTE: Small imprecision in error reporting due to caching means we + // have a conditional error here instead of a loop error. However, + // we will always get an error so this is acceptable. + conditionalError('useHook2', true), + ], + }, + { + code: ` + // Invalid because it's dangerous and might not warn otherwise. + // This *must* be invalid. + function useLabeledBlock() { + label: { + if (a) break label; + useHook(); + } + } + `, + errors: [conditionalError('useHook')], + }, + { + code: ` + // Currently invalid. + // These are variations capturing the current heuristic-- + // we only allow hooks in PascalCase or useFoo functions. + // We *could* make some of these valid. But before doing it, + // consider specific cases documented above that contain reasoning. + function a() { useState(); } + const whatever = function b() { useState(); }; + const c = () => { useState(); }; + let d = () => useState(); + e = () => { useState(); }; + ({f: () => { useState(); }}); + ({g() { useState(); }}); + const {j = () => { useState(); }} = {}; + ({k = () => { useState(); }} = {}); + `, + errors: [ + functionError('useState', 'a'), + functionError('useState', 'b'), + functionError('useState', 'c'), + functionError('useState', 'd'), + functionError('useState', 'e'), + functionError('useState', 'f'), + functionError('useState', 'g'), + functionError('useState', 'j'), + functionError('useState', 'k'), + ], + }, + { + code: ` + // Invalid because it's dangerous and might not warn otherwise. + // This *must* be invalid. + function useHook() { + if (a) return; + useState(); + } + `, + errors: [conditionalError('useState', true)], + }, + { + code: ` + // Invalid because it's dangerous and might not warn otherwise. + // This *must* be invalid. + function useHook() { + if (a) return; + if (b) { + console.log('true'); + } else { + console.log('false'); + } + useState(); + } + `, + errors: [conditionalError('useState', true)], + }, + { + code: ` + // Invalid because it's dangerous and might not warn otherwise. + // This *must* be invalid. + function useHook() { + if (b) { + console.log('true'); + } else { + console.log('false'); + } + if (a) return; + useState(); + } + `, + errors: [conditionalError('useState', true)], + }, + { + code: ` + // Invalid because it's dangerous and might not warn otherwise. + // This *must* be invalid. + function useHook() { + a && useHook1(); + b && useHook2(); + } + `, + errors: [conditionalError('useHook1'), conditionalError('useHook2')], + }, + { + code: ` + // Invalid because it's dangerous and might not warn otherwise. + // This *must* be invalid. + function useHook() { + try { + f(); + useState(); + } catch {} + } + `, + errors: [ + // NOTE: This is an error since `f()` could possibly throw. + conditionalError('useState'), + ], + }, + ], +}); + +function conditionalError(hook, hasPreviousFinalizer = false) { + return { + message: + `React Hook "${hook}" is called conditionally. React Hooks must be ` + + 'called in the exact same order in every component render.' + + (hasPreviousFinalizer + ? ' Did you accidentally call a React Hook after an early return?' + : ''), + }; +} + +function loopError(hook) { + return { + message: + `React Hook "${hook}" may be executed more than once. Possibly ` + + 'because it is called in a loop. React Hooks must be called in the ' + + 'exact same order in every component render.', + }; +} + +function functionError(hook, fn) { + return { + message: + `React Hook "${hook}" is called in function "${fn}" which is neither ` + + 'a React function component or a custom React Hook function.', + }; +} + +function genericError(hook) { + return { + message: + `React Hook "${hook}" cannot be called inside a callback. React Hooks ` + + 'must be called in a React function component or a custom React ' + + 'Hook function.', + }; +} diff --git a/packages/eslint-plugin-react-hooks/index.js b/packages/eslint-plugin-react-hooks/index.js new file mode 100644 index 0000000000000..7ab3284345f0b --- /dev/null +++ b/packages/eslint-plugin-react-hooks/index.js @@ -0,0 +1,10 @@ +/** + * Copyright (c) Facebook, Inc. and its affiliates. + * + * This source code is licensed under the MIT license found in the + * LICENSE file in the root directory of this source tree. + */ + +'use strict'; + +module.exports = require('./src/index'); diff --git a/packages/eslint-plugin-react-hooks/npm/index.js b/packages/eslint-plugin-react-hooks/npm/index.js new file mode 100644 index 0000000000000..0e91baf6cd189 --- /dev/null +++ b/packages/eslint-plugin-react-hooks/npm/index.js @@ -0,0 +1,9 @@ +'use strict'; + +// TODO: this doesn't make sense for an ESLint rule. +// We need to fix our build process to not create bundles for "raw" packages like this. +if (process.env.NODE_ENV === 'production') { + module.exports = require('./cjs/eslint-plugin-react-hooks.production.min.js'); +} else { + module.exports = require('./cjs/eslint-plugin-react-hooks.development.js'); +} diff --git a/packages/eslint-plugin-react-hooks/package.json b/packages/eslint-plugin-react-hooks/package.json new file mode 100644 index 0000000000000..f779252a27653 --- /dev/null +++ b/packages/eslint-plugin-react-hooks/package.json @@ -0,0 +1,29 @@ +{ + "name": "eslint-plugin-react-hooks", + "description": "ESLint rules for React Hooks", + "version": "0.0.0", + "repository": "facebook/react", + "files": [ + "LICENSE", + "README.md", + "index.js", + "cjs" + ], + "keywords": [ + "eslint", + "eslint-plugin", + "eslintplugin", + "react" + ], + "license": "MIT", + "bugs": { + "url": "https://github.com/facebook/react/issues" + }, + "engines": { + "node": ">=6" + }, + "homepage": "https://reactjs.org/", + "peerDependencies": { + "eslint": "^3.0.0 || ^4.0.0 || ^5.0.0" + } +} diff --git a/packages/eslint-plugin-react-hooks/src/RulesOfHooks.js b/packages/eslint-plugin-react-hooks/src/RulesOfHooks.js new file mode 100644 index 0000000000000..e77afbf3c6822 --- /dev/null +++ b/packages/eslint-plugin-react-hooks/src/RulesOfHooks.js @@ -0,0 +1,545 @@ +/** + * Copyright (c) Facebook, Inc. and its affiliates. + * + * This source code is licensed under the MIT license found in the + * LICENSE file in the root directory of this source tree. + */ + +/* eslint-disable no-for-of-loops/no-for-of-loops */ + +'use strict'; + +/** + * Catch all identifiers that begin with "use" followed by an uppercase Latin + * character to exclude identifiers like "user". + */ + +function isHookName(s) { + return /^use[A-Z0-9].*$/.test(s); +} + +/** + * We consider hooks to be a hook name identifier or a member expression + * containing a hook name. + */ + +function isHook(node) { + if (node.type === 'Identifier') { + return isHookName(node.name); + } else if ( + node.type === 'MemberExpression' && + !node.computed && + isHook(node.property) + ) { + // Only consider React.useFoo() to be namespace hooks for now to avoid false positives. + // We can expand this check later. + const obj = node.object; + return obj.type === 'Identifier' && obj.name === 'React'; + } else { + return false; + } +} + +/** + * Checks if the node is a React component name. React component names must + * always start with a non-lowercase letter. So `MyComponent` or `_MyComponent` + * are valid component names for instance. + */ + +function isComponentName(node) { + if (node.type === 'Identifier') { + return !/^[a-z]/.test(node.name); + } else { + return false; + } +} + +function isInsideComponentOrHook(node) { + while (node) { + const functionName = getFunctionName(node); + if (functionName) { + if (isComponentName(functionName) || isHook(functionName)) { + return true; + } + } + node = node.parent; + } + return false; +} + +export default { + create(context) { + const codePathReactHooksMapStack = []; + const codePathSegmentStack = []; + return { + // Maintain code segment path stack as we traverse. + onCodePathSegmentStart: segment => codePathSegmentStack.push(segment), + onCodePathSegmentEnd: () => codePathSegmentStack.pop(), + + // Maintain code path stack as we traverse. + onCodePathStart: () => codePathReactHooksMapStack.push(new Map()), + + // Process our code path. + // + // Everything is ok if all React Hooks are both reachable from the initial + // segment and reachable from every final segment. + onCodePathEnd(codePath, codePathNode) { + const reactHooksMap = codePathReactHooksMapStack.pop(); + if (reactHooksMap.size === 0) { + return; + } + + // All of the segments which are cyclic are recorded in this set. + const cyclic = new Set(); + + /** + * Count the number of code paths from the start of the function to this + * segment. For example: + * + * ```js + * function MyComponent() { + * if (condition) { + * // Segment 1 + * } else { + * // Segment 2 + * } + * // Segment 3 + * } + * ``` + * + * Segments 1 and 2 have one path to the beginning of `MyComponent` and + * segment 3 has two paths to the beginning of `MyComponent` since we + * could have either taken the path of segment 1 or segment 2. + * + * Populates `cyclic` with cyclic segments. + */ + + function countPathsFromStart(segment) { + const {cache} = countPathsFromStart; + let paths = cache.get(segment.id); + + // If `paths` is null then we've found a cycle! Add it to `cyclic` and + // any other segments which are a part of this cycle. + if (paths === null) { + if (cyclic.has(segment.id)) { + return 0; + } else { + cyclic.add(segment.id); + for (const prevSegment of segment.prevSegments) { + countPathsFromStart(prevSegment); + } + return 0; + } + } + + // We have a cached `paths`. Return it. + if (paths !== undefined) { + return paths; + } + + // Compute `paths` and cache it. Guarding against cycles. + cache.set(segment.id, null); + if (segment.prevSegments.length === 0) { + paths = 1; + } else { + paths = 0; + for (const prevSegment of segment.prevSegments) { + paths += countPathsFromStart(prevSegment); + } + } + cache.set(segment.id, paths); + + return paths; + } + + /** + * Count the number of code paths from this segment to the end of the + * function. For example: + * + * ```js + * function MyComponent() { + * // Segment 1 + * if (condition) { + * // Segment 2 + * } else { + * // Segment 3 + * } + * } + * ``` + * + * Segments 2 and 3 have one path to the end of `MyComponent` and + * segment 1 has two paths to the end of `MyComponent` since we could + * either take the path of segment 1 or segment 2. + * + * Populates `cyclic` with cyclic segments. + */ + + function countPathsToEnd(segment) { + const {cache} = countPathsToEnd; + let paths = cache.get(segment.id); + + // If `paths` is null then we've found a cycle! Add it to `cyclic` and + // any other segments which are a part of this cycle. + if (paths === null) { + if (cyclic.has(segment.id)) { + return 0; + } else { + cyclic.add(segment.id); + for (const nextSegment of segment.nextSegments) { + countPathsToEnd(nextSegment); + } + return 0; + } + } + + // We have a cached `paths`. Return it. + if (paths !== undefined) { + return paths; + } + + // Compute `paths` and cache it. Guarding against cycles. + cache.set(segment.id, null); + if (segment.nextSegments.length === 0) { + paths = 1; + } else { + paths = 0; + for (const nextSegment of segment.nextSegments) { + paths += countPathsToEnd(nextSegment); + } + } + cache.set(segment.id, paths); + + return paths; + } + + /** + * Gets the shortest path length to the start of a code path. + * For example: + * + * ```js + * function MyComponent() { + * if (condition) { + * // Segment 1 + * } + * // Segment 2 + * } + * ``` + * + * There is only one path from segment 1 to the code path start. Its + * length is one so that is the shortest path. + * + * There are two paths from segment 2 to the code path start. One + * through segment 1 with a length of two and another directly to the + * start with a length of one. The shortest path has a length of one + * so we would return that. + */ + + function shortestPathLengthToStart(segment) { + const {cache} = shortestPathLengthToStart; + let length = cache.get(segment.id); + + // If `length` is null then we found a cycle! Return infinity since + // the shortest path is definitely not the one where we looped. + if (length === null) { + return Infinity; + } + + // We have a cached `length`. Return it. + if (length !== undefined) { + return length; + } + + // Compute `length` and cache it. Guarding against cycles. + cache.set(segment.id, null); + if (segment.prevSegments.length === 0) { + length = 1; + } else { + length = Infinity; + for (const prevSegment of segment.prevSegments) { + const prevLength = shortestPathLengthToStart(prevSegment); + if (prevLength < length) { + length = prevLength; + } + } + length += 1; + } + cache.set(segment.id, length); + return length; + } + + countPathsFromStart.cache = new Map(); + countPathsToEnd.cache = new Map(); + shortestPathLengthToStart.cache = new Map(); + + // Count all code paths to the end of our component/hook. Also primes + // the `countPathsToEnd` cache. + const allPathsFromStartToEnd = countPathsToEnd(codePath.initialSegment); + + // Gets the function name for our code path. If the function name is + // `undefined` then we know either that we have an anonymous function + // expression or our code path is not in a function. In both cases we + // will want to error since neither are React functional components or + // hook functions. + const codePathFunctionName = getFunctionName(codePathNode); + + // This is a valid code path for React hooks if we are direcly in a React + // functional component or we are in a hook function. + const isSomewhereInsideComponentOrHook = isInsideComponentOrHook( + codePathNode, + ); + const isDirectlyInsideComponentOrHook = codePathFunctionName + ? isComponentName(codePathFunctionName) || + isHook(codePathFunctionName) + : false; + + // Compute the earliest finalizer level using information from the + // cache. We expect all reachable final segments to have a cache entry + // after calling `visitSegment()`. + let shortestFinalPathLength = Infinity; + for (const finalSegment of codePath.finalSegments) { + if (!finalSegment.reachable) { + continue; + } + const length = shortestPathLengthToStart(finalSegment); + if (length < shortestFinalPathLength) { + shortestFinalPathLength = length; + } + } + + // Make sure all React Hooks pass our lint invariants. Log warnings + // if not. + for (const [segment, reactHooks] of reactHooksMap) { + // NOTE: We could report here that the hook is not reachable, but + // that would be redundant with more general "no unreachable" + // lint rules. + if (!segment.reachable) { + continue; + } + + // If there are any final segments with a shorter path to start then + // we possibly have an early return. + // + // If our segment is a final segment itself then siblings could + // possibly be early returns. + const possiblyHasEarlyReturn = + segment.nextSegments.length === 0 + ? shortestFinalPathLength <= shortestPathLengthToStart(segment) + : shortestFinalPathLength < shortestPathLengthToStart(segment); + + // Count all the paths from the start of our code path to the end of + // our code path that go _through_ this segment. The critical piece + // of this is _through_. If we just call `countPathsToEnd(segment)` + // then we neglect that we may have gone through multiple paths to get + // to this point! Consider: + // + // ```js + // function MyComponent() { + // if (a) { + // // Segment 1 + // } else { + // // Segment 2 + // } + // // Segment 3 + // if (b) { + // // Segment 4 + // } else { + // // Segment 5 + // } + // } + // ``` + // + // In this component we have four code paths: + // + // 1. `a = true; b = true` + // 2. `a = true; b = false` + // 3. `a = false; b = true` + // 4. `a = false; b = false` + // + // From segment 3 there are two code paths to the end through segment + // 4 and segment 5. However, we took two paths to get here through + // segment 1 and segment 2. + // + // If we multiply the paths from start (two) by the paths to end (two) + // for segment 3 we get four. Which is our desired count. + const pathsFromStartToEnd = + countPathsFromStart(segment) * countPathsToEnd(segment); + + // Is this hook a part of a cyclic segment? + const cycled = cyclic.has(segment.id); + + for (const hook of reactHooks) { + // Report an error if a hook may be called more then once. + if (cycled) { + context.report( + hook, + `React Hook "${context.getSource(hook)}" may be executed ` + + 'more than once. Possibly because it is called in a loop. ' + + 'React Hooks must be called in the exact same order in ' + + 'every component render.', + ); + } + + // If this is not a valid code path for React hooks then we need to + // log a warning for every hook in this code path. + // + // Pick a special message depending on the scope this hook was + // called in. + if (isDirectlyInsideComponentOrHook) { + // Report an error if a hook does not reach all finalizing code + // path segments. + // + // Special case when we think there might be an early return. + if (!cycled && pathsFromStartToEnd !== allPathsFromStartToEnd) { + context.report( + hook, + `React Hook "${context.getSource(hook)}" is called ` + + 'conditionally. React Hooks must be called in the exact ' + + 'same order in every component render.' + + (possiblyHasEarlyReturn + ? ' Did you accidentally call a React Hook after an' + + ' early return?' + : ''), + ); + } + } else if ( + codePathNode.parent && + (codePathNode.parent.type === 'MethodDefinition' || + codePathNode.parent.type === 'ClassProperty') && + codePathNode.parent.value === codePathNode + ) { + // Ignore class methods for now because they produce too many + // false positives due to feature flag checks. We're less + // sensitive to them in classes because hooks would produce + // runtime errors in classes anyway, and because a use*() + // call in a class, if it works, is unambigously *not* a hook. + } else if (codePathFunctionName) { + // Custom message if we found an invalid function name. + context.report( + hook, + `React Hook "${context.getSource(hook)}" is called in ` + + `function "${context.getSource(codePathFunctionName)}" ` + + 'which is neither a React function component or a custom ' + + 'React Hook function.', + ); + } else if (codePathNode.type === 'Program') { + // For now, ignore if it's in top level scope. + // We could warn here but there are false positives related + // configuring libraries like `history`. + } else { + // Assume in all other cases the user called a hook in some + // random function callback. This should usually be true for + // anonymous function expressions. Hopefully this is clarifying + // enough in the common case that the incorrect message in + // uncommon cases doesn't matter. + if (isSomewhereInsideComponentOrHook) { + context.report( + hook, + `React Hook "${context.getSource(hook)}" cannot be called ` + + 'inside a callback. React Hooks must be called in a ' + + 'React function component or a custom React Hook function.', + ); + } + } + } + } + }, + + // Missed opportunity...We could visit all `Identifier`s instead of all + // `CallExpression`s and check that _every use_ of a hook name is valid. + // But that gets complicated and enters type-system territory, so we're + // only being strict about hook calls for now. + CallExpression(node) { + if (isHook(node.callee)) { + // Add the hook node to a map keyed by the code path segment. We will + // do full code path analysis at the end of our code path. + const reactHooksMap = last(codePathReactHooksMapStack); + const codePathSegment = last(codePathSegmentStack); + let reactHooks = reactHooksMap.get(codePathSegment); + if (!reactHooks) { + reactHooks = []; + reactHooksMap.set(codePathSegment, reactHooks); + } + reactHooks.push(node.callee); + } + }, + }; + }, +}; + +/** + * Gets tbe static name of a function AST node. For function declarations it is + * easy. For anonymous function expressions it is much harder. If you search for + * `IsAnonymousFunctionDefinition()` in the ECMAScript spec you'll find places + * where JS gives anonymous function expressions names. We roughly detect the + * same AST nodes with some exceptions to better fit our usecase. + */ + +function getFunctionName(node) { + if ( + node.type === 'FunctionDeclaration' || + (node.type === 'FunctionExpression' && node.id) + ) { + // function useHook() {} + // const whatever = function useHook() {}; + // + // Function declaration or function expression names win over any + // assignment statements or other renames. + return node.id; + } else if ( + node.type === 'FunctionExpression' || + node.type === 'ArrowFunctionExpression' + ) { + if ( + node.parent.type === 'VariableDeclarator' && + node.parent.init === node + ) { + // const useHook = () => {}; + return node.parent.id; + } else if ( + node.parent.type === 'AssignmentExpression' && + node.parent.right === node && + node.parent.operator === '=' + ) { + // useHook = () => {}; + return node.parent.left; + } else if ( + node.parent.type === 'Property' && + node.parent.value === node && + !node.parent.computed + ) { + // {useHook: () => {}} + // {useHook() {}} + return node.parent.key; + + // NOTE: We could also support `ClassProperty` and `MethodDefinition` + // here to be pedantic. However, hooks in a class are an anti-pattern. So + // we don't allow it to error early. + // + // class {useHook = () => {}} + // class {useHook() {}} + } else if ( + node.parent.type === 'AssignmentPattern' && + node.parent.right === node && + !node.parent.computed + ) { + // const {useHook = () => {}} = {}; + // ({useHook = () => {}} = {}); + // + // Kinda clowny, but we'd said we'd follow spec convention for + // `IsAnonymousFunctionDefinition()` usage. + return node.parent.left; + } else { + return undefined; + } + } else { + return undefined; + } +} + +/** + * Convenience function for peeking the last item in a stack. + */ + +function last(array) { + return array[array.length - 1]; +} diff --git a/packages/eslint-plugin-react-hooks/src/index.js b/packages/eslint-plugin-react-hooks/src/index.js new file mode 100644 index 0000000000000..a3d6216e097bc --- /dev/null +++ b/packages/eslint-plugin-react-hooks/src/index.js @@ -0,0 +1,14 @@ +/** + * Copyright (c) Facebook, Inc. and its affiliates. + * + * This source code is licensed under the MIT license found in the + * LICENSE file in the root directory of this source tree. + */ + +'use strict'; + +import RuleOfHooks from './RulesOfHooks'; + +export const rules = { + 'rules-of-hooks': RuleOfHooks, +}; diff --git a/scripts/rollup/bundles.js b/scripts/rollup/bundles.js index 0e9bb851044f8..b3076d8ceeccb 100644 --- a/scripts/rollup/bundles.js +++ b/scripts/rollup/bundles.js @@ -410,6 +410,21 @@ const bundles = [ externals: ['jest-diff'], }, + /******* ESLint Plugin for Hooks (proposal) *******/ + { + label: 'eslint-plugin-react-hooks', + // TODO: it's awkward to create a bundle for this + // but if we don't, the package won't get copied. + // We also can't create just DEV bundle because + // it contains a NODE_ENV check inside. + // We should probably tweak our build process + // to allow "raw" packages that don't get bundled. + bundleTypes: [NODE_DEV, NODE_PROD], + moduleType: ISOMORPHIC, + entry: 'eslint-plugin-react-hooks', + externals: [], + }, + { label: 'scheduler-tracing', bundleTypes: [ From 504576306461a5ff339dc99691842f0f35a8bf4c Mon Sep 17 00:00:00 2001 From: Andrew Clark Date: Mon, 29 Oct 2018 11:39:04 -0700 Subject: [PATCH 19/19] Swap order of function member in hook union types --- packages/react-dom/src/server/ReactPartialRendererHooks.js | 4 ++-- packages/react-reconciler/src/ReactFiberHooks.js | 4 ++-- packages/react/src/ReactHooks.js | 2 +- 3 files changed, 5 insertions(+), 5 deletions(-) diff --git a/packages/react-dom/src/server/ReactPartialRendererHooks.js b/packages/react-dom/src/server/ReactPartialRendererHooks.js index b859e6dfcaaae..1420f1bcc9a2a 100644 --- a/packages/react-dom/src/server/ReactPartialRendererHooks.js +++ b/packages/react-dom/src/server/ReactPartialRendererHooks.js @@ -11,7 +11,7 @@ import type {ReactContext} from 'shared/ReactTypes'; import invariant from 'shared/invariant'; import warning from 'shared/warning'; -type BasicStateAction = S | (S => S); +type BasicStateAction = (S => S) | S; type MaybeCallback = void | null | (S => mixed); type Dispatch = (A, MaybeCallback) => void; @@ -158,7 +158,7 @@ function basicStateReducer(state: S, action: BasicStateAction): S { } export function useState( - initialState: S | (() => S), + initialState: (() => S) | S, ): [S, Dispatch>] { return useReducer( basicStateReducer, diff --git a/packages/react-reconciler/src/ReactFiberHooks.js b/packages/react-reconciler/src/ReactFiberHooks.js index bb7a0148ef09f..a542f6f24487b 100644 --- a/packages/react-reconciler/src/ReactFiberHooks.js +++ b/packages/react-reconciler/src/ReactFiberHooks.js @@ -75,7 +75,7 @@ export type FunctionComponentUpdateQueue = { lastEffect: Effect | null, }; -type BasicStateAction = S | (S => S); +type BasicStateAction = (S => S) | S; type MaybeCallback = void | null | (S => mixed); @@ -332,7 +332,7 @@ export function useContext( } export function useState( - initialState: S | (() => S), + initialState: (() => S) | S, ): [S, Dispatch>] { return useReducer( basicStateReducer, diff --git a/packages/react/src/ReactHooks.js b/packages/react/src/ReactHooks.js index 0e8f46dddbf67..e8667672fd02f 100644 --- a/packages/react/src/ReactHooks.js +++ b/packages/react/src/ReactHooks.js @@ -51,7 +51,7 @@ export function useContext( return dispatcher.useContext(Context, observedBits); } -export function useState(initialState: S | (() => S)) { +export function useState(initialState: (() => S) | S) { const dispatcher = resolveDispatcher(); return dispatcher.useState(initialState); }