diff --git a/CHANGELOG.md b/CHANGELOG.md index 515ffaf9d..317c10cfc 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -3,12 +3,13 @@ ## UPCOMING **_Add new changes here as they land_** -- Selector cache lookup optimization (#1720) +- Selector cache lookup optimizations (#1720, #1736) +- Allow async selectors to re-evaluate when async dependencies are discovered with stale state (#1736) ## 0.7.1 (2022-04-12) ### Typing -- Add explict `children` prop to `` and `useRecoilBridgeAcrossReactRoots_UNSTABLE()` for TypeScript for `@types/react` with React 18 (#1718, #1717, #1726, #1731) +- Add explicit `children` prop to `` and `useRecoilBridgeAcrossReactRoots_UNSTABLE()` for TypeScript for `@types/react` with React 18 (#1718, #1717, #1726, #1731) - Update typing for family parameters to better support Map, Set, and classes with `toJSON()`. (#1709, #1703) ### Fixes diff --git a/packages/recoil/hooks/__tests__/Recoil_useRecoilSnapshot-test.js b/packages/recoil/hooks/__tests__/Recoil_useRecoilSnapshot-test.js index a7fbced4c..ed5a83ea9 100644 --- a/packages/recoil/hooks/__tests__/Recoil_useRecoilSnapshot-test.js +++ b/packages/recoil/hooks/__tests__/Recoil_useRecoilSnapshot-test.js @@ -156,7 +156,38 @@ testRecoil('useRecoilSnapshot - goto snapshots', ({strictMode}) => { }); testRecoil( - 'useRecoilSnapshot - async selectors', + 'useRecoilSnapshot - async selector', + async ({strictMode, concurrentMode}) => { + const [mySelector, resolve] = asyncSelector(); + + const snapshots = []; + function RecoilSnapshotAndSubscribe() { + const snapshot = useRecoilSnapshot(); + snapshot.retain(); + useEffect(() => { + snapshots.push(snapshot); + }, [snapshot]); + return null; + } + + renderElements(); + expect(snapshots.length).toEqual(strictMode && concurrentMode ? 2 : 1); + + act(() => resolve('RESOLVE')); + expect(snapshots.length).toEqual(strictMode && concurrentMode ? 2 : 1); + + // On the first request the selector is unresolved and returns the promise + await expect( + snapshots[0].getLoadable(mySelector).contents, + ).resolves.toEqual('RESOLVE'); + + // On the second request the resolved value is cached. + expect(snapshots[0].getLoadable(mySelector).contents).toEqual('RESOLVE'); + }, +); + +testRecoil( + 'useRecoilSnapshot - cloned async selector', async ({strictMode, concurrentMode}) => { const [mySelector, resolve] = asyncSelector(); @@ -188,6 +219,7 @@ testRecoil( expect(c.textContent).toEqual('"RESOLVE"'); expect(snapshots.length).toEqual(strictMode && concurrentMode ? 3 : 2); + // Snapshot contains cached result since it was cloned after resolved expect(snapshots[0].getLoadable(mySelector).contents).toEqual('RESOLVE'); }, ); diff --git a/packages/recoil/recoil_values/Recoil_selector.js b/packages/recoil/recoil_values/Recoil_selector.js index 1c1b86bbf..730e4ce8e 100644 --- a/packages/recoil/recoil_values/Recoil_selector.js +++ b/packages/recoil/recoil_values/Recoil_selector.js @@ -192,13 +192,19 @@ type ExecutionID = number; * executions running, but there is only one 'latest' execution, which * represents the execution that will make its way to the UI and make updates * to global state when it finishes. + * 4. The set of stateVersions which have already been tested as valid for this + * evalution. This is an optimization to avoid having to transitively + * check if any deps have changed for a state we have aleady checked. + * If additional async dependencies are discovered later, they may have + * different values in different stores/states, so this will have to be + * cleared. */ type ExecutionInfo = { // This is mutable and updated as new deps are discovered depValuesDiscoveredSoFarDuringAsyncWork: DepValues, loadingLoadable: LoadingLoadableType, executionID: ExecutionID, - stateVersion: StateID, + stateVersions: Map, }; // An object to hold the current state for loading dependencies for a particular @@ -216,8 +222,8 @@ const dependencyStack = []; // for detecting circular dependencies. const waitingStores: Map> = new Map(); const getNewExecutionID: () => ExecutionID = (() => { - let executionId = 0; - return () => executionId++; + let executionID = 0; + return () => executionID++; })(); /* eslint-disable no-redeclare */ @@ -283,37 +289,67 @@ function selector( function resolveAsync( store: Store, state: TreeState, - executionId: ExecutionID, + executionID: ExecutionID, loadable: Loadable, depValues: DepValues, ): void { setCache(state, loadable, depValues); - notifyStoresOfResolvedAsync(store, executionId); + notifyStoresOfResolvedAsync(store, executionID); } function notifyStoresOfResolvedAsync( store: Store, - executionId: ExecutionID, + executionID: ExecutionID, ): void { - if (isLatestExecution(store, executionId)) { + if (isLatestExecution(store, executionID)) { clearExecutionInfo(store); } - const stores = waitingStores.get(executionId); - if (stores !== undefined) { + notifyWaitingStores(executionID, true); + } + + /** + * Notify stores to pull the selector again if a new async dep was discovered. + * 1) Async selector adds a new dep but doesn't resolve yet. + * Note that deps for an async selector are based on the state when the + * evaluation started, in order to provide a consistent picture of state. + * 2) But, new value of dep based on the current state might cause the selector + * to resolve or resolve differently. + * 3) Therefore, this notification will pull the selector based on the current + * state for the components + */ + function notifyStoresOfNewAsyncDep( + store: Store, + executionID: ExecutionID, + ): void { + if (isLatestExecution(store, executionID)) { + const executionInfo = nullthrows(getExecutionInfo(store)); + executionInfo.stateVersions.clear(); + notifyWaitingStores(executionID, false); + } + } + + function notifyWaitingStores( + executionID: ExecutionID, + clearWaitlist: boolean, + ) { + const stores = waitingStores.get(executionID); + if (stores != null) { for (const waitingStore of stores) { markRecoilValueModified(waitingStore, nullthrows(recoilValue)); } - waitingStores.delete(executionId); + if (clearWaitlist) { + waitingStores.delete(executionID); + } } } function markStoreWaitingForResolvedAsync( store: Store, - executionId: ExecutionID, + executionID: ExecutionID, ): void { - let stores = waitingStores.get(executionId); + let stores = waitingStores.get(executionID); if (stores == null) { - waitingStores.set(executionId, (stores = new Set())); + waitingStores.set(executionID, (stores = new Set())); } stores.add(store); } @@ -352,7 +388,7 @@ function selector( promise: Promise, state: TreeState, depValues: DepValues, - executionId: ExecutionID, + executionID: ExecutionID, loadingDepsState: LoadingDepsState, ): Promise { return promise @@ -364,7 +400,7 @@ function selector( } const loadable = loadableWithValue(value); - resolveAsync(store, state, executionId, loadable, depValues); + resolveAsync(store, state, executionID, loadable, depValues); return value; }) .catch(errorOrPromise => { @@ -380,13 +416,13 @@ function selector( errorOrPromise, state, depValues, - executionId, + executionID, loadingDepsState, ); } const loadable = loadableWithError(errorOrPromise); - resolveAsync(store, state, executionId, loadable, depValues); + resolveAsync(store, state, executionID, loadable, depValues); throw errorOrPromise; }); } @@ -426,7 +462,7 @@ function selector( promise: Promise, state: TreeState, existingDeps: DepValues, - executionId: ExecutionID, + executionID: ExecutionID, loadingDepsState: LoadingDepsState, ): Promise { return promise @@ -525,10 +561,10 @@ function selector( * necessary with recoil_transition_support mode. */ if ( - isLatestExecution(store, executionId) || + isLatestExecution(store, executionID) || getExecutionInfo(store) == null ) { - notifyStoresOfResolvedAsync(store, executionId); + notifyStoresOfResolvedAsync(store, executionID); } if (cachedLoadable.state === 'hasValue') { @@ -562,7 +598,7 @@ function selector( * future we can make the behavior configurable. An ideal fix may be * to extend the tree cache to support caching loading states. */ - if (!isLatestExecution(store, executionId)) { + if (!isLatestExecution(store, executionID)) { const executionInfo = getInProgressExecutionInfo(store, state); if (executionInfo != null) { /** @@ -577,11 +613,11 @@ function selector( const [loadable, depValues] = evaluateSelectorGetter( store, state, - executionId, + executionID, ); if (loadable.state !== 'loading') { - resolveAsync(store, state, executionId, loadable, depValues); + resolveAsync(store, state, executionID, loadable, depValues); } if (loadable.state === 'hasError') { @@ -600,7 +636,7 @@ function selector( } const loadable = loadableWithError(error); - resolveAsync(store, state, executionId, loadable, existingDeps); + resolveAsync(store, state, executionID, loadable, existingDeps); throw error; }); } @@ -609,10 +645,10 @@ function selector( store: Store, state: TreeState, deps: $ReadOnlySet, - executionId: ?ExecutionID, + executionID: ?ExecutionID, ): void { if ( - isLatestExecution(store, executionId) || + isLatestExecution(store, executionID) || state.version === store.getState()?.currentTree?.version || state.version === store.getState()?.nextTree?.version ) { @@ -632,7 +668,7 @@ function selector( function evaluateSelectorGetter( store: Store, state: TreeState, - executionId: ExecutionID, + executionID: ExecutionID, ): [Loadable, DepValues] { const endPerfBlock = startPerfBlock(key); // TODO T63965866: use execution ID here let duringSynchronousExecution = true; @@ -671,7 +707,8 @@ function selector( // knows if it has to restart evaluation if one of them is updated before // the asynchronous selector completely resolves. if (!duringSynchronousExecution) { - updateDeps(store, state, new Set(depValues.keys()), executionId); + updateDeps(store, state, new Set(depValues.keys()), executionID); + notifyStoresOfNewAsyncDep(store, executionID); } switch (depLoadable.state) { @@ -723,7 +760,7 @@ function selector( result, state, depValues, - executionId, + executionID, loadingDepsState, ).finally(finishEvaluation); } else { @@ -740,7 +777,7 @@ function selector( result, state, depValues, - executionId, + executionID, loadingDepsState, ).finally(finishEvaluation); } else { @@ -758,8 +795,8 @@ function selector( } duringSynchronousExecution = false; - updateExecutionInfoDepValues(store, executionId, depValues); - updateDeps(store, state, new Set(depValues.keys()), executionId); + updateExecutionInfoDepValues(store, executionID, depValues); + updateDeps(store, state, new Set(depValues.keys()), executionID); return [loadable, depValues]; } @@ -927,12 +964,16 @@ function selector( for (const execInfo of pendingExecutions) { if ( - // If this execution is on the same version of state, then it's valid - state.version === execInfo.stateVersion || + // If this execution was already checked to be valid with this version + // of state, then let's use it! + execInfo.stateVersions.get(state.version) || // If the deps for the execution match our current state, then it's valid !anyDepChanged(execInfo.depValuesDiscoveredSoFarDuringAsyncWork) ) { + execInfo.stateVersions.set(state.version, true); return execInfo; + } else { + execInfo.stateVersions.set(state.version, false); } } @@ -961,18 +1002,18 @@ function selector( depValuesDiscoveredSoFarDuringAsyncWork: depValues, executionID: newExecutionID, loadingLoadable: loadable, - stateVersion: state.version, + stateVersions: new Map([[state.version, true]]), }); } function updateExecutionInfoDepValues( store: Store, - executionId: ExecutionID, + executionID: ExecutionID, depValues: DepValues, ) { // We only need to bother updating the deps for the latest execution because // that's all getInProgressExecutionInfo() will be looking for. - if (isLatestExecution(store, executionId)) { + if (isLatestExecution(store, executionID)) { const executionInfo = getExecutionInfo(store); if (executionInfo != null) { executionInfo.depValuesDiscoveredSoFarDuringAsyncWork = depValues; @@ -984,8 +1025,8 @@ function selector( executionInfoMap.delete(store); } - function isLatestExecution(store: Store, executionId): boolean { - return executionId === getExecutionInfo(store)?.executionID; + function isLatestExecution(store: Store, executionID): boolean { + return executionID === getExecutionInfo(store)?.executionID; } /** diff --git a/packages/recoil/recoil_values/__tests__/Recoil_selectorHooks-test.js b/packages/recoil/recoil_values/__tests__/Recoil_selectorHooks-test.js index 4bcde349f..d84dd10a0 100644 --- a/packages/recoil/recoil_values/__tests__/Recoil_selectorHooks-test.js +++ b/packages/recoil/recoil_values/__tests__/Recoil_selectorHooks-test.js @@ -412,7 +412,7 @@ describe('Updates', () => { ); expect(container.textContent).toEqual('loading'); await flushPromisesAndTimers(); - await flushPromisesAndTimers(); + await flushPromisesAndTimers(); // Double flush for open source environment expect(container.textContent).toEqual('error'); }, ); @@ -431,7 +431,7 @@ describe('Updates', () => { expect(container.textContent).toEqual('loading'); act(() => reject(new Error())); await flushPromisesAndTimers(); - await flushPromisesAndTimers(); + await flushPromisesAndTimers(); // Double flush for open source environment expect(container.textContent).toEqual('error'); }, ); @@ -632,7 +632,7 @@ describe('Dynamic Dependencies', () => { expect(container.textContent).toEqual('loading'); await flushPromisesAndTimers(); - await flushPromisesAndTimers(); // HACK: not sure why but this is needed in OSS + await flushPromisesAndTimers(); // Double flush for open source environment expect(container.textContent).toEqual('Async Dep Value'); act(() => setAtom('CHANGED Async Dep')); @@ -1268,6 +1268,7 @@ describe('Async Selectors', () => { expect(el.textContent).toEqual('loading'); await flushPromisesAndTimers(); + await flushPromisesAndTimers(); // Double flush for open source environment expect(el.textContent).toEqual('"READY"'); }); @@ -1423,7 +1424,7 @@ describe('Async Selectors', () => { expect(resolvers.length).toBe(2); resolvers[1][0]('hello'); await flushPromisesAndTimers(); - await flushPromisesAndTimers(); + await flushPromisesAndTimers(); // Double flush for open source environment expect(container.textContent).toEqual('"hello"'); // Cause sel to have no subscribers: @@ -1458,7 +1459,7 @@ describe('Async Selectors', () => { // When the faster second request resolves, we should see its result: resolvers[1][0]('hello'); await flushPromisesAndTimers(); - await flushPromisesAndTimers(); + await flushPromisesAndTimers(); // Double flush for open source environment expect(container.textContent).toEqual('"hello"'); }); @@ -1479,12 +1480,14 @@ describe('Async Selectors', () => { , ); + await flushPromisesAndTimers(); - await flushPromisesAndTimers(); + await flushPromisesAndTimers(); // Double flush for open source environment expect(container.textContent).toEqual('1'); + act(() => updateValue(1)); await flushPromisesAndTimers(); - await flushPromisesAndTimers(); + await flushPromisesAndTimers(); // Double flush for open source environment expect(container.textContent).toEqual('2'); }); @@ -1620,84 +1623,81 @@ describe('Async Selectors', () => { act(() => resolve('bar')); await act(flushPromisesAndTimers); - await act(flushPromisesAndTimers); + await act(flushPromisesAndTimers); // Double flush for open source environment expect(c.textContent).toEqual('bar'); }); - testRecoil( - 'Selectors read in a another root notify all roots', - async () => { - // This version of the test uses another RecoilRoot as its second store - const switchAtom = atom({ - key: 'notifiesAllStores/twoRoots/switch', - default: false, - }); - - const selectorA = selector({ - key: 'notifiesAllStores/twoRoots/a', - get: () => 'SELECTOR A', - }); + testRecoil('Selectors read in another root notify all roots', async () => { + // This version of the test uses another RecoilRoot as its second store + const switchAtom = atom({ + key: 'notifiesAllStores/twoRoots/switch', + default: false, + }); - let resolve: $FlowFixMe = _ => { - throw new Error('error in test'); - }; - const selectorB = selector({ - key: 'notifiesAllStores/twoRoots/b', - get: async () => - new Promise(r => { - resolve = r; - }), - }); + const selectorA = selector({ + key: 'notifiesAllStores/twoRoots/a', + get: () => 'SELECTOR A', + }); - function TestComponent({ - setSwitch, - }: { - setSwitch: ((boolean) => void) => void, - }) { - const [shouldQuery, setShouldQuery] = useRecoilState(switchAtom); - const query = useRecoilValueLoadable( - shouldQuery ? selectorB : selectorA, - ); - setSwitch(setShouldQuery); - return query.state === 'hasValue' ? query.contents : 'loading'; - } + let resolve: string => void = () => { + throw new Error('error in test'); + }; + const selectorB = selector({ + key: 'notifiesAllStores/twoRoots/b', + get: async () => + new Promise(r => { + resolve = r; + }), + }); - let setRootASelector; - const rootA = renderElements( - { - setRootASelector = setSelector; - }} - />, - ); - let setRootBSelector; - const rootB = renderElements( - { - setRootBSelector = setSelector; - }} - />, + function TestComponent({ + setSwitch, + }: { + setSwitch: ((boolean) => void) => void, + }) { + const [shouldQuery, setShouldQuery] = useRecoilState(switchAtom); + const query = useRecoilValueLoadable( + shouldQuery ? selectorB : selectorA, ); + setSwitch(setShouldQuery); + return query.state === 'hasValue' ? query.contents : 'loading'; + } - expect(rootA.textContent).toEqual('SELECTOR A'); - expect(rootB.textContent).toEqual('SELECTOR A'); + let setRootASelector; + const rootA = renderElements( + { + setRootASelector = setSelector; + }} + />, + ); + let setRootBSelector; + const rootB = renderElements( + { + setRootBSelector = setSelector; + }} + />, + ); - act(() => setRootASelector(true)); // cause rootA to read the selector - expect(rootA.textContent).toEqual('loading'); - expect(rootB.textContent).toEqual('SELECTOR A'); + expect(rootA.textContent).toEqual('SELECTOR A'); + expect(rootB.textContent).toEqual('SELECTOR A'); - act(() => setRootBSelector(true)); // cause rootB to read the selector - expect(rootA.textContent).toEqual('loading'); - expect(rootB.textContent).toEqual('loading'); + act(() => setRootASelector(true)); // cause rootA to read the selector + expect(rootA.textContent).toEqual('loading'); + expect(rootB.textContent).toEqual('SELECTOR A'); - act(() => resolve('SELECTOR B')); + act(() => setRootBSelector(true)); // cause rootB to read the selector + expect(rootA.textContent).toEqual('loading'); + expect(rootB.textContent).toEqual('loading'); - await flushPromisesAndTimers(); + act(() => resolve('SELECTOR B')); - expect(rootA.textContent).toEqual('SELECTOR B'); - expect(rootB.textContent).toEqual('SELECTOR B'); - }, - ); + await flushPromisesAndTimers(); + + expect(rootA.textContent).toEqual('SELECTOR B'); + expect(rootB.textContent).toEqual('SELECTOR B'); + }); }); }); @@ -1908,17 +1908,17 @@ describe('Multiple stores', () => { act(() => resolvers.STALE('STALE')); await flushPromisesAndTimers(); - await flushPromisesAndTimers(); + await flushPromisesAndTimers(); // Double flush for open source environment expect(c.textContent).toBe('LOADING_ALOADING_B'); act(() => resolvers.UPDATE('RESOLVE_A')); await flushPromisesAndTimers(); - await flushPromisesAndTimers(); + await flushPromisesAndTimers(); // Double flush for open source environment expect(c.textContent).toBe('"UPDATE:RESOLVE_A"LOADING_B'); act(() => resolvers.DEFAULT('RESOLVE_B')); await flushPromisesAndTimers(); - await flushPromisesAndTimers(); + await flushPromisesAndTimers(); // Double flush for open source environment expect(c.textContent).toBe('"UPDATE:RESOLVE_A""DEFAULT:RESOLVE_B"'); act(() => setAtomB('UPDATE')); @@ -1996,17 +1996,17 @@ describe('Multiple stores', () => { act(() => resolveStale('STALE')); await flushPromisesAndTimers(); - await flushPromisesAndTimers(); + await flushPromisesAndTimers(); // Double flush for open source environment expect(c.textContent).toBe('LOADING_ALOADING_B'); act(() => resolveB('RESOLVE_B')); await flushPromisesAndTimers(); - await flushPromisesAndTimers(); + await flushPromisesAndTimers(); // Double flush for open source environment expect(c.textContent).toBe('LOADING_A"B:RESOLVE_B"'); act(() => resolveA('RESOLVE_A')); await flushPromisesAndTimers(); - await flushPromisesAndTimers(); + await flushPromisesAndTimers(); // Double flush for open source environment expect(c.textContent).toBe('"A:RESOLVE_A""B:RESOLVE_B"'); act(() => setAtomA('B')); @@ -2083,7 +2083,7 @@ describe('Multiple stores', () => { // Resolving store B act(() => resolvers.OTHER('OTHER')); await flushPromisesAndTimers(); - await flushPromisesAndTimers(); + await flushPromisesAndTimers(); // Double flush for open source environment expect(c.textContent).toBe('"SET""loading""OTHER:OTHER"'); // Resolving store A @@ -2160,16 +2160,17 @@ describe('Multiple stores', () => { testRecoil('Diverging shared selectors', async () => { const myAtom = stringAtom(); atom({ - key: 'selector stores nested atom', + key: 'selector stores diverging atom', default: 'DEFAULT', }); const mySelector = selector({ - key: 'selector stores nested atom inner', + key: 'selector stores diverging selector', get: async ({get}) => { await Promise.resolve(); const value = get(myAtom); + await Promise.resolve(); // So resolution occurs during act() if (value === 'RESOLVE') { return value; } @@ -2211,13 +2212,84 @@ describe('Multiple stores', () => { setAtomA('SETA'); }); await flushPromisesAndTimers(); + await flushPromisesAndTimers(); // Double flush for open source environment + expect(c.textContent).toBe('LOAD_A LOAD_B '); + + act(() => { + setAtomB('RESOLVE'); + }); await flushPromisesAndTimers(); + await flushPromisesAndTimers(); // Double flush for open source environment + expect(c.textContent).toBe('LOAD_A "RESOLVE"'); + }); + + testRecoil('Diverged shared selectors', async () => { + const myAtom = stringAtom(); + atom({ + key: 'selector stores diverged atom', + default: 'DEFAULT', + }); + + let addDeps; + const addDepsPromise = new Promise(resolve => { + addDeps = resolve; + }); + const mySelector = selector({ + key: 'selector stores diverged selector', + get: async ({get}) => { + await addDepsPromise; + const value = get(myAtom); + + await Promise.resolve(); // So resolution occurs during act() + if (value === 'RESOLVE') { + return value; + } + await new Promise(() => {}); + }, + }); + + let setAtomA; + function SetAtomA() { + setAtomA = useSetRecoilState(myAtom); + return null; + } + let setAtomB; + function SetAtomB() { + setAtomB = useSetRecoilState(myAtom); + return null; + } + + const c = renderUnwrappedElements( + <> + + + + + + + + + + + + + , + ); + expect(c.textContent).toBe('LOAD_A LOAD_B '); act(() => { + setAtomA('SETA'); setAtomB('RESOLVE'); }); await flushPromisesAndTimers(); + await flushPromisesAndTimers(); // Double flush for open source environment + expect(c.textContent).toBe('LOAD_A LOAD_B '); + + await act(async () => { + addDeps(); + }); await flushPromisesAndTimers(); + await flushPromisesAndTimers(); // Double flush for open source environment expect(c.textContent).toBe('LOAD_A "RESOLVE"'); }); });