Fix React hook dependency warnings in Cart & Checkout blocks + withAttributes HOC (https://github.com/woocommerce/woocommerce-blocks/pull/3314)

* Fix React hook dependency warnings in Cart & Checkout blocks

* Fix React hook dependency warnings in withAttributes HOC

* Fix select validation

* Fix test

* Remove unnecessary optional chaining

* Undo merge of two useEffects
This commit is contained in:
Albert Juhé Lluveras 2020-10-27 15:37:18 +01:00 committed by GitHub
parent f66aa18e43
commit 55e1a15149
9 changed files with 122 additions and 96 deletions

View File

@ -100,7 +100,8 @@ const AddressForm = ( {
}
}, [
values,
countryValidationError,
countryValidationError.message,
countryValidationError.hidden,
setValidationErrors,
clearValidationError,
type,

View File

@ -27,7 +27,7 @@ const StoryComponent = ( { label, errorMessage } ) => {
} = useValidationContext();
useEffect( () => {
setValidationErrors( { country: errorMessage } );
}, [ errorMessage ] );
}, [ errorMessage, setValidationErrors ] );
const updateCountry = ( country ) => {
clearValidationError( 'country' );
selectCountry( country );

View File

@ -41,7 +41,7 @@ const ValidatedSelect = ( {
clearValidationError,
} = useValidationContext();
const validateSelect = () => {
useEffect( () => {
if ( ! required || currentValue ) {
clearValidationError( errorId );
} else {
@ -52,18 +52,21 @@ const ValidatedSelect = ( {
},
} );
}
};
useEffect( () => {
validateSelect();
}, [ currentValue ] );
}, [
clearValidationError,
currentValue,
errorId,
errorMessage,
required,
setValidationErrors,
] );
// Remove validation errors when unmounted.
useEffect( () => {
return () => {
clearValidationError( errorId );
};
}, [ errorId ] );
}, [ clearValidationError, errorId ] );
const error = getValidationError( errorId ) || {};

View File

@ -2,7 +2,7 @@
* External dependencies
*/
import { __ } from '@wordpress/i18n';
import { useRef, useEffect } from 'react';
import { useCallback, useRef, useEffect, useState } from 'react';
import PropTypes from 'prop-types';
import classnames from 'classnames';
import { useValidationContext } from '@woocommerce/base-context';
@ -27,6 +27,7 @@ const ValidatedTextInput = ( {
showError = true,
...rest
} ) => {
const [ isPristine, setIsPristine ] = useState( true );
const inputRef = useRef();
const {
getValidationError,
@ -39,39 +40,51 @@ const ValidatedTextInput = ( {
const textInputId = id || 'textinput-' + instanceId;
errorId = errorId || textInputId;
const validateInput = ( errorsHidden = true ) => {
if ( inputRef.current.checkValidity() ) {
clearValidationError( errorId );
} else {
setValidationErrors( {
[ errorId ]: {
message:
inputRef.current.validationMessage ||
__( 'Invalid value.', 'woo-gutenberg-products-block' ),
hidden: errorsHidden,
},
} );
}
};
const validateInput = useCallback(
( errorsHidden = true ) => {
if ( inputRef.current.checkValidity() ) {
clearValidationError( errorId );
} else {
setValidationErrors( {
[ errorId ]: {
message:
inputRef.current.validationMessage ||
__(
'Invalid value.',
'woo-gutenberg-products-block'
),
hidden: errorsHidden,
},
} );
}
},
[ clearValidationError, errorId, setValidationErrors ]
);
useEffect( () => {
if ( focusOnMount ) {
inputRef.current.focus();
if ( isPristine ) {
if ( focusOnMount ) {
inputRef.current.focus();
}
setIsPristine( false );
}
}, [ focusOnMount ] );
}, [ focusOnMount, isPristine, setIsPristine ] );
useEffect( () => {
if ( validateOnMount ) {
validateInput();
if ( isPristine ) {
if ( validateOnMount ) {
validateInput();
}
setIsPristine( false );
}
}, [ validateOnMount ] );
}, [ isPristine, setIsPristine, validateOnMount, validateInput ] );
// Remove validation errors when unmounted.
useEffect( () => {
return () => {
clearValidationError( errorId );
};
}, [ errorId ] );
}, [ clearValidationError, errorId ] );
const errorMessage = getValidationError( errorId ) || {};
const hasError = errorMessage.message && ! errorMessage.hidden;

