diff --git a/CHANGELOG.md b/CHANGELOG.md index f2d8844..34bebbc 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -32,6 +32,7 @@ - Only clone the current snapshot for callbacks if the callback actually uses it. (#1501) - Cache the cloned snapshots from callbacks unless there was a state change. (#1533) - Fix transitive selector refresh for some cases (#1409) +- Fix some corner cases with async selectors and multiple stores (#1568) - Atom Effects - Rename option from `effects_UNSTABLE` to just `effects` as the interface is mostly stabilizing (#1520) - Run atom effects when atoms are initialized from a set during a transaction from `useRecoilTransaction_UNSTABLE()` (#1466) diff --git a/packages/recoil/adt/Recoil_Loadable.js b/packages/recoil/adt/Recoil_Loadable.js index 81b2a54..2a7b698 100644 --- a/packages/recoil/adt/Recoil_Loadable.js +++ b/packages/recoil/adt/Recoil_Loadable.js @@ -198,6 +198,10 @@ export type Loadable<+T> = | $ReadOnly> | $ReadOnly>; +export type ValueLoadableType<+T> = $ReadOnly>; +export type ErrorLoadableType<+T> = $ReadOnly>; +export type LoadingLoadableType<+T> = $ReadOnly>; + function loadableWithValue<+T>(value: T): $ReadOnly> { return Object.freeze(new ValueLoadable(value)); } diff --git a/packages/recoil/recoil_values/Recoil_selector.js b/packages/recoil/recoil_values/Recoil_selector.js index 5ad8902..35353e2 100644 --- a/packages/recoil/recoil_values/Recoil_selector.js +++ b/packages/recoil/recoil_values/Recoil_selector.js @@ -53,7 +53,7 @@ */ 'use strict'; -import type {Loadable} from '../adt/Recoil_Loadable'; +import type {Loadable, LoadingLoadableType} from '../adt/Recoil_Loadable'; import type {CachePolicy} from '../caches/Recoil_CachePolicy'; import type { NodeCacheRoute, @@ -185,10 +185,10 @@ type ExecutionId = number; * to global state when it finishes. */ type ExecutionInfo = { - depValuesDiscoveredSoFarDuringAsyncWork: ?DepValues, - latestLoadable: ?Loadable, - latestExecutionId: ?ExecutionId, - stateVersion: ?StateID, + depValuesDiscoveredSoFarDuringAsyncWork: DepValues, + latestLoadable: LoadingLoadableType, + latestExecutionId: ExecutionId, + stateVersion: StateID, }; // An object to hold the current state for loading dependencies for a particular @@ -205,6 +205,11 @@ type LoadingDepsState = { const dependencyStack = []; // for detecting circular dependencies. const waitingStores: Map> = new Map(); +const getNewExecutionId: () => ExecutionId = (() => { + let executionId = 0; + return () => executionId++; +})(); + /* eslint-disable no-redeclare */ declare function selector( options: ReadOnlySelectorOptions, @@ -213,20 +218,6 @@ declare function selector( options: ReadWriteSelectorOptions, ): RecoilState; -const getNewExecutionId: () => ExecutionId = (() => { - let executionId = 0; - return () => executionId++; -})(); - -function getInitialExecutionInfo(): ExecutionInfo { - return { - depValuesDiscoveredSoFarDuringAsyncWork: null, - latestLoadable: null, - latestExecutionId: null, - stateVersion: null, - }; -} - function selector( options: ReadOnlySelectorOptions | ReadWriteSelectorOptions, ): RecoilValue { @@ -266,14 +257,6 @@ function selector( return !gkx('recoil_memory_managament_2020') || liveStoresCount > 0; } - function getExecutionInfo(store: Store): ExecutionInfo { - if (!executionInfoMap.has(store)) { - executionInfoMap.set(store, getInitialExecutionInfo()); - } - - return nullthrows(executionInfoMap.get(store)); - } - function selectorInit(store: Store): () => void { store.getState().knownSelectors.add(key); liveStoresCount++; @@ -286,27 +269,45 @@ function selector( return getConfigDeletionHandler(key) !== undefined && !selectorIsLive(); } - function notifyStoreWhenAsyncSettles( + function resolveAsync( store: Store, + state: TreeState, executionId: ExecutionId, + loadable: Loadable, + depValues: DepValues, ): void { - let stores = waitingStores.get(executionId); - if (stores == null) { - waitingStores.set(executionId, (stores = new Set())); - } - stores.add(store); + setCache(state, loadable, depValues); + setDepsInStore(store, state, new Set(depValues.keys()), executionId); + notifyStoresOfResolvedAsync(store, executionId); } - function notifyStoresOfSettledAsync(executionId: ExecutionId): void { + function notifyStoresOfResolvedAsync( + store: Store, + executionId: ExecutionId, + ): void { + if (isLatestExecution(store, executionId)) { + clearExecutionInfo(store); + } const stores = waitingStores.get(executionId); if (stores !== undefined) { - for (const store of stores) { - markRecoilValueModified(store, nullthrows(recoilValue)); + for (const waitingStore of stores) { + markRecoilValueModified(waitingStore, nullthrows(recoilValue)); } waitingStores.delete(executionId); } } + function markStoreWaitingForResolvedAsync( + store: Store, + executionId: ExecutionId, + ): void { + let stores = waitingStores.get(executionId); + if (stores == null) { + waitingStores.set(executionId, (stores = new Set())); + } + stores.add(store); + } + function getCachedNodeLoadable( store: Store, state: TreeState, @@ -374,29 +375,22 @@ function selector( .then(value => { if (!selectorIsLive()) { // The selector was released since the request began; ignore the response. - clearExecutionInfo(store, executionId); + clearExecutionInfo(store); throw CANCELED; } const loadable = loadableWithValue(value); - - maybeFreezeValue(value); - setCache(state, depValuesToDepRoute(depValues), loadable); - setDepsInStore(store, state, new Set(depValues.keys()), executionId); - setLoadableInStoreToNotifyDeps(store, loadable, executionId); - + resolveAsync(store, state, executionId, loadable, depValues); return value; }) .catch(errorOrPromise => { if (!selectorIsLive()) { // The selector was released since the request began; ignore the response. - clearExecutionInfo(store, executionId); + clearExecutionInfo(store); throw CANCELED; } - if (isLatestExecution(store, executionId)) { - updateExecutionInfoDepValues(depValues, store, executionId); - } + updateExecutionInfoDepValues(store, executionId, depValues); if (isPromise(errorOrPromise)) { return wrapPendingDependencyPromise( @@ -410,12 +404,7 @@ function selector( } const loadable = loadableWithError(errorOrPromise); - - maybeFreezeValue(errorOrPromise); - setCache(state, depValuesToDepRoute(depValues), loadable); - setDepsInStore(store, state, new Set(depValues.keys()), executionId); - setLoadableInStoreToNotifyDeps(store, loadable, executionId); - + resolveAsync(store, state, executionId, loadable, depValues); throw errorOrPromise; }); } @@ -462,7 +451,7 @@ function selector( .then(resolvedDep => { if (!selectorIsLive()) { // The selector was released since the request began; ignore the response. - clearExecutionInfo(store, executionId); + clearExecutionInfo(store); throw CANCELED; } @@ -507,7 +496,7 @@ function selector( * is executing may be a discarded tree (so store.getGraph(state.version) * will be empty), and the full dep tree may not be in the selector * caches in the case where the selector's cache was cleared. To solve - * for this we would have to keep track of all running selector + * for this we would have to keep track of all running selector * executions and their downstream deps. Because this only covers edge * cases, that complexity might not be justifyable. */ @@ -558,10 +547,9 @@ function selector( */ if ( isLatestExecution(store, executionId) || - getExecutionInfo(store).latestExecutionId == null + getExecutionInfo(store) == null ) { - setExecutionInfo(cachedLoadable, store); - notifyStoresOfSettledAsync(executionId); + notifyStoresOfResolvedAsync(store, executionId); } if (cachedLoadable.state === 'hasValue') { @@ -597,7 +585,7 @@ function selector( */ if (!isLatestExecution(store, executionId)) { const executionInfo = getExecutionInfoOfInProgressExecution(state); - if (executionInfo?.latestLoadable?.state === 'loading') { + if (executionInfo?.latestLoadable.state === 'loading') { /** * Returning promise here without wrapping as the wrapper logic was * already done upstream when this promise was generated. @@ -613,25 +601,15 @@ function selector( executionId, ); - if (isLatestExecution(store, executionId)) { - updateExecutionInfoDepValues(depValues, store, executionId); - } + updateExecutionInfoDepValues(store, executionId, depValues); if (loadable.state !== 'loading') { - setCache(state, depValuesToDepRoute(depValues), loadable); - setDepsInStore(store, state, new Set(depValues.keys()), executionId); - setLoadableInStoreToNotifyDeps(store, loadable, executionId); + resolveAsync(store, state, executionId, loadable, depValues); } if (loadable.state === 'hasError') { throw loadable.contents; } - - /** - * Returning any promises here without wrapping as the wrapepr logic was - * already done when we called evaluateSelectorGetter() to get this - * loadable - */ return loadable.contents; }) .catch(error => { @@ -640,36 +618,16 @@ function selector( throw CANCELED; } if (!selectorIsLive()) { - clearExecutionInfo(store, executionId); + clearExecutionInfo(store); throw CANCELED; } const loadable = loadableWithError(error); - - maybeFreezeValue(error); - setCache( - state, - depValuesToDepRoute(existingDeps), - loadableWithError(error), - ); - setDepsInStore(store, state, new Set(existingDeps.keys()), executionId); - setLoadableInStoreToNotifyDeps(store, loadable, executionId); - + resolveAsync(store, state, executionId, loadable, existingDeps); throw error; }); } - function setLoadableInStoreToNotifyDeps( - store: Store, - loadable: Loadable, - executionId: ExecutionId, - ): void { - if (isLatestExecution(store, executionId)) { - setExecutionInfo(loadable, store); - notifyStoresOfSettledAsync(executionId); - } - } - function setDepsInStore( store: Store, state: TreeState, @@ -822,10 +780,6 @@ function selector( loadable = loadableWithValue(result); } - if (loadable.state !== 'loading') { - maybeFreezeValue(loadable.contents); - } - return [loadable, depValues]; } @@ -876,7 +830,7 @@ function selector( store, state, depsAfterCacheDone, - executionInfo.latestExecutionId, + executionInfo?.latestExecutionId, ); } @@ -924,12 +878,13 @@ function selector( * partial list of deps-- we need the full list of deps to ensure that we * are returning the correct result from cache. */ - if (loadable.state !== 'loading') { - setCache(state, depValuesToDepRoute(newDepValues), loadable); + if (loadable.state === 'loading') { + setExecutionInfo(store, newExecutionId, loadable, newDepValues, state); + markStoreWaitingForResolvedAsync(store, newExecutionId); } else { - notifyStoreWhenAsyncSettles(store, newExecutionId); + clearExecutionInfo(store); + setCache(state, loadable, newDepValues); } - setExecutionInfo(loadable, store, newDepValues, newExecutionId, state); return loadable; } @@ -960,7 +915,7 @@ function selector( const cachedVal = getValFromCacheAndUpdatedDownstreamDeps(store, state); if (cachedVal != null) { - setExecutionInfo(cachedVal, store); + clearExecutionInfo(store); return cachedVal; } @@ -970,7 +925,7 @@ function selector( // FIXME: this won't work with custom caching b/c it uses separate cache if (inProgressExecutionInfo != null) { if (inProgressExecutionInfo.latestLoadable?.state === 'loading') { - notifyStoreWhenAsyncSettles( + markStoreWaitingForResolvedAsync( store, nullthrows(inProgressExecutionInfo.latestExecutionId), ); @@ -1008,7 +963,7 @@ function selector( const executionInfo = getExecutionInfo(store); const oldDepValues = - executionInfo.depValuesDiscoveredSoFarDuringAsyncWork ?? new Map(); + executionInfo?.depValuesDiscoveredSoFarDuringAsyncWork ?? new Map(); const cachedDepValuesCheckedForThisVersion = Array( (mapOfCheckedVersions.get(state.version) ?? new Map()).entries(), @@ -1023,7 +978,7 @@ function selector( if ( oldDepValues == null || - state.version === executionInfo.stateVersion || + state.version === executionInfo?.stateVersion || isCachedVersionSame ) { return false; @@ -1038,6 +993,10 @@ function selector( }); } + function getExecutionInfo(store: Store): ?ExecutionInfo { + return executionInfoMap.get(store); + } + /** * This function will update the selector's execution info when the selector * has either finished running an execution or has started a new execution. If @@ -1046,65 +1005,58 @@ function selector( * just finished. */ function setExecutionInfo( - loadable: Loadable, store: Store, - depValues?: DepValues, - newExecutionId?: ExecutionId, - state?: TreeState, + newExecutionId: ExecutionId, + loadable: LoadingLoadableType, + depValues: DepValues, + state: TreeState, ) { - const executionInfo = getExecutionInfo(store); - - if (loadable.state === 'loading') { - executionInfo.depValuesDiscoveredSoFarDuringAsyncWork = depValues; - executionInfo.latestExecutionId = newExecutionId; - executionInfo.latestLoadable = loadable; - executionInfo.stateVersion = state?.version; - } else { - executionInfo.depValuesDiscoveredSoFarDuringAsyncWork = null; - executionInfo.latestExecutionId = null; - executionInfo.latestLoadable = null; - executionInfo.stateVersion = null; - } + executionInfoMap.set(store, { + depValuesDiscoveredSoFarDuringAsyncWork: depValues, + latestExecutionId: newExecutionId, + latestLoadable: loadable, + stateVersion: state.version, + }); } function updateExecutionInfoDepValues( - depValues: DepValues, store: Store, executionId: ExecutionId, + depValues: DepValues, ) { if (isLatestExecution(store, executionId)) { const executionInfo = getExecutionInfo(store); - executionInfo.depValuesDiscoveredSoFarDuringAsyncWork = depValues; + if (executionInfo != null) { + executionInfo.depValuesDiscoveredSoFarDuringAsyncWork = depValues; + } } } - function clearExecutionInfo(store: Store, executionId: ExecutionId) { - if (isLatestExecution(store, executionId)) { - executionInfoMap.delete(store); - } + function clearExecutionInfo(store: Store) { + executionInfoMap.delete(store); } function isLatestExecution(store: Store, executionId): boolean { - const executionInfo = getExecutionInfo(store); - return executionId === executionInfo.latestExecutionId; - } - - function maybeFreezeValue(val) { - if (__DEV__) { - if (Boolean(options.dangerouslyAllowMutability) === false) { - deepFreezeValue(val); - } - } + return executionId === getExecutionInfo(store)?.latestExecutionId; } function setCache( state: TreeState, - cacheRoute: NodeCacheRoute, loadable: Loadable, + depValues: DepValues, ) { + if (__DEV__) { + if ( + loadable.state !== 'loading' && + Boolean(options.dangerouslyAllowMutability) === false + ) { + deepFreezeValue(loadable.contents); + } + } + state.atomValues.set(key, loadable); try { - cache.set(cacheRoute, loadable); + cache.set(depValuesToDepRoute(depValues), loadable); } catch (error) { throw err( `Problem with setting cache for selector "${key}": ${error.message}`, diff --git a/packages/recoil/recoil_values/__tests__/Recoil_selectorHooks-test.js b/packages/recoil/recoil_values/__tests__/Recoil_selectorHooks-test.js index 40d7132..778d034 100644 --- a/packages/recoil/recoil_values/__tests__/Recoil_selectorHooks-test.js +++ b/packages/recoil/recoil_values/__tests__/Recoil_selectorHooks-test.js @@ -28,6 +28,7 @@ let React, Profiler, act, batchUpdates, + RecoilRoot, atom, constSelector, errorSelector, @@ -40,6 +41,7 @@ let React, flushPromisesAndTimers, renderElements, renderElementsWithSuspenseCount, + componentThatReadsAndWritesAtom, useRecoilState, useRecoilValue, useRecoilValueLoadable, @@ -56,6 +58,7 @@ const testRecoil = getRecoilTestFn(() => { ({act} = require('ReactTestUtils')); ({batchUpdates} = require('../../core/Recoil_Batching')); + ({RecoilRoot} = require('../../core/Recoil_RecoilRoot')); atom = require('../../recoil_values/Recoil_atom'); constSelector = require('../../recoil_values/Recoil_constSelector'); errorSelector = require('../../recoil_values/Recoil_errorSelector'); @@ -69,6 +72,7 @@ const testRecoil = getRecoilTestFn(() => { flushPromisesAndTimers, renderElements, renderElementsWithSuspenseCount, + componentThatReadsAndWritesAtom, } = require('recoil-shared/__test_utils__/Recoil_TestingUtils')); ({reactMode} = require('../../core/Recoil_ReactMode')); ({ @@ -1660,6 +1664,197 @@ testRecoil( }, ); +describe('Multiple stores', () => { + testRecoil('sync in multiple', () => { + const myAtom = atom({key: 'selector stores sync atom', default: 'DEFAULT'}); + const mySelector = selector({ + key: 'selector stores sync selector', + get: () => myAtom, + set: ({set}, newValue) => set(myAtom, newValue), + }); + + const [ComponentA, setAtomA] = componentThatReadsAndWritesAtom(mySelector); + const [ComponentB, setAtomB] = componentThatReadsAndWritesAtom(mySelector); + const c = renderElements( + <> + + + + + + + , + ); + + expect(c.textContent).toBe('"DEFAULT""DEFAULT"'); + + act(() => setAtomA('A')); + expect(c.textContent).toBe('"A""DEFAULT"'); + + act(() => setAtomB('B')); + expect(c.textContent).toBe('"A""B"'); + }); + + testRecoil('async in multiple', async () => { + const resolvers = {}; + const promises = { + DEFAULT: new Promise(resolve => { + resolvers.DEFAULT = resolve; + }), + STALE: new Promise(resolve => { + resolvers.STALE = resolve; + }), + UPDATE: new Promise(resolve => { + resolvers.UPDATE = resolve; + }), + }; + const myAtom = atom({ + key: 'selector stores async atom', + default: 'DEFAULT', + }); + const mySelector = selector({ + key: 'selector stores async selector', + get: async ({get}) => { + const side = get(myAtom); + const str = await promises[side]; + return side + ':' + str; + }, + set: ({set}, newValue) => set(myAtom, newValue), + }); + + const [ComponentA, setAtomA] = componentThatReadsAndWritesAtom(mySelector); + const [ComponentB, setAtomB] = componentThatReadsAndWritesAtom(mySelector); + const c = renderElements( + <> + + LOADING_A}> + + + + + LOADING_B}> + + + + , + ); + + expect(c.textContent).toBe('LOADING_ALOADING_B'); + + act(() => setAtomA('STALE')); + expect(c.textContent).toBe('LOADING_ALOADING_B'); + + act(() => setAtomA('UPDATE')); + expect(c.textContent).toBe('LOADING_ALOADING_B'); + + act(() => resolvers.STALE('STALE')); + await flushPromisesAndTimers(); + await flushPromisesAndTimers(); + expect(c.textContent).toBe('LOADING_ALOADING_B'); + + act(() => resolvers.UPDATE('RESOLVE_A')); + await flushPromisesAndTimers(); + await flushPromisesAndTimers(); + expect(c.textContent).toBe('"UPDATE:RESOLVE_A"LOADING_B'); + + act(() => resolvers.DEFAULT('RESOLVE_B')); + await flushPromisesAndTimers(); + await flushPromisesAndTimers(); + expect(c.textContent).toBe('"UPDATE:RESOLVE_A""DEFAULT:RESOLVE_B"'); + + act(() => setAtomB('UPDATE')); + expect(c.textContent).toBe('"UPDATE:RESOLVE_A""UPDATE:RESOLVE_A"'); + }); + + testRecoil('derived in multiple', async () => { + let resolveA; + const atomA = atom({ + key: 'selector stores derived atom A', + default: new Promise(resolve => { + resolveA = resolve; + }), + }); + let resolveB; + const atomB = atom({ + key: 'selector stores derived atom B', + default: new Promise(resolve => { + resolveB = resolve; + }), + }); + let resolveStale; + const atomStale = atom({ + key: 'selector stores derived atom Stale', + default: new Promise(resolve => { + resolveStale = resolve; + }), + }); + const switchAtom = atom({ + key: 'selector stores derived atom Switch', + default: 'A', + }); + + const mySelector = selector({ + key: 'selector stores derived selector', + get: async ({get}) => { + const side = get(switchAtom); + return ( + side + + ':' + + (side === 'STALE' + ? get(atomStale) + : side === 'A' + ? get(atomA) + : get(atomB)) + ); + }, + set: ({set}, newValue) => set(switchAtom, newValue), + }); + + const [ComponentA, setAtomA] = componentThatReadsAndWritesAtom(mySelector); + const [ComponentB, setAtomB] = componentThatReadsAndWritesAtom(mySelector); + const c = renderElements( + <> + + LOADING_A}> + + + + + LOADING_B}> + + + + , + ); + + expect(c.textContent).toBe('LOADING_ALOADING_B'); + + act(() => setAtomB('STALE')); + expect(c.textContent).toBe('LOADING_ALOADING_B'); + + act(() => setAtomB('B')); + expect(c.textContent).toBe('LOADING_ALOADING_B'); + + act(() => resolveStale('STALE')); + await flushPromisesAndTimers(); + await flushPromisesAndTimers(); + expect(c.textContent).toBe('LOADING_ALOADING_B'); + + act(() => resolveB('RESOLVE_B')); + await flushPromisesAndTimers(); + await flushPromisesAndTimers(); + expect(c.textContent).toBe('LOADING_A"B:RESOLVE_B"'); + + act(() => resolveA('RESOLVE_A')); + await flushPromisesAndTimers(); + await flushPromisesAndTimers(); + expect(c.textContent).toBe('"A:RESOLVE_A""B:RESOLVE_B"'); + + act(() => setAtomA('B')); + expect(c.textContent).toBe('"B:RESOLVE_B""B:RESOLVE_B"'); + }); +}); + describe('Counts', () => { describe('Evaluation', () => { testRecoil('Selector functions are evaluated just once', () => {