From 91e5675d3b9227f08b459766201f6574e9c0c5ae Mon Sep 17 00:00:00 2001 From: Brian Vaughn Date: Fri, 21 Feb 2020 13:24:35 -0800 Subject: [PATCH] Read mutable source composed hooks from current dispatcher Rather than passing them in explicitly. This removes the extra function call. --- .../react-reconciler/src/ReactFiberHooks.js | 123 ++++-------------- ...eactHooksWithNoopRenderer-test.internal.js | 36 +++++ 2 files changed, 60 insertions(+), 99 deletions(-) diff --git a/packages/react-reconciler/src/ReactFiberHooks.js b/packages/react-reconciler/src/ReactFiberHooks.js index 8af3ce2b6bad8..e7f6bb8e26934 100644 --- a/packages/react-reconciler/src/ReactFiberHooks.js +++ b/packages/react-reconciler/src/ReactFiberHooks.js @@ -914,23 +914,7 @@ function readFromUnsubcribedMutableSource( } } -type MutableSourceRef = {| - getSnapshot: null | MutableSourceGetSnapshotFn, - source: null | MutableSource, - subscribe: null | MutableSourceSubscribeFn, -|}; - -function useMutableSourceImpl( - useEffectImpl: ( - create: () => (() => void) | void, - deps: Array | void | null, - ) => void, - useRefImpl: ( - initialValue: MutableSourceRef, - ) => {|current: MutableSourceRef|}, - useStateImpl: ( - initialState: () => MutableSourceState, - ) => [MutableSourceState, Dispatch], +function useMutableSource( source: MutableSource, getSnapshot: MutableSourceGetSnapshotFn, subscribe: MutableSourceSubscribeFn, @@ -938,7 +922,9 @@ function useMutableSourceImpl( const getVersion = source._getVersion; const version = getVersion(); - const [state, setState] = useStateImpl( + const dispatcher = ReactCurrentDispatcher.current; + + const [state, setState] = dispatcher.useState( () => ({ getSnapshot, @@ -948,18 +934,16 @@ function useMutableSourceImpl( }: MutableSourceState), ); - const ref = useRefImpl({ + const ref = dispatcher.useRef({ getSnapshot: null, source: null, subscribe: null, }); - let didUnsubscribe = false; - // If we got a new source or subscribe function, // we'll need to subscribe in a passive effect, // and also check for any changes that fire between render and subscribe. - useEffectImpl(() => { + dispatcher.useEffect(() => { const shouldResubscribe = subscribe !== ref.current.subscribe || source !== ref.current.source; @@ -975,22 +959,19 @@ function useMutableSourceImpl( if (shouldResubscribe) { const handleChange = () => { - // It's possible that this callback will be invoked even after being unsubscribed, - // if it's removed as a result of a subscription event/update. - // In this case, React might log a DEV warning about an update from an unmounted component. - // We can avoid triggering that warning with this check. - if (didUnsubscribe) { - return; - } - const latestGetSnapshot = ((ref.current .getSnapshot: any): MutableSourceGetSnapshotFn); let newSnapshot = null; - let snapshotDidThrow = false; try { newSnapshot = latestGetSnapshot(source._source); } catch (error) { - snapshotDidThrow = true; + // A selector might throw after a source mutation. + // e.g. it might try to read from a part of the store that no longer exists. + // In this case we should still schedule an update with React. + // Worst case the selector will throw again and then an error boundary will handle it. + setState(() => { + throw error; + }); } setState(prevState => { @@ -1005,14 +986,6 @@ function useMutableSourceImpl( return prevState; } - // A selector might throw after a source mutation. - // e.g. it might try to read from a part of the store that no longer exists. - // In this case we should still schedule an update with React. - // Worst case the selector will throw again and then an error boundary will handle it. - if (snapshotDidThrow) { - return {...prevState}; - } - // If the value hasn't changed, no update is needed. // Return state as-is so React can bail out and avoid an unnecessary render. if (prevState.snapshot === newSnapshot) { @@ -1044,10 +1017,7 @@ function useMutableSourceImpl( } } - return () => { - didUnsubscribe = true; - unsubscribe(); - }; + return unsubscribe; } }, [source, subscribe, getSnapshot]); @@ -1084,51 +1054,6 @@ function useMutableSourceImpl( return state.snapshot; } -function mountMutableSource( - source: MutableSource, - getSnapshot: MutableSourceGetSnapshotFn, - subscribe: MutableSourceSubscribeFn, -): Snapshot { - return useMutableSourceImpl( - mountEffect, - mountRef, - mountState, - source, - getSnapshot, - subscribe, - ); -} - -function updateMutableSource( - source: MutableSource, - getSnapshot: MutableSourceGetSnapshotFn, - subscribe: MutableSourceSubscribeFn, -): Snapshot { - return useMutableSourceImpl( - updateEffect, - updateRef, - updateState, - source, - getSnapshot, - subscribe, - ); -} - -function rerenderMutableSource( - source: MutableSource, - getSnapshot: MutableSourceGetSnapshotFn, - subscribe: MutableSourceSubscribeFn, -): Snapshot { - return useMutableSourceImpl( - updateEffect, - updateRef, - rerenderState, - source, - getSnapshot, - subscribe, - ); -} - function mountState( initialState: (() => S) | S, ): [S, Dispatch>] { @@ -1696,7 +1621,7 @@ const HooksDispatcherOnMount: Dispatcher = { useResponder: createDeprecatedResponderListener, useDeferredValue: mountDeferredValue, useTransition: mountTransition, - useMutableSource: mountMutableSource, + useMutableSource: useMutableSource, }; const HooksDispatcherOnUpdate: Dispatcher = { @@ -1715,7 +1640,7 @@ const HooksDispatcherOnUpdate: Dispatcher = { useResponder: createDeprecatedResponderListener, useDeferredValue: updateDeferredValue, useTransition: updateTransition, - useMutableSource: updateMutableSource, + useMutableSource: useMutableSource, }; const HooksDispatcherOnRerender: Dispatcher = { @@ -1734,7 +1659,7 @@ const HooksDispatcherOnRerender: Dispatcher = { useResponder: createDeprecatedResponderListener, useDeferredValue: rerenderDeferredValue, useTransition: rerenderTransition, - useMutableSource: rerenderMutableSource, + useMutableSource: useMutableSource, }; let HooksDispatcherOnMountInDEV: Dispatcher | null = null; @@ -1891,7 +1816,7 @@ if (__DEV__) { ): Snapshot { currentHookNameInDev = 'useMutableSource'; mountHookTypesDev(); - return mountMutableSource(source, getSnapshot, subscribe); + return useMutableSource(source, getSnapshot, subscribe); }, }; @@ -2017,7 +1942,7 @@ if (__DEV__) { ): Snapshot { currentHookNameInDev = 'useMutableSource'; updateHookTypesDev(); - return updateMutableSource(source, getSnapshot, subscribe); + return useMutableSource(source, getSnapshot, subscribe); }, }; @@ -2143,7 +2068,7 @@ if (__DEV__) { ): Snapshot { currentHookNameInDev = 'useMutableSource'; updateHookTypesDev(); - return updateMutableSource(source, getSnapshot, subscribe); + return useMutableSource(source, getSnapshot, subscribe); }, }; @@ -2269,7 +2194,7 @@ if (__DEV__) { ): Snapshot { currentHookNameInDev = 'useMutableSource'; updateHookTypesDev(); - return rerenderMutableSource(source, getSnapshot, subscribe); + return useMutableSource(source, getSnapshot, subscribe); }, }; @@ -2410,7 +2335,7 @@ if (__DEV__) { currentHookNameInDev = 'useMutableSource'; warnInvalidHookAccess(); mountHookTypesDev(); - return mountMutableSource(source, getSnapshot, subscribe); + return useMutableSource(source, getSnapshot, subscribe); }, }; @@ -2551,7 +2476,7 @@ if (__DEV__) { currentHookNameInDev = 'useMutableSource'; warnInvalidHookAccess(); updateHookTypesDev(); - return updateMutableSource(source, getSnapshot, subscribe); + return useMutableSource(source, getSnapshot, subscribe); }, }; @@ -2692,7 +2617,7 @@ if (__DEV__) { currentHookNameInDev = 'useMutableSource'; warnInvalidHookAccess(); updateHookTypesDev(); - return rerenderMutableSource(source, getSnapshot, subscribe); + return useMutableSource(source, getSnapshot, subscribe); }, }; } diff --git a/packages/react-reconciler/src/__tests__/ReactHooksWithNoopRenderer-test.internal.js b/packages/react-reconciler/src/__tests__/ReactHooksWithNoopRenderer-test.internal.js index 18e159cbc5cdc..d15ce1c35e7c6 100644 --- a/packages/react-reconciler/src/__tests__/ReactHooksWithNoopRenderer-test.internal.js +++ b/packages/react-reconciler/src/__tests__/ReactHooksWithNoopRenderer-test.internal.js @@ -3222,6 +3222,42 @@ function loadModules({ }); }); + it('should not warn about updates that fire between unmount and passive unsubcribe', () => { + const source = createSource('one'); + const mutableSource = createMutableSource(source); + + function Wrapper() { + React.useLayoutEffect(() => () => { + Scheduler.unstable_yieldValue('layout unmount'); + }); + return ( + + ); + } + + act(() => { + ReactNoop.renderToRootWithID(, 'root', () => + Scheduler.unstable_yieldValue('Sync effect'), + ); + expect(Scheduler).toFlushAndYield(['only:one', 'Sync effect']); + ReactNoop.flushPassiveEffects(); + + // Umounting a root should remove the remaining event listeners in a passive effect + ReactNoop.unmountRootWithID('root'); + expect(Scheduler).toFlushAndYieldThrough(['layout unmount']); + + // Changes to source should not cause a warning, + // even though the unsubscribe hasn't run yet (since it's a pending passive effect). + source.value = 'two'; + expect(Scheduler).toFlushAndYield([]); + }); + }); + // TODO (useMutableSource) Test for multiple updates at different priorities });