View File

@ -55,6 +55,7 @@ export const useStoreCartItemQuantity = ( cartItem ) => {
},
[ cartItemKey ]
);
const previousIsPendingQuantity = usePrevious( isPendingQuantity );
const isPendingDelete = useSelect(
( select ) => {
@ -67,6 +68,7 @@ export const useStoreCartItemQuantity = ( cartItem ) => {
},
[ cartItemKey ]
);
const previousIsPendingDelete = usePrevious( isPendingDelete );
const removeItem = () => {
return cartItemKey
@ -78,34 +80,46 @@ export const useStoreCartItemQuantity = ( cartItem ) => {
// Observe debounced quantity value, fire action to update server on change.
useEffect( () => {
// Don't run it if quantity didn't change but it was set for the first time.
if ( cartItemKey && Number.isFinite( previousDebouncedQuantity ) ) {
if (
cartItemKey &&
Number.isFinite( previousDebouncedQuantity ) &&
previousDebouncedQuantity !== debouncedQuantity
) {
changeCartItemQuantity( cartItemKey, debouncedQuantity ).then(
triggerFragmentRefresh
);
}
}, [ debouncedQuantity, cartItemKey ] );
}, [
cartItemKey,
changeCartItemQuantity,
debouncedQuantity,
previousDebouncedQuantity,
] );
useEffect( () => {
if ( isPendingQuantity ) {
dispatchActions.incrementCalculating();
} else {
dispatchActions.decrementCalculating();
if ( previousIsPendingQuantity !== isPendingQuantity ) {
if ( isPendingQuantity ) {
dispatchActions.incrementCalculating();
} else {
dispatchActions.decrementCalculating();
}
}
}, [ isPendingQuantity ] );
}, [ dispatchActions, isPendingQuantity, previousIsPendingQuantity ] );
useEffect( () => {
if ( isPendingDelete ) {
dispatchActions.incrementCalculating();
} else if ( ! isPendingDelete && cartErrors.length ) {
dispatchActions.decrementCalculating();
if ( previousIsPendingDelete !== isPendingDelete ) {
if ( isPendingDelete ) {
dispatchActions.incrementCalculating();
} else {
dispatchActions.decrementCalculating();
}
}
return () => {
if ( isPendingDelete ) {
dispatchActions.decrementCalculating();
}
};
}, [ isPendingDelete ] );
}, [ dispatchActions, isPendingDelete, previousIsPendingDelete ] );
return {
isPendingDelete,

View File

@ -85,7 +85,7 @@ const Cart = ( { attributes } ) => {
id: error.code,
} );
} );
}, [ cartItemErrors ] );
}, [ addErrorNotice, cartItemErrors ] );
const totalsCurrency = getCurrencyFromPriceResponse( cartTotals );

View File

