Improvements to emitEventWithAbort. (#4158)

* modify emitEventWithAbort to change return value

`emitEventWithAbort` now returns an array of responses up to the first response that triggered an error or fail response instead of aborting on success responses too.

* update add to cart form context provider

* update checkout state context provider

* update payment method data context provider

* update tests and fix thrown error handling.

Author: nerrad

assets/js/base/context/event-emit/emitters.ts

@@ -3,6 +3,7 @@
  */
 import { getObserversByPriority } from './utils';
 import type { EventObserversType } from './types';
+import { isErrorResponse, isFailResponse } from '../hooks/use-emit-response';
 
 /**
  * Emits events on registered observers for the provided type and passes along
@@ -44,20 +45,24 @@ export const emitEvent = async (
 
 /**
  * Emits events on registered observers for the provided type and passes along
- * the provided data. This event emitter will abort and return any value from
- * observers that return an object which should contain a type property.
+ * the provided data. This event emitter will abort if an observer throws an
+ * error or if the response includes an object with an error type property.
+ *
+ * Any successful observer responses before abort will be included in the returned package.
  *
  * @param {Object} observers The registered observers to omit to.
  * @param {string} eventType The event type being emitted.
  * @param {*}      data      Data passed along to the observer when it is invoked.
  *
- * @return {Promise} Returns a promise that resolves to either boolean or the return value of the aborted observer.
+ * @return {Promise} Returns a promise that resolves to either boolean, or an array of responses
+ *                   from registered observers that were invoked up to the point of an error.
  */
 export const emitEventWithAbort = async (
 	observers: EventObserversType,
 	eventType: string,
 	data: unknown
-): Promise< unknown > => {
+): Promise< Array< unknown > > => {
+	const observerResponses = [];
 	const observersByType = getObserversByPriority( observers, eventType );
 	for ( const observer of observersByType ) {
 		try {
@@ -67,16 +72,24 @@ export const emitEventWithAbort = async (
 			}
 			if ( ! response.hasOwnProperty( 'type' ) ) {
 				throw new Error(
-					'If you want to abort event emitter processing, your observer must return an object with a type property'
+					'Returned objects from event emitter observers must return an object with a type property'
 				);
 			}
-			return response;
+			if ( isErrorResponse( response ) || isFailResponse( response ) ) {
+				observerResponses.push( response );
+				// early abort.
+				return observerResponses;
+			}
+			// all potential abort conditions have been considered push the
+			// response to the array.
+			observerResponses.push( response );
 		} catch ( e ) {
 			// We don't handle thrown errors but just console.log for troubleshooting.
 			// eslint-disable-next-line no-console
 			console.error( e );
-			return { type: 'error' };
+			observerResponses.push( { type: 'error' } );
+			return observerResponses;
 		}
 	}
-	return true;
+	return observerResponses;
 };

assets/js/base/context/event-emit/test/emitters.js

@@ -30,6 +30,13 @@ describe( 'Testing emitters', () => {
 				'observerPromiseWithResolvedValue',
 				{ priority: 10, callback: observerPromiseWithResolvedValue },
 			],
+			[
+				'observerSuccessType',
+				{
+					priority: 10,
+					callback: jest.fn().mockReturnValue( { type: 'success' } ),
+				},
+			],
 		] );
 	} );
 	describe( 'Testing emitEvent()', () => {
@@ -39,11 +46,11 @@ describe( 'Testing emitters', () => {
 			expect( console ).toHaveErroredWith( 'an error' );
 			expect( observerA ).toHaveBeenCalledTimes( 1 );
 			expect( observerB ).toHaveBeenCalledWith( 'foo' );
-			expect( response ).toBe( true );
+			expect( response ).toEqual( [ { type: 'success' } ] );
 		} );
 	} );
 	describe( 'Testing emitEventWithAbort()', () => {
-		it( 'does not abort on any return value other than an object with a type property', async () => {
+		it( 'does not abort on any return value other than an object with an error or fail type property', async () => {
 			observerMocks.delete( 'observerPromiseWithReject' );
 			const observers = { test: observerMocks };
 			const response = await emitEventWithAbort(
@@ -54,17 +61,16 @@ describe( 'Testing emitters', () => {
 			expect( console ).not.toHaveErrored();
 			expect( observerB ).toHaveBeenCalledTimes( 1 );
 			expect( observerPromiseWithResolvedValue ).toHaveBeenCalled();
-			expect( response ).toBe( true );
+			expect( response ).toEqual( [ { type: 'success' } ] );
 		} );
-		it( 'Aborts on a return value with an object that has a type property', async () => {
+		it( 'Aborts on a return value with an object that has a a fail type property', async () => {
 			const validObjectResponse = jest
 				.fn()
-				.mockReturnValue( { type: 'success' } );
+				.mockReturnValue( { type: 'failure' } );
 			observerMocks.set( 'observerValidObject', {
 				priority: 5,
 				callback: validObjectResponse,
 			} );
-			observerMocks.delete( 'observerPromiseWithReject' );
 			const observers = { test: observerMocks };
 			const response = await emitEventWithAbort(
 				observers,
@@ -74,7 +80,7 @@ describe( 'Testing emitters', () => {
 			expect( console ).not.toHaveErrored();
 			expect( validObjectResponse ).toHaveBeenCalledTimes( 1 );
 			expect( observerPromiseWithResolvedValue ).not.toHaveBeenCalled();
-			expect( response ).toEqual( { type: 'success' } );
+			expect( response ).toEqual( [ { type: 'failure' } ] );
 		} );
 		it( 'throws an error on an object returned from observer without a type property', async () => {
 			const failingObjectResponse = jest.fn().mockReturnValue( {} );
@@ -91,7 +97,7 @@ describe( 'Testing emitters', () => {
 			expect( console ).toHaveErrored();
 			expect( failingObjectResponse ).toHaveBeenCalledTimes( 1 );
 			expect( observerPromiseWithResolvedValue ).not.toHaveBeenCalled();
-			expect( response ).toEqual( { type: 'error' } );
+			expect( response ).toEqual( [ { type: 'error' } ] );
 		} );
 	} );
 	describe( 'Test Priority', () => {

assets/js/base/context/hooks/use-emit-response.js

@@ -25,19 +25,19 @@ const noticeContexts = {
 	EXPRESS_PAYMENTS: 'wc/express-payment-area',
 };
 
-const isSuccessResponse = ( response ) => {
+export const isSuccessResponse = ( response ) => {
 	return isResponseOf( response, responseTypes.SUCCESS );
 };
 
-const isErrorResponse = ( response ) => {
+export const isErrorResponse = ( response ) => {
 	return isResponseOf( response, responseTypes.ERROR );
 };
 
-const isFailResponse = ( response ) => {
+export const isFailResponse = ( response ) => {
 	return isResponseOf( response, responseTypes.FAIL );
 };
 
-const shouldRetry = ( response ) => {
+export const shouldRetry = ( response ) => {
 	return typeof response.retry === 'undefined' || response.retry === true;
 };
 

assets/js/base/context/providers/add-to-cart-form/form-state/index.js

@@ -206,17 +206,31 @@ export const AddToCartFormStateContextProvider = ( {
 	 */
 	useEffect( () => {
 		if ( addToCartFormState.status === STATUS.AFTER_PROCESSING ) {
+			// @todo: This data package differs from what is passed through in
+			// the checkout state context. Should we introduce a "context"
+			// property in the data package for this emitted event so that
+			// observers are able to know what context the event is firing in?
 			const data = {
 				processingResponse: addToCartFormState.processingResponse,
 			};
 
-			const handleErrorResponse = ( response ) => {
-				if ( response.message ) {
-					const errorOptions = response.messageContext
-						? { context: response.messageContext }
-						: undefined;
-					addErrorNotice( response.message, errorOptions );
-				}
+			const handleErrorResponse = ( observerResponses ) => {
+				let handled = false;
+				observerResponses.forEach( ( response ) => {
+					const { message, messageContext } = response;
+					if (
+						( isErrorResponse( response ) ||
+							isFailResponse( response ) ) &&
+						message
+					) {
+						const errorOptions = messageContext
+							? { context: messageContext }
+							: undefined;
+						handled = true;
+						addErrorNotice( message, errorOptions );
+					}
+				} );
+				return handled;
 			};
 
 			if ( addToCartFormState.hasError ) {
@@ -225,13 +239,8 @@ export const AddToCartFormStateContextProvider = ( {
 					currentObservers,
 					EMIT_TYPES.ADD_TO_CART_AFTER_PROCESSING_WITH_ERROR,
 					data
-				).then( ( response ) => {
-					if (
-						isErrorResponse( response ) ||
-						isFailResponse( response )
-					) {
-						handleErrorResponse( response );
-					} else {
+				).then( ( observerResponses ) => {
+					if ( ! handleErrorResponse( observerResponses ) ) {
 						// no error handling in place by anything so let's fall back to default
 						const message =
 							data.processingResponse?.message ||
@@ -252,12 +261,8 @@ export const AddToCartFormStateContextProvider = ( {
 				currentObservers,
 				EMIT_TYPES.ADD_TO_CART_AFTER_PROCESSING_WITH_SUCCESS,
 				data
-			).then( ( response ) => {
-				if (
-					isErrorResponse( response ) ||
-					isFailResponse( response )
-				) {
-					handleErrorResponse( response );
+			).then( ( observerResponses ) => {
+				if ( handleErrorResponse( observerResponses ) ) {
 					// this will set an error which will end up
 					// triggering the onAddToCartAfterProcessingWithError emitter.
 					// and then setting to IDLE state.

assets/js/base/context/providers/cart-checkout/checkout-state/index.js

@@ -260,6 +260,26 @@ export const CheckoutStateProvider = ( {
 		) {
 			return;
 		}
+
+		const handleErrorResponse = ( observerResponses ) => {
+			let errorResponse = null;
+			observerResponses.forEach( ( response ) => {
+				const { message, messageContext } = response;
+				if (
+					( isErrorResponse( response ) ||
+						isFailResponse( response ) ) &&
+					message
+				) {
+					const errorOptions = messageContext
+						? { context: messageContext }
+						: undefined;
+					errorResponse = response;
+					addErrorNotice( message, errorOptions );
+				}
+			} );
+			return errorResponse;
+		};
+
 		if ( checkoutState.status === STATUS.AFTER_PROCESSING ) {
 			const data = {
 				redirectUrl: checkoutState.redirectUrl,
@@ -275,21 +295,14 @@ export const CheckoutStateProvider = ( {
 					currentObservers.current,
 					EMIT_TYPES.CHECKOUT_AFTER_PROCESSING_WITH_ERROR,
 					data
-				).then( ( response ) => {
-					if (
-						isErrorResponse( response ) ||
-						isFailResponse( response )
-					) {
-						if ( response.message ) {
-							const errorOptions = {
-								id: response?.messageContext,
-								context: response?.messageContext,
-							};
-							addErrorNotice( response.message, errorOptions );
-						}
+				).then( ( observerResponses ) => {
+					const errorResponse = handleErrorResponse(
+						observerResponses
+					);
+					if ( errorResponse !== null ) {
 						// irrecoverable error so set complete
-						if ( ! shouldRetry( response ) ) {
-							dispatch( actions.setComplete( response ) );
+						if ( ! shouldRetry( errorResponse ) ) {
+							dispatch( actions.setComplete( errorResponse ) );
 						} else {
 							dispatch( actions.setIdle() );
 						}
@@ -326,21 +339,34 @@ export const CheckoutStateProvider = ( {
 					currentObservers.current,
 					EMIT_TYPES.CHECKOUT_AFTER_PROCESSING_WITH_SUCCESS,
 					data
-				).then( ( response ) => {
-					if ( isSuccessResponse( response ) ) {
-						dispatch( actions.setComplete( response ) );
-					} else if (
-						isErrorResponse( response ) ||
-						isFailResponse( response )
-					) {
-						if ( response.message ) {
-							const errorOptions = response.messageContext
-								? { context: response.messageContext }
+				).then( ( observerResponses ) => {
+					let successResponse, errorResponse;
+					observerResponses.forEach( ( response ) => {
+						if ( isSuccessResponse( response ) ) {
+							// the last observer response always "wins" for success.
+							successResponse = response;
+						}
+						if (
+							isErrorResponse( response ) ||
+							isFailResponse( response )
+						) {
+							errorResponse = response;
+						}
+					} );
+					if ( successResponse && ! errorResponse ) {
+						dispatch( actions.setComplete( successResponse ) );
+					} else if ( errorResponse ) {
+						if ( errorResponse.message ) {
+							const errorOptions = errorResponse.messageContext
+								? { context: errorResponse.messageContext }
 								: undefined;
-							addErrorNotice( response.message, errorOptions );
+							addErrorNotice(
+								errorResponse.message,
+								errorOptions
+							);
 						}
-						if ( ! shouldRetry( response ) ) {
-							dispatch( actions.setComplete( response ) );
+						if ( ! shouldRetry( errorResponse ) ) {
+							dispatch( actions.setComplete( errorResponse ) );
 						} else {
 							// this will set an error which will end up
 							// triggering the onCheckoutAfterProcessingWithError emitter.