Skip to content
This repository has been archived by the owner on Feb 23, 2024. It is now read-only.

Commit

Permalink
Improvements to emitEventWithAbort. (#4158)
Browse files Browse the repository at this point in the history 
* 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.
  • Loading branch information
@nerrad @grogou
nerrad authored and grogou committed Aug 20, 2021
1 parent 2be9a4e commit ed7eded
Show file tree
Hide file tree
Showing 6 changed files with 154 additions and 85 deletions.
29 changes: 21 additions & 8 deletions assets/js/base/context/event-emit/emitters.ts
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down Expand Up @@ -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 {
Expand All @@ -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;
};
22 changes: 14 additions & 8 deletions assets/js/base/context/event-emit/test/emitters.js
View file
Original file line number Diff line number Diff line change
Expand Up @@ -30,6 +30,13 @@ describe( 'Testing emitters', () => {
'observerPromiseWithResolvedValue',
{ priority: 10, callback: observerPromiseWithResolvedValue },
],
[
'observerSuccessType',
{
priority: 10,
callback: jest.fn().mockReturnValue( { type: 'success' } ),
},
],
] );
} );
describe( 'Testing emitEvent()', () => {
Expand All @@ -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(
Expand All @@ -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,
Expand All @@ -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( {} );
Expand All @@ -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', () => {
Expand Down
8 changes: 4 additions & 4 deletions assets/js/base/context/hooks/use-emit-response.js
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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;
};

Expand Down
45 changes: 25 additions & 20 deletions assets/js/base/context/providers/add-to-cart-form/form-state/index.js
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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 ) {
Expand All @@ -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 ||
Expand All @@ -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.
Expand Down
80 changes: 53 additions & 27 deletions assets/js/base/context/providers/cart-checkout/checkout-state/index.js
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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,
Expand All @@ -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() );
}
Expand Down Expand Up @@ -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.
Expand Down
Loading

0 comments on commit ed7eded

Please sign in to comment.