@ -40,13 +40,7 @@ describe( 'Testing cart', () => {
screen.getByText( /Proceed to Checkout/i )
).toBeInTheDocument();
/**
* @todo Investigate extra POST request on initial cart render.
*
* We have an unfixed bug in our test in which the full cart triggers a POST
* request to `wc/store/cart/update-item` causing the fetch to be called twice.
*/
expect( fetchMock ).toHaveBeenCalledTimes( 2 );
expect( fetchMock ).toHaveBeenCalledTimes( 1 );
} );
it( 'renders empty cart if there are no items in the cart', async () => {
fetchMock.mockResponse( ( req ) => {

View File

@ -3,7 +3,6 @@
*/
import { useState, useEffect } from '@wordpress/element';
import { getAttributes, getTerms } from '@woocommerce/editor-components/utils';
import { find } from 'lodash';
/**
* Internal dependencies
@ -14,12 +13,12 @@ import { formatError } from '../base/utils/errors.js';
* Get attribute data (name, taxonomy etc) from server data.
*
* @param {number} attributeId Attribute ID to look for.
* @param {Array} attributeList List of attributes.
* @param {Array|null} attributeList List of attributes.
* @param {string} matchField Field to match on. e.g. id or slug.
*/
const getAttributeData = ( attributeId, attributeList, matchField = 'id' ) => {
return attributeList
? find( attributeList, [ matchField, attributeId ] )
return Array.isArray( attributeList )
? attributeList.find( ( attr ) => attr[ matchField ] === attributeId )
: null;
};
@ -30,8 +29,9 @@ const getAttributeData = ( attributeId, attributeList, matchField = 'id' ) => {
*/
const withAttributes = ( OriginalComponent ) => {
return ( props ) => {
const { selected } = props;
const [ attributes, setAttributes ] = useState( [] );
const { selected = [] } = props;
const selectedSlug = selected.length ? selected[ 0 ].attr_slug : null;
const [ attributes, setAttributes ] = useState( null );
const [ expandedAttribute, setExpandedAttribute ] = useState( 0 );
const [ termsList, setTermsList ] = useState( {} );
const [ loading, setLoading ] = useState( true );
@ -39,43 +39,44 @@ const withAttributes = ( OriginalComponent ) => {
const [ error, setError ] = useState( null );
useEffect( () => {
getAttributes()
.then( ( newAttributes ) => {
newAttributes = newAttributes.map( ( attribute ) => ( {
...attribute,
parent: 0,
} ) );
if ( attributes === null ) {
getAttributes()
.then( ( newAttributes ) => {
newAttributes = newAttributes.map( ( attribute ) => ( {
...attribute,
parent: 0,
} ) );
setAttributes( newAttributes );
setAttributes( newAttributes );
if ( selected.length > 0 ) {
const selectedAttributeFromTerm = attributes
? getAttributeData(
selected[ 0 ].attr_slug,
newAttributes,
'taxonomy'
)
: null;
if ( selectedAttributeFromTerm ) {
setExpandedAttribute(
selectedAttributeFromTerm.id
if ( selectedSlug ) {
const selectedAttributeFromTerm = getAttributeData(
selectedSlug,
newAttributes,
'taxonomy'
);
if ( selectedAttributeFromTerm ) {
setExpandedAttribute(
selectedAttributeFromTerm.id
);
}
}
}
} )
.catch( async ( e ) => {
setError( await formatError( e ) );
} )
.finally( () => {
setLoading( false );
} );
}, [] );
} )
.catch( async ( e ) => {
setError( await formatError( e ) );
} )
.finally( () => {
setLoading( false );
} );
}
}, [ attributes, selectedSlug ] );
useEffect( () => {
const attributeData = attributes
? getAttributeData( expandedAttribute, attributes )
: null;
const attributeData = getAttributeData(
expandedAttribute,
attributes
);
if ( ! attributeData ) {
return;
@ -91,10 +92,10 @@ const withAttributes = ( OriginalComponent ) => {
attr_slug: attributeData.taxonomy,
} ) );
setTermsList( {
...termsList,
setTermsList( ( previousTermsList ) => ( {
...previousTermsList,
[ expandedAttribute ]: newTerms,
} );
} ) );
} )
.catch( async ( e ) => {
setError( await formatError( e ) );
@ -107,7 +108,7 @@ const withAttributes = ( OriginalComponent ) => {
return (
<OriginalComponent
{ ...props }
attributes={ attributes }
attributes={ attributes || [] }
error={ error }
expandedAttribute={ expandedAttribute }
onExpandAttribute={ setExpandedAttribute }

View File

@ -86,11 +86,11 @@ export const useElementOptions = ( overloadedOptions ) => {
? { showIcon: isActive }
: {};
return {
...options,
...prevOptions,
style: {
...options.style,
...prevOptions.style,
base: {
...options.style.base,
...prevOptions.style.base,
'::placeholder': {
color,
},