Update debouncing function - use trailing edge debounce only and impl…

…ement a maxWait

packages/preferences-persistence/README.md

@@ -41,8 +41,9 @@ _Parameters_
 -   _options_ `Object`:
 -   _options.preloadedData_ `?Object`: Any persisted preferences data that should be preloaded. When set, the persistence layer will avoid fetching data from the REST API.
 -   _options.localStorageRestoreKey_ `?string`: The key to use for restoring the localStorage backup, used when the persistence layer calls `localStorage.getItem` or `localStorage.setItem`.
--   _options.requestDebounceMS_ `?number`: Debounce requests to the API so that they only occur at minimum every `requestDebounceMS` milliseconds, and don't swamp the server. Defaults to 2500ms.
--   _options.expensiveRequestDebounceMS_ `?number`: A longer debounce that can be defined for updates that have `isExpensive=true` defined. defaults to 60000ms.
+-   _options.requestDebounceMS_ `?number`: Debounce requests to the API so that they only occur at minimum every `requestDebounceMS` milliseconds, and don't swamp the server. Defaults to 1000ms.
+-   _options.expensiveRequestDebounceMS_ `?number`: A longer debounce that can be defined for updates that have `isExpensive=true` defined. defaults to 5000ms.
+-   _options.maxWaitMS_ `?number`:
 
 _Returns_
 

packages/preferences-persistence/src/create/create-async-debouncer.js

@@ -1,43 +1,29 @@
 /**
- * Performs a leading edge debounce of async functions.
+ * Performs a trailing edge debounce of async functions.
  *
- * If three functions are throttled at the same time:
- * - The first happens immediately.
- * - The second is never called.
- * - The third happens `delayMS` milliseconds after the first has resolved.
+ * If the debounce is called twice at the same time:
+ * - the first is queued via a timeout on the first call.
+ * - the first timeout is cancelled and the second is queued on the second call.
+ *
+ * If the second call begins a promise resolution and a third call is made,
+ * the third call awaits resolution before queuing a third timeout.
  *
  * This is distinct from `{ debounce } from @wordpress/compose` in that it
- * waits for promise resolution.
+ * waits for promise resolution and is supports each debounce invocation
+ * having a variable delay.
  *
+ * @param {Object} options
+ * @param {number} options.maxWaitMS
  * @return {Function} A function that debounces the async function passed to it
  *                    in the first parameter by the time passed in the second
  *                    options parameter.
  */
