From f61a3719d4eceacac87c51def32e8f79968bad98 Mon Sep 17 00:00:00 2001 From: Douglas Armstrong Date: Wed, 13 Apr 2022 00:28:42 -0700 Subject: [PATCH] Cosmetic selector cleanup (#1721) Summary: Pull Request resolved: https://github.com/facebookexperimental/Recoil/pull/1721 Minor cosementic cleanup of the selector implementation to make names more accurate and the code easier to maintain. Reviewed By: noritheduck Differential Revision: D35493524 fbshipit-source-id: 4f937fa057b5ef2125400e1c8b636fc49302eb9f --- packages/recoil/core/Recoil_Graph.js | 113 +++------ packages/recoil/core/Recoil_GraphTypes.js | 4 +- .../recoil/recoil_values/Recoil_selector.js | 235 ++++++++---------- 3 files changed, 148 insertions(+), 204 deletions(-) diff --git a/packages/recoil/core/Recoil_Graph.js b/packages/recoil/core/Recoil_Graph.js index 056fbd4c0..894e5c420 100644 --- a/packages/recoil/core/Recoil_Graph.js +++ b/packages/recoil/core/Recoil_Graph.js @@ -11,7 +11,7 @@ 'use strict'; -import type {DependencyMap, Graph} from './Recoil_GraphTypes'; +import type {Graph} from './Recoil_GraphTypes'; import type {NodeKey, StateID} from './Recoil_Keys'; import type {Store} from './Recoil_State'; @@ -19,7 +19,7 @@ const differenceSets = require('recoil-shared/util/Recoil_differenceSets'); const mapMap = require('recoil-shared/util/Recoil_mapMap'); const nullthrows = require('recoil-shared/util/Recoil_nullthrows'); const recoverableViolation = require('recoil-shared/util/Recoil_recoverableViolation'); -export type {DependencyMap, Graph} from './Recoil_GraphTypes'; +export type {Graph} from './Recoil_GraphTypes'; function makeGraph(): Graph { return { @@ -40,61 +40,54 @@ function cloneGraph(graph: Graph): Graph { // Note that this overwrites the deps of existing nodes, rather than unioning // the new deps with the old deps. -function mergeDependencyMapIntoGraph( - deps: DependencyMap, +function mergeDepsIntoGraph( + key: NodeKey, + newDeps: $ReadOnlySet, graph: Graph, // If olderGraph is given then we will not overwrite changes made to the given // graph compared with olderGraph: olderGraph?: Graph, ): void { const {nodeDeps, nodeToNodeSubscriptions} = graph; + const oldDeps = nodeDeps.get(key); - deps.forEach((upstreams, downstream) => { - const existingUpstreams = nodeDeps.get(downstream); + if (oldDeps && olderGraph && oldDeps !== olderGraph.nodeDeps.get(key)) { + return; + } - if ( - existingUpstreams && - olderGraph && - existingUpstreams !== olderGraph.nodeDeps.get(downstream) - ) { - return; - } + // Update nodeDeps: + nodeDeps.set(key, newDeps); - // Update nodeDeps: - nodeDeps.set(downstream, new Set(upstreams)); + // Add new deps to nodeToNodeSubscriptions: + const addedDeps = + oldDeps == null ? newDeps : differenceSets(newDeps, oldDeps); + for (const dep of addedDeps) { + if (!nodeToNodeSubscriptions.has(dep)) { + nodeToNodeSubscriptions.set(dep, new Set()); + } + const existing = nullthrows(nodeToNodeSubscriptions.get(dep)); + existing.add(key); + } - // Add new deps to nodeToNodeSubscriptions: - const addedUpstreams = - existingUpstreams == null - ? upstreams - : differenceSets(upstreams, existingUpstreams); - addedUpstreams.forEach(upstream => { - if (!nodeToNodeSubscriptions.has(upstream)) { - nodeToNodeSubscriptions.set(upstream, new Set()); + // Remove removed deps from nodeToNodeSubscriptions: + if (oldDeps) { + const removedDeps = differenceSets(oldDeps, newDeps); + for (const dep of removedDeps) { + if (!nodeToNodeSubscriptions.has(dep)) { + return; + } + const existing = nullthrows(nodeToNodeSubscriptions.get(dep)); + existing.delete(key); + if (existing.size === 0) { + nodeToNodeSubscriptions.delete(dep); } - const existing = nullthrows(nodeToNodeSubscriptions.get(upstream)); - existing.add(downstream); - }); - - // Remove removed deps from nodeToNodeSubscriptions: - if (existingUpstreams) { - const removedUpstreams = differenceSets(existingUpstreams, upstreams); - removedUpstreams.forEach(upstream => { - if (!nodeToNodeSubscriptions.has(upstream)) { - return; - } - const existing = nullthrows(nodeToNodeSubscriptions.get(upstream)); - existing.delete(downstream); - if (existing.size === 0) { - nodeToNodeSubscriptions.delete(upstream); - } - }); } - }); + } } -function saveDependencyMapToStore( - dependencyMap: DependencyMap, +function saveDepsToStore( + key: NodeKey, + deps: $ReadOnlySet, store: Store, version: StateID, ): void { @@ -115,13 +108,13 @@ function saveDependencyMapToStore( // Merge the dependencies discovered into the store's dependency map // for the version that was read: const graph = store.getGraph(version); - mergeDependencyMapIntoGraph(dependencyMap, graph); + mergeDepsIntoGraph(key, deps, graph); // If this version is not the latest version, also write these dependencies // into later versions if they don't already have their own: if (version === storeState.previousTree?.version) { const currentGraph = store.getGraph(storeState.currentTree.version); - mergeDependencyMapIntoGraph(dependencyMap, currentGraph, graph); + mergeDepsIntoGraph(key, deps, currentGraph, graph); } if ( version === storeState.previousTree?.version || @@ -130,39 +123,13 @@ function saveDependencyMapToStore( const nextVersion = storeState.nextTree?.version; if (nextVersion !== undefined) { const nextGraph = store.getGraph(nextVersion); - mergeDependencyMapIntoGraph(dependencyMap, nextGraph, graph); + mergeDepsIntoGraph(key, deps, nextGraph, graph); } } } -function mergeDepsIntoDependencyMap( - from: DependencyMap, - into: DependencyMap, -): void { - from.forEach((upstreamDeps, downstreamNode) => { - if (!into.has(downstreamNode)) { - into.set(downstreamNode, new Set()); - } - const deps = nullthrows(into.get(downstreamNode)); - upstreamDeps.forEach(dep => deps.add(dep)); - }); -} - -function addToDependencyMap( - downstream: NodeKey, - upstream: NodeKey, - dependencyMap: DependencyMap, -): void { - if (!dependencyMap.has(downstream)) { - dependencyMap.set(downstream, new Set()); - } - nullthrows(dependencyMap.get(downstream)).add(upstream); -} - module.exports = { - addToDependencyMap, cloneGraph, graph: makeGraph, - mergeDepsIntoDependencyMap, - saveDependencyMapToStore, + saveDepsToStore, }; diff --git a/packages/recoil/core/Recoil_GraphTypes.js b/packages/recoil/core/Recoil_GraphTypes.js index 4c81024a9..894c13144 100644 --- a/packages/recoil/core/Recoil_GraphTypes.js +++ b/packages/recoil/core/Recoil_GraphTypes.js @@ -17,13 +17,11 @@ export type Graph = $ReadOnly<{ // TODO rename these properties to be more descriptive and symetric. // Upstream Node dependencies // NOTE: if you ever make the sets in nodeDeps mutable you must change the - // logic in mergeDependencyMapIntoGraph that relies on reference equality + // logic in mergeDepsIntoGraph() that relies on reference equality // of these sets in avoiding overwriting newer deps with older ones. nodeDeps: Map>, // Downstream Node subscriptions nodeToNodeSubscriptions: Map>, }>; -export type DependencyMap = Map>; - module.exports = ({}: {...}); diff --git a/packages/recoil/recoil_values/Recoil_selector.js b/packages/recoil/recoil_values/Recoil_selector.js index 9de73559b..1c1b86bbf 100644 --- a/packages/recoil/recoil_values/Recoil_selector.js +++ b/packages/recoil/recoil_values/Recoil_selector.js @@ -92,7 +92,7 @@ const { peekNodeLoadable, setNodeValue, } = require('../core/Recoil_FunctionalCore'); -const {saveDependencyMapToStore} = require('../core/Recoil_Graph'); +const {saveDepsToStore} = require('../core/Recoil_Graph'); const { DEFAULT_VALUE, RecoilValueNotReady, @@ -164,14 +164,14 @@ class Canceled {} const CANCELED: Canceled = new Canceled(); /** - * An ExecutionId is an arbitrary ID that lets us distinguish executions from + * An ExecutionID is an arbitrary ID that lets us distinguish executions from * each other. This is necessary as we need a way of solving this problem: * "given 3 async executions, only update state for the 'latest' execution when - * it finishes running regardless of when the other 2 finish". ExecutionIds + * it finishes running regardless of when the other 2 finish". ExecutionIDs * provide a convenient way of identifying executions so that we can track and * manage them over time. */ -type ExecutionId = number; +type ExecutionID = number; /** * ExecutionInfo is useful for managing async work and resolving race @@ -196,8 +196,8 @@ type ExecutionId = number; type ExecutionInfo = { // This is mutable and updated as new deps are discovered depValuesDiscoveredSoFarDuringAsyncWork: DepValues, - latestLoadable: LoadingLoadableType, - latestExecutionId: ExecutionId, + loadingLoadable: LoadingLoadableType, + executionID: ExecutionID, stateVersion: StateID, }; @@ -213,9 +213,9 @@ type LoadingDepsState = { }; const dependencyStack = []; // for detecting circular dependencies. -const waitingStores: Map> = new Map(); +const waitingStores: Map> = new Map(); -const getNewExecutionId: () => ExecutionId = (() => { +const getNewExecutionID: () => ExecutionID = (() => { let executionId = 0; return () => executionId++; })(); @@ -283,7 +283,7 @@ function selector( function resolveAsync( store: Store, state: TreeState, - executionId: ExecutionId, + executionId: ExecutionID, loadable: Loadable, depValues: DepValues, ): void { @@ -293,7 +293,7 @@ function selector( function notifyStoresOfResolvedAsync( store: Store, - executionId: ExecutionId, + executionId: ExecutionID, ): void { if (isLatestExecution(store, executionId)) { clearExecutionInfo(store); @@ -309,7 +309,7 @@ function selector( function markStoreWaitingForResolvedAsync( store: Store, - executionId: ExecutionId, + executionId: ExecutionID, ): void { let stores = waitingStores.get(executionId); if (stores == null) { @@ -347,12 +347,12 @@ function selector( * dependency promise. Dependency promises should be passed to * wrapPendingDependencyPromise()). */ - function wrapPendingPromise( + function wrapResultPromise( store: Store, promise: Promise, state: TreeState, depValues: DepValues, - executionId: ExecutionId, + executionId: ExecutionID, loadingDepsState: LoadingDepsState, ): Promise { return promise @@ -374,8 +374,6 @@ function selector( throw CANCELED; } - // updateExecutionInfoDepValues(store, executionId, depValues); - if (isPromise(errorOrPromise)) { return wrapPendingDependencyPromise( store, @@ -421,14 +419,14 @@ function selector( * Note this function should not be passed a promise that was returned from * get(). The intention is that this function is only passed promises that * were thrown due to a pending dependency. Promises returned by get() should - * be passed to wrapPendingPromise() instead. + * be passed to wrapResultPromise() instead. */ function wrapPendingDependencyPromise( store: Store, promise: Promise, state: TreeState, existingDeps: DepValues, - executionId: ExecutionId, + executionId: ExecutionID, loadingDepsState: LoadingDepsState, ): Promise { return promise @@ -506,10 +504,7 @@ function selector( * the selector already has a code path that lets us exit early when * an async dep resolves. */ - const cachedLoadable = getValFromCacheAndUpdatedDownstreamDeps( - store, - state, - ); + const cachedLoadable = getLoadableFromCacheAndUpdateDeps(store, state); if (cachedLoadable && cachedLoadable.state !== 'loading') { /** * This has to notify stores of a resolved async, even if there is no @@ -527,7 +522,7 @@ function selector( * I think this is only an issue with "early" rendering since the * components got their value using the in-progress execution. * We don't have a unit test for this case yet. I'm not sure it is - * necessary with recoil_concurrent_support mode. + * necessary with recoil_transition_support mode. */ if ( isLatestExecution(store, executionId) || @@ -568,16 +563,13 @@ function selector( * to extend the tree cache to support caching loading states. */ if (!isLatestExecution(store, executionId)) { - const executionInfo = getExecutionInfoOfInProgressExecution( - store, - state, - ); - if (executionInfo?.latestLoadable.state === 'loading') { + const executionInfo = getInProgressExecutionInfo(store, state); + if (executionInfo != null) { /** * Returning promise here without wrapping as the wrapper logic was * already done upstream when this promise was generated. */ - return executionInfo.latestLoadable.contents; + return executionInfo.loadingLoadable.contents; } } @@ -588,8 +580,6 @@ function selector( executionId, ); - updateExecutionInfoDepValues(store, executionId, depValues); - if (loadable.state !== 'loading') { resolveAsync(store, state, executionId, loadable, depValues); } @@ -615,31 +605,34 @@ function selector( }); } - function setDepsInStore( + function updateDeps( store: Store, state: TreeState, - deps: Set, - executionId: ?ExecutionId, + deps: $ReadOnlySet, + executionId: ?ExecutionID, ): void { if ( isLatestExecution(store, executionId) || state.version === store.getState()?.currentTree?.version || state.version === store.getState()?.nextTree?.version ) { - saveDependencyMapToStore( - new Map([[key, deps]]), + saveDepsToStore( + key, + deps, store, store.getState()?.nextTree?.version ?? store.getState().currentTree.version, ); - deps.forEach(nodeKey => discoveredDependencyNodeKeys.add(nodeKey)); + } + for (const nodeKey of deps) { + discoveredDependencyNodeKeys.add(nodeKey); } } 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; @@ -662,29 +655,25 @@ function selector( * starting a new set versus adding it in existing state deps because * the version of state that we update deps for may be a more recent version * than the version the selector was called with. This is because the latest - * execution will update the deps of the current/latest version of state ( - * this is safe to do because the fact that the selector is the latest + * execution will update the deps of the current/latest version of state + * (This is safe to do because the fact that the selector is the latest * execution means the deps we discover below are our best guess at the * deps for the current/latest state in the store) */ const depValues = new Map(); - const deps = new Set(); - function getRecoilValue(dep: RecoilValue): S { - const {key: depKey} = dep; + function getRecoilValue({key: depKey}: RecoilValue): S { + const depLoadable = getNodeLoadable(store, state, depKey); + + depValues.set(depKey, depLoadable); - deps.add(depKey); // We need to update asynchronous dependencies as we go so the selector // knows if it has to restart evaluation if one of them is updated before // the asynchronous selector completely resolves. if (!duringSynchronousExecution) { - setDepsInStore(store, state, deps, executionId); + updateDeps(store, state, new Set(depValues.keys()), executionId); } - const depLoadable = getNodeLoadable(store, state, depKey); - - depValues.set(depKey, depLoadable); - switch (depLoadable.state) { case 'hasValue': return depLoadable.contents; @@ -729,7 +718,7 @@ function selector( } if (isPromise(result)) { - result = wrapPendingPromise( + result = wrapResultPromise( store, result, state, @@ -769,11 +758,12 @@ function selector( } duringSynchronousExecution = false; - setDepsInStore(store, state, deps, executionId); + updateExecutionInfoDepValues(store, executionId, depValues); + updateDeps(store, state, new Set(depValues.keys()), executionId); return [loadable, depValues]; } - function getValFromCacheAndUpdatedDownstreamDeps( + function getLoadableFromCacheAndUpdateDeps( store: Store, state: TreeState, ): ?Loadable { @@ -786,7 +776,6 @@ function selector( // Second, look up in the selector cache and update the deps in the store const depsAfterCacheLookup = new Set(); - const executionInfo = getExecutionInfo(store); try { cachedLoadable = cache.get( nodeKey => { @@ -824,11 +813,11 @@ function selector( * a change in deps in the store if the store deps for this state are empty * or stale. */ - setDepsInStore( + updateDeps( store, state, depsAfterCacheLookup, - executionInfo?.latestExecutionId, + getExecutionInfo(store)?.executionID, ); } @@ -836,60 +825,7 @@ function selector( } /** - * FIXME: dep keys should take into account the state of the loadable to - * prevent the edge case where a loadable with an error and a loadable with - * an error as a value are treated as the same thing incorrectly. For example - * these two should be treated differently: - * - * selector({key: '', get: () => new Error('hi')}); - * selector({key: '', get () => {throw new Error('hi')}}); - * - * With current implementation they are treated the same - */ - function depValuesToDepRoute(depValues: DepValues): NodeCacheRoute { - return Array.from(depValues.entries()).map(([depKey, valLoadable]) => [ - depKey, - valLoadable.contents, - ]); - } - - function getValFromRunningNewExecutionAndUpdatedDeps( - store: Store, - state: TreeState, - ): Loadable { - const newExecutionId = getNewExecutionId(); - - const [loadable, newDepValues] = evaluateSelectorGetter( - store, - state, - newExecutionId, - ); - - /** - * Conditionally updates the cache with a given loadable. - * - * We only cache loadables that are not loading because our cache keys are - * based on dep values, which are in an unfinished state for loadables that - * have a 'loading' state (new deps may be discovered while the selector - * runs its async code). We never want to cache partial dependencies b/c it - * could lead to errors, such as prematurely returning the result based on a - * 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') { - setExecutionInfo(store, newExecutionId, loadable, newDepValues, state); - markStoreWaitingForResolvedAsync(store, newExecutionId); - } else { - clearExecutionInfo(store); - setCache(state, loadable, newDepValues); - } - - return loadable; - } - - /** - * Given a tree state, this function returns the "selector result", which is - * defined as a size-2 tuple of [DependencyMap, Loadable]. + * Given a tree state, this function returns a Loadable of the current state. * * The selector's get() function will only be re-evaluated if _both_ of the * following statements are true: @@ -906,43 +842,66 @@ function selector( * update global state when it is finished. Any non-latest executions will * run to completion and update the selector cache but not global state). */ - function getSelectorValAndUpdatedDeps( + function getSelectorLoadableAndUpdateDeps( store: Store, state: TreeState, ): Loadable { - const cachedVal = getValFromCacheAndUpdatedDownstreamDeps(store, state); - + // First, see if our current state is cached + const cachedVal = getLoadableFromCacheAndUpdateDeps(store, state); if (cachedVal != null) { clearExecutionInfo(store); return cachedVal; } - const inProgressExecutionInfo = getExecutionInfoOfInProgressExecution( - store, - state, - ); - - // FIXME: this won't work with custom caching b/c it uses separate cache + // Second, check if there is already an ongoing execution based on the current state + const inProgressExecutionInfo = getInProgressExecutionInfo(store, state); if (inProgressExecutionInfo != null) { - if (inProgressExecutionInfo.latestLoadable?.state === 'loading') { + if (inProgressExecutionInfo.loadingLoadable?.state === 'loading') { markStoreWaitingForResolvedAsync( store, - nullthrows(inProgressExecutionInfo.latestExecutionId), + inProgressExecutionInfo.executionID, ); } // FIXME: check after the fact to see if we made the right choice by waiting - return nullthrows(inProgressExecutionInfo.latestLoadable); + return inProgressExecutionInfo.loadingLoadable; + } + + // Third, start a new evaluation of the selector + const newExecutionID = getNewExecutionID(); + const [loadable, newDepValues] = evaluateSelectorGetter( + store, + state, + newExecutionID, + ); + + /** + * Conditionally updates the cache with a given loadable. + * + * We only cache loadables that are not loading because our cache keys are + * based on dep values, which are in an unfinished state for loadables that + * have a 'loading' state (new deps may be discovered while the selector + * runs its async code). We never want to cache partial dependencies b/c it + * could lead to errors, such as prematurely returning the result based on a + * 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') { + setExecutionInfo(store, newExecutionID, loadable, newDepValues, state); + markStoreWaitingForResolvedAsync(store, newExecutionID); + } else { + clearExecutionInfo(store); + setCache(state, loadable, newDepValues); } - return getValFromRunningNewExecutionAndUpdatedDeps(store, state); + return loadable; } /** * Searches execution info across all stores to see if there is an in-progress * execution whose dependency values match the values of the requesting store. */ - function getExecutionInfoOfInProgressExecution( + function getInProgressExecutionInfo( store: Store, state: TreeState, ): ?ExecutionInfo { @@ -993,24 +952,26 @@ function selector( */ function setExecutionInfo( store: Store, - newExecutionId: ExecutionId, + newExecutionID: ExecutionID, loadable: LoadingLoadableType, depValues: DepValues, state: TreeState, ) { executionInfoMap.set(store, { depValuesDiscoveredSoFarDuringAsyncWork: depValues, - latestExecutionId: newExecutionId, - latestLoadable: loadable, + executionID: newExecutionID, + loadingLoadable: loadable, stateVersion: state.version, }); } 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)) { const executionInfo = getExecutionInfo(store); if (executionInfo != null) { @@ -1024,7 +985,25 @@ function selector( } function isLatestExecution(store: Store, executionId): boolean { - return executionId === getExecutionInfo(store)?.latestExecutionId; + return executionId === getExecutionInfo(store)?.executionID; + } + + /** + * FIXME: dep keys should take into account the state of the loadable to + * prevent the edge case where a loadable with an error and a loadable with + * an error as a value are treated as the same thing incorrectly. For example + * these two should be treated differently: + * + * selector({key: '', get: () => new Error('hi')}); + * selector({key: '', get () => {throw new Error('hi')}}); + * + * With current implementation they are treated the same + */ + function depValuesToDepRoute(depValues: DepValues): NodeCacheRoute { + return Array.from(depValues.entries()).map(([depKey, valLoadable]) => [ + depKey, + valLoadable.contents, + ]); } function setCache( @@ -1079,7 +1058,7 @@ function selector( function selectorGet(store: Store, state: TreeState): Loadable { return detectCircularDependencies(() => - getSelectorValAndUpdatedDeps(store, state), + getSelectorLoadableAndUpdateDeps(store, state), ); }