Data: Call resolver isFulfilled once per argument set (#6360)

* Data: Call resolver isFulfilled once per argument set

* Data: Use equivalent-key-map for tracking fulfillment by arguments

data/index.js

@@ -3,8 +3,8 @@
  */
 import isShallowEqual from 'shallowequal';
 import { combineReducers, createStore } from 'redux';
-import { flowRight, without, mapValues } from 'lodash';
-import memoize from 'memize';
+import { flowRight, without, mapValues, overEvery } from 'lodash';
+import EquivalentKeyMap from 'equivalent-key-map';
 
 /**
  * WordPress dependencies
@@ -137,15 +137,49 @@ export function registerResolvers( reducerKey, newResolvers ) {
 		}
 
 		const store = stores[ reducerKey ];
-		const resolver = newResolvers[ key ];
 
-		const rawFulfill = async ( ...args ) => {
+		// Normalize resolver shape to object.
+		let resolver = newResolvers[ key ];
+		if ( ! resolver.fulfill ) {
+			resolver = { fulfill: resolver };
+		}
+
+		/**
+		 * To ensure that fulfillment occurs only once per arguments set
+		 * (even for deeply "equivalent" arguments), track calls.
+		 *
+		 * @type {EquivalentKeyMap}
+		 */
+		const fulfilledByEquivalentArgs = new EquivalentKeyMap();
+
+		/**
+		 * Returns true if resolver fulfillment has already occurred for an
+		 * equivalent set of arguments. Includes side effect when returning
+		 * false to ensure the next invocation returns true.
+		 *
+		 * @param {Array} args Arguments set.
+		 *
+		 * @return {boolean} Whether fulfillment has already occurred.
+		 */
+		function hasBeenFulfilled( args ) {
+			const hasArguments = fulfilledByEquivalentArgs.has( args );
+			if ( ! hasArguments ) {
+				fulfilledByEquivalentArgs.set( args, true );
+			}
+
+			return hasArguments;
+		}
+
+		async function fulfill( ...args ) {
+			if ( hasBeenFulfilled( args ) ) {
+				return;
+			}
+
 			// At this point, selectors have already been pre-bound to inject
 			// state, it would not be otherwise provided to fulfill.
 			const state = store.getState();
 
-			const fulfill = resolver.fulfill ? resolver.fulfill : resolver;
-			let fulfillment = fulfill( state, ...args );
+			let fulfillment = resolver.fulfill( state, ...args );
 
 			// Attempt to normalize fulfillment as async iterable.
 			fulfillment = toAsyncIterable( fulfillment );
@@ -159,16 +193,19 @@ export function registerResolvers( reducerKey, newResolvers ) {
 					store.dispatch( maybeAction );
 				}
 			}
-		};
+		}
 
-		// Ensure single invocation per argument set via memoization
-		// or via isFulfilled call if provided.
-		const fulfill = resolver.isFulfilled ? ( ...args ) => {
-			const state = store.getState();
-			if ( ! resolver.isFulfilled( state, ...args ) ) {
-				rawFulfill( ...args );
-			}
-		} : memoize( rawFulfill );
+		if ( typeof resolver.isFulfilled === 'function' ) {
+			// When resolver provides its own fulfillment condition, fulfill
+			// should only occur if not already fulfilled (opt-out condition).
+			fulfill = overEvery( [
+				( ...args ) => {
+					const state = store.getState();
+					return ! resolver.isFulfilled( state, ...args );
+				},
+				fulfill,
+			] );
+		}
 
 		return ( ...args ) => {
 			fulfill( ...args );

data/test/index.js

@@ -139,27 +139,62 @@ describe( 'registerResolvers', () => {
 	} );
 
 	it( 'should use isFulfilled definition before calling the side effect', () => {
-		const resolver = jest.fn();
-		let count = 0;
+		const fulfill = jest.fn().mockImplementation( ( state, page ) => {
+			return { type: 'SET_PAGE', page, result: [] };
+		} );
+
+		const store = registerReducer( 'demo', ( state = {}, action ) => {
+			switch ( action.type ) {
+				case 'SET_PAGE':
+					return {
+						...state,
+						[ action.page ]: action.result,
+					};
+			}
+
+			return state;
+		} );
+
+		store.dispatch( { type: 'SET_PAGE', page: 4, result: [] } );
 
-		registerReducer( 'demo', ( state = 'OK' ) => state );
 		registerSelectors( 'demo', {
-			getValue: ( state ) => state,
+			getPage: ( state, page ) => state[ page ],
 		} );
 		registerResolvers( 'demo', {
-			getValue: {
-				fulfill: ( ...args ) => {
-					count++;
-					resolver( ...args );
+			getPage: {
+				fulfill,
+				isFulfilled( state, page ) {
+					return state.hasOwnProperty( page );
 				},
-				isFulfilled: () => count > 1,
 			},
 		} );
 
-		for ( let i = 0; i < 4; i++ ) {
-			select( 'demo' ).getValue( 'arg1', 'arg2' );
-		}
-		expect( resolver ).toHaveBeenCalledTimes( 2 );
+		select( 'demo' ).getPage( 1 );
+		select( 'demo' ).getPage( 2 );
+
+		expect( fulfill ).toHaveBeenCalledTimes( 2 );
+
+		select( 'demo' ).getPage( 1 );
+		select( 'demo' ).getPage( 2 );
+		select( 'demo' ).getPage( 3, {} );
+
+		// Expected: First and second page fulfillments already triggered, so
+		// should only be one more than previous assertion set.
+		expect( fulfill ).toHaveBeenCalledTimes( 3 );
+
+		select( 'demo' ).getPage( 1 );
+		select( 'demo' ).getPage( 2 );
+		select( 'demo' ).getPage( 3, {} );
+		select( 'demo' ).getPage( 4 );
+
+		// Expected:
+		//  - Fourth page was pre-filled. Necessary to determine via
+		//    isFulfilled, but fulfillment resolver should not be triggered.
+		//  - Third page arguments are not strictly equal but are equivalent,
+		//    so fulfillment should already be satisfied.
+		expect( fulfill ).toHaveBeenCalledTimes( 3 );
+
+		select( 'demo' ).getPage( 4, {} );
 	} );
 
 	it( 'should resolve action to dispatch', ( done ) => {

package.json

@@ -26,6 +26,7 @@
 		"dom-react": "2.2.0",
 		"dom-scroll-into-view": "1.2.1",
 		"element-closest": "2.0.2",
+		"equivalent-key-map": "0.1.1",
 		"escape-string-regexp": "1.0.5",
 		"eslint-plugin-wordpress": "git://github.com/WordPress-Coding-Standards/eslint-plugin-wordpress.git#1774343f6226052a46b081e01db3fca8793cc9f1",
 		"hpq": "1.2.0",