From 1a55ac6405fff19b63a6e9d0bbd6002241a7fd0a Mon Sep 17 00:00:00 2001 From: daishi Date: Thu, 18 Jul 2019 22:37:47 +0900 Subject: [PATCH 1/3] invoke-listeners-in-render --- CHANGELOG.md | 2 ++ src/Provider.js | 13 ++++++++++--- src/batchedUpdates.js | 2 ++ src/batchedUpdates.native.js | 2 ++ src/useSelector.js | 16 ++++++++-------- src/useTrackedState.js | 22 ++++++++++++---------- 6 files changed, 36 insertions(+), 21 deletions(-) create mode 100644 src/batchedUpdates.js create mode 100644 src/batchedUpdates.native.js diff --git a/CHANGELOG.md b/CHANGELOG.md index a47d45d..296ecf1 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -1,6 +1,8 @@ # Change Log ## [Unreleased] +### Changed +- No useLayoutEffect for invoking listeners (which leads de-opt sync mode) ## [4.0.0] - 2019-06-22 ### Changed diff --git a/src/Provider.js b/src/Provider.js index e0b17e1..34b82f2 100644 --- a/src/Provider.js +++ b/src/Provider.js @@ -6,7 +6,8 @@ import { useRef, } from 'react'; -import { useIsomorphicLayoutEffect, useForceUpdate } from './utils'; +import { batchedUpdates } from './batchedUpdates'; +import { useForceUpdate } from './utils'; // ------------------------------------------------------- // context @@ -47,9 +48,15 @@ export const Provider = ({ const forceUpdate = useForceUpdate(); const state = store.getState(); const listeners = useRef([]); - useIsomorphicLayoutEffect(() => { + // we call listeners in render intentionally. + // listeners are not technically pure, but + // otherwise we can't get benefits from concurrent mode. + // we make sure to work with double or more invocation of listeners. + // maybe we don't need `batchedUpdates` here to ensure top-down updates, + // but put it just in case. (review wanted) + batchedUpdates(() => { listeners.current.forEach(listener => listener(state)); - }, [state]); + }); const subscribe = useCallback((listener) => { listeners.current.push(listener); const unsubscribe = () => { diff --git a/src/batchedUpdates.js b/src/batchedUpdates.js new file mode 100644 index 0000000..578a4ce --- /dev/null +++ b/src/batchedUpdates.js @@ -0,0 +1,2 @@ +// eslint-disable-next-line +export { unstable_batchedUpdates as batchedUpdates } from 'react-dom'; diff --git a/src/batchedUpdates.native.js b/src/batchedUpdates.native.js new file mode 100644 index 0000000..07a2169 --- /dev/null +++ b/src/batchedUpdates.native.js @@ -0,0 +1,2 @@ +// eslint-disable-next-line +export { unstable_batchedUpdates as batchedUpdates } from 'react-native'; diff --git a/src/useSelector.js b/src/useSelector.js index aad53ba..94a241c 100644 --- a/src/useSelector.js +++ b/src/useSelector.js @@ -30,17 +30,17 @@ export const useSelector = (selector, eqlFn, opts) => { }); useEffect(() => { const callback = (nextState) => { - if (ref.current.state === nextState) return; - let changed; try { - changed = !ref.current.equalityFn(ref.current.selected, ref.current.selector(nextState)); + if (ref.current.state === nextState + || ref.current.equalityFn(ref.current.selected, ref.current.selector(nextState))) { + // not changed + return; + } } catch (e) { - changed = true; // stale props or some other reason - } - if (changed) { - ref.current.state = nextState; - forceUpdate(); + // ignored (stale props or some other reason) } + ref.current.state = nextState; + forceUpdate(); }; const unsubscribe = subscribe(callback); return unsubscribe; diff --git a/src/useTrackedState.js b/src/useTrackedState.js index 55ae2c1..958fa54 100644 --- a/src/useTrackedState.js +++ b/src/useTrackedState.js @@ -33,17 +33,19 @@ export const useTrackedState = (opts = {}) => { }); useEffect(() => { const callback = (nextState) => { - const changed = isDeepChanged( - lastTracked.current.state, - nextState, - lastTracked.current.affected, - lastTracked.current.cache, - lastTracked.current.assumeChangedIfNotAffected, - ); - if (changed) { - lastTracked.current.state = nextState; - forceUpdate(); + if (lastTracked.current.state === nextState + || !isDeepChanged( + lastTracked.current.state, + nextState, + lastTracked.current.affected, + lastTracked.current.cache, + lastTracked.current.assumeChangedIfNotAffected, + )) { + // not changed + return; } + lastTracked.current.state = nextState; + forceUpdate(); }; const unsubscribe = subscribe(callback); return unsubscribe; From 7bbf8d16011ec32cfd36e6ede994bb346edecab0 Mon Sep 17 00:00:00 2001 From: daishi Date: Sat, 20 Jul 2019 14:15:58 +0900 Subject: [PATCH 2/3] do not mutate refs in callback because it is called in render --- src/useSelector.js | 1 - src/useTrackedSelectors.js | 27 ++++++++++++++------------- src/useTrackedState.js | 1 - 3 files changed, 14 insertions(+), 15 deletions(-) diff --git a/src/useSelector.js b/src/useSelector.js index 94a241c..81e7ce7 100644 --- a/src/useSelector.js +++ b/src/useSelector.js @@ -39,7 +39,6 @@ export const useSelector = (selector, eqlFn, opts) => { } catch (e) { // ignored (stale props or some other reason) } - ref.current.state = nextState; forceUpdate(); }; const unsubscribe = subscribe(callback); diff --git a/src/useTrackedSelectors.js b/src/useTrackedSelectors.js index 8875a88..b346be2 100644 --- a/src/useTrackedSelectors.js +++ b/src/useTrackedSelectors.js @@ -62,30 +62,31 @@ export const useTrackedSelectors = (selectorMap, opts = {}) => { // update ref const lastTracked = useRef(null); useIsomorphicLayoutEffect(() => { - lastTracked.current = { keys, mapped, trapped }; + lastTracked.current = { + state, + keys, + mapped, + trapped, + }; }); // subscription useEffect(() => { const callback = (nextState) => { + if (lastTracked.current.state === nextState) return; try { - let changed = false; - const nextMapped = createMap(lastTracked.current.keys, (key) => { + const changed = lastTracked.current.keys.some((key) => { + if (!lastTracked.current.trapped.usage.has(key)) return false; const lastResult = lastTracked.current.mapped[key]; - if (!lastTracked.current.trapped.usage.has(key)) return lastResult; const nextResult = runSelector(nextState, lastResult.selector); - if (nextResult.value !== lastResult.value) { - changed = true; - } - return nextResult; + return nextResult.value !== lastResult.value; }); - if (changed) { - lastTracked.current.mapped = nextMapped; - forceUpdate(); + if (!changed) { + return; } } catch (e) { - // detect erorr (probably stale props) - forceUpdate(); + // ignored (probably stale props) } + forceUpdate(); }; const unsubscribe = subscribe(callback); return unsubscribe; diff --git a/src/useTrackedState.js b/src/useTrackedState.js index 958fa54..675afbe 100644 --- a/src/useTrackedState.js +++ b/src/useTrackedState.js @@ -44,7 +44,6 @@ export const useTrackedState = (opts = {}) => { // not changed return; } - lastTracked.current.state = nextState; forceUpdate(); }; const unsubscribe = subscribe(callback); From 69b8d7009cb9c8ba7b3e49e0b0a3dfc3b4da56a4 Mon Sep 17 00:00:00 2001 From: daishi Date: Sat, 20 Jul 2019 15:07:43 +0900 Subject: [PATCH 3/3] eliminate batchedUpdates --- src/Provider.js | 7 +------ src/batchedUpdates.js | 2 -- src/batchedUpdates.native.js | 2 -- 3 files changed, 1 insertion(+), 10 deletions(-) delete mode 100644 src/batchedUpdates.js delete mode 100644 src/batchedUpdates.native.js diff --git a/src/Provider.js b/src/Provider.js index 34b82f2..cdb0a1f 100644 --- a/src/Provider.js +++ b/src/Provider.js @@ -6,7 +6,6 @@ import { useRef, } from 'react'; -import { batchedUpdates } from './batchedUpdates'; import { useForceUpdate } from './utils'; // ------------------------------------------------------- @@ -52,11 +51,7 @@ export const Provider = ({ // listeners are not technically pure, but // otherwise we can't get benefits from concurrent mode. // we make sure to work with double or more invocation of listeners. - // maybe we don't need `batchedUpdates` here to ensure top-down updates, - // but put it just in case. (review wanted) - batchedUpdates(() => { - listeners.current.forEach(listener => listener(state)); - }); + listeners.current.forEach(listener => listener(state)); const subscribe = useCallback((listener) => { listeners.current.push(listener); const unsubscribe = () => { diff --git a/src/batchedUpdates.js b/src/batchedUpdates.js deleted file mode 100644 index 578a4ce..0000000 --- a/src/batchedUpdates.js +++ /dev/null @@ -1,2 +0,0 @@ -// eslint-disable-next-line -export { unstable_batchedUpdates as batchedUpdates } from 'react-dom'; diff --git a/src/batchedUpdates.native.js b/src/batchedUpdates.native.js deleted file mode 100644 index 07a2169..0000000 --- a/src/batchedUpdates.native.js +++ /dev/null @@ -1,2 +0,0 @@ -// eslint-disable-next-line -export { unstable_batchedUpdates as batchedUpdates } from 'react-native';