Validate variations on register

WordPress · Apr 21, 2021 · 637b62b · 637b62b
1 parent 66af876
commit 637b62b
Show file tree

Hide file tree

Showing 4 changed files with 101 additions and 72 deletions.
diff --git a/packages/blocks/src/api/registration.js b/packages/blocks/src/api/registration.js
@@ -13,6 +13,7 @@ import {
 	pick,
 	pickBy,
 	some,
+	castArray,
 } from 'lodash';
 
 /**
@@ -303,11 +304,50 @@ export function registerBlockType( name, settings ) {
 		return;
 	}
 
+	validateVariations( settings );
+
 	dispatch( blocksStore ).addBlockTypes( settings );
 
 	return settings;
 }
 
+function validateVariations( settings ) {
+	if ( ! Array.isArray( settings.variations ) ) {
+		return;
+	}
+	const attributeKeys = Object.keys( settings.attributes || {} );
+	settings.variations.forEach( ( variation ) => {
+		if ( Array.isArray( variation.isActive ) ) {
+			const undefinedAttributes = variation.isActive.filter(
+				( attribute ) => ! attributeKeys.includes( attribute )
+			);
+			if ( undefinedAttributes.length === 0 ) {
+				return;
+			}
+
+			console.warn(
+				`The block "${ settings.name }"'s "${
+					variation.name
+				}" variation "isActive" array contains attributes ("${ undefinedAttributes.join(
+					'", "'
+				) }") that aren't defined in the block type. They will be ignored.`
+			);
+			variation.isActive = variation.isActive.filter( ( attribute ) =>
+				attributeKeys.includes( attribute )
+			);
+		} else if (
+			! isFunction( variation.isActive ) &&
+			typeof variation.isActive !== 'undefined' &&
+			variation.isActive !== null
+		) {
+			console.warn(
+				`The block "${ settings.name }"'s "${ variation.name }" variation "isActive" must be either an array or a function.`
+			);
+			delete variation.isActive;
+		}
+	} );
+}
+
 /**
  * Registers a new block collection to group blocks in the same namespace in the inserter.
  *
@@ -577,6 +617,8 @@ export const getBlockVariations = ( blockName, scope ) => {
  * @param {WPBlockVariation} variation Object describing a block variation.
  */
 export const registerBlockVariation = ( blockName, variation ) => {
+	const blockType = getBlockType( blockName );
+	validateVariations( { ...blockType, variations: castArray( variation ) } );
 	dispatch( blocksStore ).addBlockVariations( blockName, variation );
 };
 

diff --git a/packages/blocks/src/api/test/registration.js b/packages/blocks/src/api/test/registration.js
@@ -648,6 +648,60 @@ describe( 'blocks', () => {
 			} );
 		} );
 
+		it( "should warn and remove isActive if a variation's isActive is not an array or function", () => {
+			const blockType = {
+				save: noop,
+				category: 'text',
+				title: 'block title',
+				variations: [
+					{
+						name: 'variation-1',
+						isActive: 'ignore this',
+					},
+				],
+			};
+			registerBlockType( 'core/test-block-with-variations', blockType );
+			expect(
+				getBlockType( 'core/test-block-with-variations' ).variations
+			).toEqual( [
+				{
+					name: 'variation-1',
+				},
+			] );
+			expect( console ).toHaveWarnedWith(
+				'The block "core/test-block-with-variations"\'s "variation-1" variation "isActive" must be either an array or a function.'
+			);
+		} );
+
+		it( "should warn and remove extra attribute if a variation isActive's array contains attribute that is not defined in the block type", () => {
+			const blockType = {
+				save: noop,
+				category: 'text',
+				title: 'block title',
+				attributes: {
+					link: {},
+				},
+				variations: [
+					{
+						name: 'variation-1',
+						isActive: [ 'link', 'text' ],
+					},
+				],
+			};
+			registerBlockType( 'core/test-block-with-variations', blockType );
+			expect(
+				getBlockType( 'core/test-block-with-variations' ).variations
+			).toEqual( [
+				{
+					name: 'variation-1',
+					isActive: [ 'link' ],
+				},
+			] );
+			expect( console ).toHaveWarnedWith(
+				'The block "core/test-block-with-variations"\'s "variation-1" variation "isActive" array contains attributes ("text") that aren\'t defined in the block type. They will be ignored.'
+			);
+		} );
+
 		describe( 'applyFilters', () => {
 			afterEach( () => {
 				removeAllFilters( 'blocks.registerBlockType' );

diff --git a/packages/blocks/src/store/selectors.js b/packages/blocks/src/store/selectors.js
@@ -110,15 +110,11 @@ export function getActiveBlockVariation( state, blockName, attributes, scope ) {
 
 	const match = variations?.find( ( variation ) => {
 		if ( Array.isArray( variation.isActive ) ) {
-			const blockType = getBlockType( state, blockName );
-			const attributeKeys = Object.keys( blockType.attributes || {} );
-			return variation.isActive
-				.filter( ( attribute ) => attributeKeys.includes( attribute ) )
-				.every(
-					( attribute ) =>
-						attributes[ attribute ] ===
-						variation.attributes[ attribute ]
-				);
+			return variation.isActive.every(
+				( attribute ) =>
+					attributes[ attribute ] ===
+					variation.attributes[ attribute ]
+			);
 		}
 
 		return variation.isActive?.( attributes, variation.attributes );

diff --git a/packages/blocks/src/store/test/selectors.js b/packages/blocks/src/store/test/selectors.js
@@ -463,69 +463,6 @@ describe( 'selectors', () => {
 					} )
 				).toEqual( variations[ 2 ] );
 			} );
-			it( 'should ignore attributes that are not defined in the block type', () => {
-				const variations = [
-					{
-						name: 'variation-1',
-						attributes: {
-							firstTestAttribute: 1,
-							secondTestAttribute: 10,
-							undefinedTestAttribute: 100,
-						},
-						isActive: [
-							'firstTestAttribute',
-							'secondTestAttribute',
-							'undefinedTestAttribute',
-						],
-					},
-					{
-						name: 'variation-2',
-						attributes: {
-							firstTestAttribute: 2,
-							secondTestAttribute: 20,
-							undefinedTestAttribute: 200,
-						},
-						isActive: [
-							'firstTestAttribute',
-							'secondTestAttribute',
-							'undefinedTestAttribute',
-						],
-					},
-				];
-
-				const state = createBlockVariationsStateWithTestBlockType(
-					variations
-				);
-
-				expect(
-					getActiveBlockVariation( state, blockName, {
-						firstTestAttribute: 1,
-						secondTestAttribute: 10,
-						undefinedTestAttribute: 100,
-					} )
-				).toEqual( variations[ 0 ] );
-				expect(
-					getActiveBlockVariation( state, blockName, {
-						firstTestAttribute: 1,
-						secondTestAttribute: 10,
-						undefinedTestAttribute: 1234,
-					} )
-				).toEqual( variations[ 0 ] );
-				expect(
-					getActiveBlockVariation( state, blockName, {
-						firstTestAttribute: 2,
-						secondTestAttribute: 20,
-						undefinedTestAttribute: 200,
-					} )
-				).toEqual( variations[ 1 ] );
-				expect(
-					getActiveBlockVariation( state, blockName, {
-						firstTestAttribute: 2,
-						secondTestAttribute: 20,
-						undefinedTestAttribute: 2345,
-					} )
-				).toEqual( variations[ 1 ] );
-			} );
 		} );
 		describe( 'getDefaultBlockVariation', () => {
 			it( 'should return the default variation when set', () => {