diff --git a/CHANGELOG.md b/CHANGELOG.md index 08dfd21221..ceff97fa25 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -27,9 +27,11 @@ ### Other Fixes and Optimizations - Only clone the current snapshot for callbacks if the callback actually uses it. (#1501) - Fix transitive selector refresh for some cases (#1409) -- Run atom effects when atoms are initialized from a set during a transaction from `useRecoilTransaction_UNSTABLE()` (#1466) -- Unsubscribe `onSet()` handlers in atom effects when atoms are cleaned up. (#1509) -- Atom effects are cleaned up when initialized by a Snapshot which is released. (#1511) +- Atom Effects + - Run atom effects when atoms are initialized from a set during a transaction from `useRecoilTransaction_UNSTABLE()` (#1466) + - Atom effects are cleaned up when initialized by a Snapshot which is released. (#1511) + - Unsubscribe `onSet()` handlers in atom effects when atoms are cleaned up. (#1509) + - Call `onSet()` when atoms are initialized with `` (#1519, #1511) - Avoid extra re-renders in some cases when a component uses a different atom/selector. (#825) - `` will only call `initializeState()` once during the initial render. (#1372) diff --git a/packages/recoil/core/__tests__/Recoil_RecoilRoot-test.js b/packages/recoil/core/__tests__/Recoil_RecoilRoot-test.js index 26093d3d6f..3a69f6d0da 100644 --- a/packages/recoil/core/__tests__/Recoil_RecoilRoot-test.js +++ b/packages/recoil/core/__tests__/Recoil_RecoilRoot-test.js @@ -109,18 +109,14 @@ describe('initializeState', () => { effects_UNSTABLE: [ ({setSelf}) => { effectRan++; - setSelf(current => { - // Effects are run first. - expect(current).toEqual('DEFAULT'); - return 'EFFECT'; - }); + setSelf('EFFECT'); }, ], }); function initializeState({set}) { set(myAtom, current => { - // Effects are run first, initializeState() takes precedence + // Effects are run first expect(current).toEqual('EFFECT'); return 'INITIALIZE'; }); @@ -136,15 +132,51 @@ describe('initializeState', () => { expect(container1.textContent).toEqual('NO READ'); expect(effectRan).toEqual(strictMode ? (concurrentMode ? 4 : 3) : 2); + effectRan = 0; const container2 = renderElements( , ); - // Effects are run first, initializeState() takes precedence + // Effects takes precedence expect(container2.textContent).toEqual('"EFFECT"'); - expect(effectRan).toEqual(strictMode ? (concurrentMode ? 8 : 6) : 4); + expect(effectRan).toEqual(strictMode ? (concurrentMode ? 4 : 3) : 2); + }, + ); + + testRecoil( + 'onSet() called when atom initialized with initializeState', + () => { + const setValues = []; + const myAtom = atom({ + key: 'RecoilRoot - initializeState - onSet', + default: 0, + effects_UNSTABLE: [ + ({onSet, setSelf}) => { + onSet(value => { + setValues.push(value); + // Confirm setSelf() works when initialized with initializeState + setSelf(value + 1); + }); + }, + ], + }); + + const [MyAtom, setAtom] = componentThatReadsAndWritesAtom(myAtom); + + const c = renderElements( + set(myAtom, 1)}> + + , + ); + + expect(c.textContent).toBe('1'); + expect(setValues).toEqual([]); + + act(() => setAtom(2)); + expect(setValues).toEqual([2]); + expect(c.textContent).toBe('3'); }, ); diff --git a/packages/recoil/hooks/__tests__/Recoil_useRecoilCallback-test.js b/packages/recoil/hooks/__tests__/Recoil_useRecoilCallback-test.js index ac7ba363e6..be6a857824 100644 --- a/packages/recoil/hooks/__tests__/Recoil_useRecoilCallback-test.js +++ b/packages/recoil/hooks/__tests__/Recoil_useRecoilCallback-test.js @@ -24,6 +24,7 @@ let React, selector, useRecoilCallback, useRecoilValue, + useRecoilState, useSetRecoilState, ReadsAtom, flushPromisesAndTimers, @@ -43,6 +44,7 @@ const testRecoil = getRecoilTestFn(() => { useRecoilCallback, useSetRecoilState, useRecoilValue, + useRecoilState, } = require('../../Recoil_index')); ({ ReadsAtom, @@ -375,157 +377,202 @@ testRecoil('Consistent callback function', () => { expect(out.textContent).toBe('1'); }); -testRecoil( - 'Atom effects are initialized twice if first seen on snapshot and then on root store', - ({strictMode, concurrentMode}) => { - const sm = strictMode ? 1 : 0; - let numTimesEffectInit = 0; +describe('Atom Effects', () => { + testRecoil( + 'Atom effects are initialized twice if first seen on snapshot and then on root store', + ({strictMode, concurrentMode}) => { + const sm = strictMode ? 1 : 0; + let numTimesEffectInit = 0; + + const atomWithEffect = atom({ + key: 'atomWithEffect', + default: 0, + effects_UNSTABLE: [ + () => { + numTimesEffectInit++; + }, + ], + }); - const atomWithEffect = atom({ - key: 'atomWithEffect', - default: 0, - effects_UNSTABLE: [ - () => { - numTimesEffectInit++; - }, - ], - }); + // StrictMode will render the component twice + let renderCount = 0; - // StrictMode will render the component twice - let renderCount = 0; + const Component = () => { + const readAtomFromSnapshot = useRecoilCallback(({snapshot}) => () => { + snapshot.getLoadable(atomWithEffect); + }); - const Component = () => { - const readAtomFromSnapshot = useRecoilCallback(({snapshot}) => () => { - snapshot.getLoadable(atomWithEffect); + readAtomFromSnapshot(); // first initialization + expect(numTimesEffectInit).toBe(1 + sm * renderCount); + + useRecoilValue(atomWithEffect); // second initialization + expect(numTimesEffectInit).toBe(2); + + renderCount++; + return null; + }; + + const c = renderElements(); + expect(c.textContent).toBe(''); // Confirm no failures from rendering + expect(numTimesEffectInit).toBe(strictMode && concurrentMode ? 3 : 2); + }, + ); + + testRecoil( + 'Atom effects are initialized once if first seen on root store and then on snapshot', + ({strictMode, concurrentMode}) => { + let numTimesEffectInit = 0; + + const atomWithEffect = atom({ + key: 'atomWithEffect2', + default: 0, + effects_UNSTABLE: [ + () => { + numTimesEffectInit++; + }, + ], }); - readAtomFromSnapshot(); // first initialization - expect(numTimesEffectInit).toBe(1 + sm * renderCount); + const Component = () => { + const readAtomFromSnapshot = useRecoilCallback(({snapshot}) => () => { + snapshot.getLoadable(atomWithEffect); + }); - useRecoilValue(atomWithEffect); // second initialization - expect(numTimesEffectInit).toBe(2); + useRecoilValue(atomWithEffect); // first initialization + expect(numTimesEffectInit).toBe(1); - renderCount++; - return null; - }; + /** + * should not re-initialize b/c snapshot should inherit from latest state, + * wherein atom was already initialized + */ + readAtomFromSnapshot(); + expect(numTimesEffectInit).toBe(1); - const c = renderElements(); - expect(c.textContent).toBe(''); // Confirm no failures from rendering - expect(numTimesEffectInit).toBe(strictMode && concurrentMode ? 3 : 2); - }, -); - -testRecoil( - 'Atom effects are initialized once if first seen on root store and then on snapshot', - ({strictMode, concurrentMode}) => { - let numTimesEffectInit = 0; - - const atomWithEffect = atom({ - key: 'atomWithEffect2', + return null; + }; + + const c = renderElements(); + expect(c.textContent).toBe(''); // Confirm no failures from rendering + expect(numTimesEffectInit).toBe(strictMode && concurrentMode ? 2 : 1); + }, + ); + + testRecoil('onSet() called when atom initialized with snapshot', () => { + const setValues = []; + const myAtom = atom({ + key: 'useRecoilCallback - atom effect - onSet', default: 0, effects_UNSTABLE: [ - () => { - numTimesEffectInit++; + ({onSet, setSelf}) => { + onSet(value => { + setValues.push(value); + // Confirm setSelf() still valid when initialized from snapshot + setSelf(value + 1); + }); }, ], }); + let setAtom; const Component = () => { const readAtomFromSnapshot = useRecoilCallback(({snapshot}) => () => { - snapshot.getLoadable(atomWithEffect); + snapshot.getLoadable(myAtom); }); - useRecoilValue(atomWithEffect); // first initialization - expect(numTimesEffectInit).toBe(1); - - /** - * should not re-initialize b/c snapshot should inherit from latest state, - * wherein atom was already initialized - */ + // First initialization with snapshot readAtomFromSnapshot(); - expect(numTimesEffectInit).toBe(1); - return null; + // Second initialization with hook + let value; + [value, setAtom] = useRecoilState(myAtom); + return value; }; const c = renderElements(); - expect(c.textContent).toBe(''); // Confirm no failures from rendering - expect(numTimesEffectInit).toBe(strictMode && concurrentMode ? 2 : 1); - }, -); - -testRecoil('Refresh selector cache - transitive', () => { - const getA = jest.fn(() => 'A'); - const selectorA = selector({ - key: 'useRecoilCallback refresh ancestors A', - get: getA, - }); - const getB = jest.fn(({get}) => get(selectorA) + 'B'); - const selectorB = selector({ - key: 'useRecoilCallback refresh ancestors B', - get: getB, - }); + expect(c.textContent).toBe('0'); + expect(setValues).toEqual([]); - const getC = jest.fn(({get}) => get(selectorB) + 'C'); - const selectorC = selector({ - key: 'useRecoilCallback refresh ancestors C', - get: getC, + act(() => setAtom(1)); + expect(setValues).toEqual([1]); + expect(c.textContent).toBe('2'); }); +}); - let refreshSelector; - function Component() { - refreshSelector = useRecoilCallback(({refresh}) => () => { - refresh(selectorC); +describe('Selector Cache', () => { + testRecoil('Refresh selector cache - transitive', () => { + const getA = jest.fn(() => 'A'); + const selectorA = selector({ + key: 'useRecoilCallback refresh ancestors A', + get: getA, }); - return useRecoilValue(selectorC); - } - const container = renderElements(); - expect(container.textContent).toBe('ABC'); - expect(getC).toHaveBeenCalledTimes(1); - expect(getB).toHaveBeenCalledTimes(1); - expect(getA).toHaveBeenCalledTimes(1); - - act(() => refreshSelector()); - expect(container.textContent).toBe('ABC'); - expect(getC).toHaveBeenCalledTimes(2); - expect(getB).toHaveBeenCalledTimes(2); - expect(getA).toHaveBeenCalledTimes(2); -}); + const getB = jest.fn(({get}) => get(selectorA) + 'B'); + const selectorB = selector({ + key: 'useRecoilCallback refresh ancestors B', + get: getB, + }); -testRecoil('Refresh selector cache - clears entire cache', async () => { - const myatom = atom({ - key: 'useRecoilCallback refresh entire cache atom', - default: 'a', - }); + const getC = jest.fn(({get}) => get(selectorB) + 'C'); + const selectorC = selector({ + key: 'useRecoilCallback refresh ancestors C', + get: getC, + }); - let i = 0; - const myselector = selector({ - key: 'useRecoilCallback refresh entire cache selector', - get: ({get}) => [get(myatom), i++], + let refreshSelector; + function Component() { + refreshSelector = useRecoilCallback(({refresh}) => () => { + refresh(selectorC); + }); + return useRecoilValue(selectorC); + } + + const container = renderElements(); + expect(container.textContent).toBe('ABC'); + expect(getC).toHaveBeenCalledTimes(1); + expect(getB).toHaveBeenCalledTimes(1); + expect(getA).toHaveBeenCalledTimes(1); + + act(() => refreshSelector()); + expect(container.textContent).toBe('ABC'); + expect(getC).toHaveBeenCalledTimes(2); + expect(getB).toHaveBeenCalledTimes(2); + expect(getA).toHaveBeenCalledTimes(2); }); - let setMyAtom; - let refreshSelector; - function Component() { - const [atomValue, iValue] = useRecoilValue(myselector); - refreshSelector = useRecoilCallback(({refresh}) => () => { - refresh(myselector); + testRecoil('Refresh selector cache - clears entire cache', async () => { + const myatom = atom({ + key: 'useRecoilCallback refresh entire cache atom', + default: 'a', }); - setMyAtom = useSetRecoilState(myatom); - return `${atomValue}-${iValue}`; - } - const container = renderElements(); - expect(container.textContent).toBe('a-0'); + let i = 0; + const myselector = selector({ + key: 'useRecoilCallback refresh entire cache selector', + get: ({get}) => [get(myatom), i++], + }); - act(() => setMyAtom('b')); - expect(container.textContent).toBe('b-1'); + let setMyAtom; + let refreshSelector; + function Component() { + const [atomValue, iValue] = useRecoilValue(myselector); + refreshSelector = useRecoilCallback(({refresh}) => () => { + refresh(myselector); + }); + setMyAtom = useSetRecoilState(myatom); + return `${atomValue}-${iValue}`; + } + + const container = renderElements(); + expect(container.textContent).toBe('a-0'); - act(() => refreshSelector()); - expect(container.textContent).toBe('b-2'); + act(() => setMyAtom('b')); + expect(container.textContent).toBe('b-1'); - act(() => setMyAtom('a')); - expect(container.textContent).toBe('a-3'); + act(() => refreshSelector()); + expect(container.textContent).toBe('b-2'); + + act(() => setMyAtom('a')); + expect(container.textContent).toBe('a-3'); + }); }); diff --git a/packages/recoil/recoil_values/Recoil_atom.js b/packages/recoil/recoil_values/Recoil_atom.js index b2885e212a..831207a9a6 100644 --- a/packages/recoil/recoil_values/Recoil_atom.js +++ b/packages/recoil/recoil_values/Recoil_atom.js @@ -308,12 +308,11 @@ function baseAtom(options: BaseAtomOptions): RecoilState { const setSelf = (effect: AtomEffect) => (valueOrUpdater: NewValueOrUpdater) => { if (duringInit) { + const currentLoadable = getLoadable(node); const currentValue: T | DefaultValue = - initValue instanceof DefaultValue || isPromise(initValue) - ? defaultLoadable.state === 'hasValue' - ? defaultLoadable.contents - : DEFAULT_VALUE - : initValue; + currentLoadable.state === 'hasValue' + ? currentLoadable.contents + : DEFAULT_VALUE; initValue = typeof valueOrUpdater === 'function' ? // cast to any because we can't restrict T from being a function without losing support for opaque types diff --git a/packages/recoil/recoil_values/__tests__/Recoil_atom-test.js b/packages/recoil/recoil_values/__tests__/Recoil_atom-test.js index 69e6d7c70d..1409d21966 100644 --- a/packages/recoil/recoil_values/__tests__/Recoil_atom-test.js +++ b/packages/recoil/recoil_values/__tests__/Recoil_atom-test.js @@ -396,6 +396,7 @@ describe('Effects', () => { testRecoil('async set', () => { let setAtom, resetAtom; + let effectRan = false; const myAtom = atom({ key: 'atom effect init set', @@ -405,7 +406,8 @@ describe('Effects', () => { setAtom = setSelf; resetAtom = resetSelf; setSelf(x => { - expect(x).toEqual('DEFAULT'); + expect(x).toEqual(effectRan ? 'INIT' : 'DEFAULT'); + effectRan = true; return 'INIT'; }); }, diff --git a/packages/shared/__test_utils__/Recoil_TestingUtils.js b/packages/shared/__test_utils__/Recoil_TestingUtils.js index 12cfebf863..240254f75b 100644 --- a/packages/shared/__test_utils__/Recoil_TestingUtils.js +++ b/packages/shared/__test_utils__/Recoil_TestingUtils.js @@ -49,6 +49,8 @@ const err = require('recoil-shared/util/Recoil_err'); const ReactDOM = require('react-dom'); // @oss-only const StrictMode = React.StrictMode; // @oss-only +const QUICK_TEST = false; + // @fb-only: const IS_INTERNAL = true; const IS_INTERNAL = false; // @oss-only @@ -363,46 +365,52 @@ const testGKs = }); } - runTests({strictMode: false, concurrentMode: false}); - runTests({strictMode: true, concurrentMode: false}); - if (isConcurrentModeAvailable()) { + if (QUICK_TEST) { runTests({strictMode: false, concurrentMode: true}); - // 2020-12-20: The internal isn't yet enabled to run effects - // multiple times. So, rely on GitHub CI actions to test this for now. - if (!IS_INTERNAL) { - runTests({strictMode: true, concurrentMode: true}); + } else { + runTests({strictMode: false, concurrentMode: false}); + runTests({strictMode: true, concurrentMode: false}); + if (isConcurrentModeAvailable()) { + runTests({strictMode: false, concurrentMode: true}); + // 2020-12-20: The internal isn't yet enabled to run effects + // multiple times. So, rely on GitHub CI actions to test this for now. + if (!IS_INTERNAL) { + runTests({strictMode: true, concurrentMode: true}); + } } } }; -const WWW_GKS_TO_TEST = [ - // OSS for React <18: - ['recoil_hamt_2020', 'recoil_suppress_rerender_in_callback'], // Also enables early rendering - // Current internal default: - ['recoil_hamt_2020', 'recoil_mutable_source'], - // Internal with suppress, early rendering, and useTransition() support: - [ - 'recoil_hamt_2020', - 'recoil_mutable_source', - 'recoil_suppress_rerender_in_callback', // Also enables early rendering - ], - // OSS for React 18, test internally: - [ - 'recoil_hamt_2020', - 'recoil_sync_external_store', - 'recoil_suppress_rerender_in_callback', // Only used for fallback if no useSyncExternalStore() - ], - // Latest with GC: - [ - 'recoil_hamt_2020', - 'recoil_sync_external_store', - 'recoil_suppress_rerender_in_callback', - 'recoil_memory_managament_2020', - 'recoil_release_on_cascading_update_killswitch_2021', - ], - // Experimental mode for useTransition() support: - // ['recoil_hamt_2020', 'recoil_concurrent_legacy'], -]; +const WWW_GKS_TO_TEST = QUICK_TEST + ? [['recoil_hamt_2020', 'recoil_sync_external_store']] + : [ + // OSS for React <18: + ['recoil_hamt_2020', 'recoil_suppress_rerender_in_callback'], // Also enables early rendering + // Current internal default: + ['recoil_hamt_2020', 'recoil_mutable_source'], + // Internal with suppress, early rendering, and useTransition() support: + [ + 'recoil_hamt_2020', + 'recoil_mutable_source', + 'recoil_suppress_rerender_in_callback', // Also enables early rendering + ], + // OSS for React 18, test internally: + [ + 'recoil_hamt_2020', + 'recoil_sync_external_store', + 'recoil_suppress_rerender_in_callback', // Only used for fallback if no useSyncExternalStore() + ], + // Latest with GC: + [ + 'recoil_hamt_2020', + 'recoil_sync_external_store', + 'recoil_suppress_rerender_in_callback', + 'recoil_memory_managament_2020', + 'recoil_release_on_cascading_update_killswitch_2021', + ], + // Experimental mode for useTransition() support: + // ['recoil_hamt_2020', 'recoil_concurrent_legacy'], + ]; /** * GK combinations to exclude in OSS, presumably because these combinations pass