-export default function createAsyncDebouncer() {
+export default function createAsyncDebouncer( { maxWaitMS } = {} ) {
 	let timeoutId;
 	let activePromise;
+	let firstRequestTime;
 
-	return async function debounced( func, { delayMS, isTrailing = false } ) {
-		// This is a leading edge debounce. If there's no promise or timeout
-		// in progress, call the debounced function immediately.
-		if ( ! isTrailing && ! activePromise && ! timeoutId ) {
-			return new Promise( ( resolve, reject ) => {
-				// Keep a reference to the promise.
-				activePromise = func()
-					.then( ( ...thenArgs ) => {
-						resolve( ...thenArgs );
-					} )
-					.catch( ( error ) => {
-						reject( error );
-					} )
-					.finally( () => {
-						// As soon this promise is complete, clear the way for the
-						// next one to happen immediately.
-						activePromise = null;
-					} );
-			} );
-		}
-
+	return async function debounced( func, { delayMS } ) {
 		if ( activePromise ) {
 			// Let any active promises finish before queuing the next request.
 			await activePromise;
@@ -52,6 +38,19 @@ export default function createAsyncDebouncer() {
 
 		// Trigger any trailing edge calls to the function.
 		return new Promise( ( resolve, reject ) => {
+			let maxedDelay = delayMS;
+			if ( maxWaitMS !== undefined ) {
+				if ( firstRequestTime ) {
+					const elapsed = Date.now() - firstRequestTime;
+					// Ensure wait is less than the maxWait.
+					maxedDelay = Math.max( 0, maxWaitMS - elapsed );
+					// But also not longer than `delayMS`.
+					maxedDelay = Math.min( delayMS, maxedDelay );
+				} else {
+					firstRequestTime = Date.now();
+				}
+			}
+
 			// Schedule the next request but with a delay.
 			timeoutId = setTimeout( () => {
 				activePromise = func()
@@ -63,11 +62,12 @@ export default function createAsyncDebouncer() {
 					} )
 					.finally( () => {
 						// As soon this promise is complete, clear the way for the
-						// next one to happen immediately.
+						// next function to be queued.
 						activePromise = null;
 						timeoutId = null;
+						firstRequestTime = null;
 					} );
-			}, delayMS );
+			}, maxedDelay );
 		} );
 	};
 }

packages/preferences-persistence/src/create/index.js

@@ -24,21 +24,23 @@ const localStorage = window.localStorage;
  *                                                     `localStorage.setItem`.
  * @param {?number} options.requestDebounceMS          Debounce requests to the API so that they only occur at
  *                                                     minimum every `requestDebounceMS` milliseconds, and don't
- *                                                     swamp the server. Defaults to 2500ms.
+ *                                                     swamp the server. Defaults to 1000ms.
  *
  * @param {?number} options.expensiveRequestDebounceMS A longer debounce that can be defined for updates that have
- *                                                     `isExpensive=true` defined. defaults to 60000ms.
+ *                                                     `isExpensive=true` defined. defaults to 5000ms.
  *
+ * @param {?number} options.maxWaitMS
  * @return {Object} A persistence layer for WordPress user meta.
  */
 export default function create( {
 	preloadedData,
 	localStorageRestoreKey = 'WP_PREFERENCES_RESTORE_DATA',
-	requestDebounceMS = 2500,
-	expensiveRequestDebounceMS = 60000,
+	requestDebounceMS = 1000,
+	expensiveRequestDebounceMS = 5000,
+	maxWaitMS = 10000,
 } = {} ) {
 	let cache = preloadedData;
-	const debounce = createAsyncDebouncer();
+	const debounce = createAsyncDebouncer( { maxWaitMS } );
 
 	async function get() {
 		if ( cache ) {
@@ -108,7 +110,6 @@ export default function create( {
 				delayMS: isExpensive
 					? expensiveRequestDebounceMS
 					: requestDebounceMS,
-				isTrailing: isExpensive,
 			}
 		);
 	}

packages/preferences-persistence/src/create/test/create-async-debouncer.js

@@ -17,102 +17,142 @@ function timeout( milliseconds ) {
 }
 
 describe( 'debounceAsync', () => {
-	it( 'uses a leading debounce by default, the first call happens immediately', () => {
+	it( 'uses a trailing debounce by default, the first call happens after delayMS', async () => {
+		jest.useFakeTimers();
 		const fn = jest.fn( async () => {} );
 		const debounce = createAsyncDebouncer();
 		debounce( () => fn(), { delayMS: 20 } );
+
+		// It isn't called immediately.
+		expect( fn ).not.toHaveBeenCalled();
+
+		await flushPromises();
+		jest.advanceTimersByTime( 19 );
+
+		// It isn't called before `delayMS`.
+		expect( fn ).not.toHaveBeenCalled();
+
+		await flushPromises();
+		jest.advanceTimersByTime( 2 );
+
+		// It is called after `delayMS`.
 		expect( fn ).toHaveBeenCalledTimes( 1 );
+
+		jest.runOnlyPendingTimers();
+		jest.useRealTimers();
 	} );
 
-	it( 'calls the function on the leading edge and then once on the trailing edge when there are multiple leading edge calls', async () => {
+	it( 'calls the function on the trailing edge only once when there are multiple trailing edge calls', async () => {
 		jest.useFakeTimers();
 		const fn = jest.fn( async () => {} );
 		const debounce = createAsyncDebouncer();
 
 		debounce( () => fn( 'A' ), { delayMS: 20 } );
-
-		expect( fn ).toHaveBeenCalledTimes( 1 );
-
 		debounce( () => fn( 'B' ), { delayMS: 20 } );
 		debounce( () => fn( 'C' ), { delayMS: 20 } );
 		debounce( () => fn( 'D' ), { delayMS: 20 } );
 
 		await flushPromises();
 		jest.runAllTimers();
 
-		expect( fn ).toHaveBeenCalledTimes( 2 );
-		expect( fn ).toHaveBeenCalledWith( 'A' );
+		expect( fn ).toHaveBeenCalledTimes( 1 );
 		expect( fn ).toHaveBeenCalledWith( 'D' );
 
 		jest.runOnlyPendingTimers();
 		jest.useRealTimers();
 	} );
 
-	it( 'can be configured to use a trailing edge debounce, the first call happens after the delay', () => {
+	it( 'supports variable delay for each invocation of the debounce', async () => {
 		jest.useFakeTimers();
 		const fn = jest.fn( async () => {} );
 		const debounce = createAsyncDebouncer();
-		debounce( () => fn(), { delayMS: 20, isTrailing: true } );
 
-		// The function isn't called immediately.
-		expect( fn ).toHaveBeenCalledTimes( 0 );
+		debounce( () => fn(), { delayMS: 5 } );
 
-		// After the delay, the function is called.
-		jest.advanceTimersByTime( 20 );
-		expect( fn ).toHaveBeenCalledTimes( 1 );
-	} );
+		// Advance to just before the first delay.
+		await flushPromises();
+		jest.advanceTimersByTime( 4 );
 
-	it( 'calls the function on the trailing edge only once when there are multiple trailing edge calls', async () => {
-		jest.useFakeTimers();
-		const fn = jest.fn( async () => {} );
-		const debounce = createAsyncDebouncer();
+		expect( fn ).toHaveBeenCalledTimes( 0 );
 
-		debounce( () => fn( 'A' ), { delayMS: 20, isTrailing: true } );
-		debounce( () => fn( 'B' ), { delayMS: 20, isTrailing: true } );
-		debounce( () => fn( 'C' ), { delayMS: 20, isTrailing: true } );
-		debounce( () => fn( 'D' ), { delayMS: 20, isTrailing: true } );
+		// Trigger a second shorter debounce
+		debounce( () => fn(), { delayMS: 2 } );
 
+		// Advance to just after the second delay.
 		await flushPromises();
-		jest.runAllTimers();
+		jest.advanceTimersByTime( 3 );
 
 		expect( fn ).toHaveBeenCalledTimes( 1 );
-		expect( fn ).toHaveBeenCalledWith( 'D' );
 
 		jest.runOnlyPendingTimers();
 		jest.useRealTimers();
 	} );
 
-	it( 'ensures the delay has elapsed between calls', async () => {
+	it( 'waits for promise resolution in the callback before debouncing again', async () => {
 		jest.useFakeTimers();
+
+		// The callback takes 10ms to resolve.
 		const fn = jest.fn( async () => timeout( 10 ) );
 		const debounce = createAsyncDebouncer();
 
-		// The first call has been triggered, but will take 10ms to resolve.
 		debounce( () => fn(), { delayMS: 20 } );
-		debounce( () => fn(), { delayMS: 20 } );
-		debounce( () => fn(), { delayMS: 20 } );
-		debounce( () => fn(), { delayMS: 20 } );
-		expect( fn ).toHaveBeenCalledTimes( 1 );
 
-		// The first call has resolved. The delay period has started but has yet to finish.
+		// Advance to just after delayMS, but before the callback has resolved.
 		await flushPromises();
-		jest.advanceTimersByTime( 11 );
+		jest.advanceTimersByTime( 25 );
+
+		// The callback has started invoking but hasn't finished resolution
 		expect( fn ).toHaveBeenCalledTimes( 1 );
 
-		// The second call is about to commence, but hasn't yet.
+		// Trigger another call.
+		debounce( () => fn(), { delayMS: 20 } );
+
+		// Advanced by enough to resolve the first timeout.
 		await flushPromises();
-		jest.advanceTimersByTime( 18 );
+		jest.advanceTimersByTime( 10 );
+
 		expect( fn ).toHaveBeenCalledTimes( 1 );
 
-		// The second call has now commenced.
+		// Then advance by enough to invoke the second timeout.
 		await flushPromises();
-		jest.advanceTimersByTime( 2 );
+		jest.advanceTimersByTime( 20 );
+
+		// The second callback should have started but now be resolving.
 		expect( fn ).toHaveBeenCalledTimes( 2 );
 
-		// No more calls happen.
+		jest.runOnlyPendingTimers();
+		jest.useRealTimers();
+	} );
+
+	it( 'invokes the callback when the maxWaitMS is reached, even when delayMS is still yet to elapse', async () => {
+		jest.useFakeTimers();
+		const fn = jest.fn( async () => {} );
+		const debounce = createAsyncDebouncer( { maxWaitMS: 8 } );
+
+		// The first call has been triggered, but will take 4ms to resolve.
+		debounce( () => fn(), { delayMS: 4 } );
+
+		// Advance by less than the delayMS (total time: 3ms).
 		await flushPromises();
-		jest.runAllTimers();
-		expect( fn ).toHaveBeenCalledTimes( 2 );
+		jest.advanceTimersByTime( 3 );
+		expect( fn ).toHaveBeenCalledTimes( 0 );
+
+		// Trigger the debounce a second time, extending the delay
+		debounce( () => fn(), { delayMS: 4 } );
+
+		// Advance again by less than the delayMS (total time: 6ms).
+		await flushPromises();
+		jest.advanceTimersByTime( 3 );
+		expect( fn ).toHaveBeenCalledTimes( 0 );
+
+		// Trigger the debounce a third time, extending the delay
+		debounce( () => fn(), { delayMS: 4 } );
+
+		// Advance again by less than the delayMS, but this time the maxWait should
+		// cause an invocation of the callback. (total time: 9ms, max wait: 8ms).
+		await flushPromises();
+		jest.advanceTimersByTime( 3 );
+		expect( fn ).toHaveBeenCalledTimes( 1 );
 
 		jest.runOnlyPendingTimers();
 		jest.useRealTimers();
@@ -125,13 +165,13 @@ describe( 'debounceAsync', () => {
 
 		// Test the return value via awaiting.
 		const returnValue = await debounce( () => fn(), {
-			delayMS: 20,
+			delayMS: 1,
 		} );
 		expect( returnValue ).toBe( 'test' );
 
 		// Test then-ing.
 		await debounce( () => fn(), {
-			delayMS: 20,
+			delayMS: 1,
 		} ).then( ( thenValue ) => expect( thenValue ).toBe( 'test' ) );
 	} );
 
@@ -147,7 +187,7 @@ describe( 'debounceAsync', () => {
 		// Test traditional try/catch.
 		try {
 			await debounce( () => fn(), {
-				delayMS: 20,
+				delayMS: 1,
 			} );
 		} catch ( error ) {
 			// Disable reason - the test uses `expect.assertions` to ensure
@@ -158,7 +198,7 @@ describe( 'debounceAsync', () => {
 
 		// Test chained .catch().
 		await await debounce( () => fn(), {
-			delayMS: 20,
+			delayMS: 1,
 		} ).catch( ( error ) => {
 			// Disable reason - the test uses `expect.assertions` to ensure
 			// conditional assertions are called.