Merge pull request #8927 from google/enhancement/8904-validate-have-s…

…ettings-change-alignment

Validate have settings change alignment

assets/js/googlesitekit/data/create-settings-store.js

@@ -32,7 +32,7 @@ import {
 	commonStore,
 	combineStores,
 } from 'googlesitekit-data';
-import { createStrictSelect } from './utils';
+import { createStrictSelect, createValidationSelector } from './utils';
 import {
 	camelCaseToPascalCase,
 	camelCaseToConstantCase,
@@ -267,7 +267,27 @@ export const createSettingsStore = (
 		},
 	};
 
+	const {
+		safeSelector: haveSettingsChanged,
+		dangerousSelector: __dangerousHaveSettingsChanged,
+	} = createValidationSelector( validateHaveSettingsChanged );
+
 	const selectors = {
+		/**
+		 * Indicates whether the current settings have changed from what is saved.
+		 *
+		 * @since 1.6.0
+		 * @since 1.77.0 Added ability to filter settings using `keys` argument.
+		 * @since 1.129.0 Changed the approach to use validateHaveSettingsChanged callback.
+		 * @since n.e.x.t Updated implementation to use safeSelector and dangerousSelector returned from createValidationSelector.
+		 *
+		 * @param {Object}     state Data store's state.
+		 * @param {Array|null} keys  Settings keys to check; if not provided, all settings are checked.
+		 * @return {boolean} True if the settings have changed, false otherwise.
+		 */
+		haveSettingsChanged,
+		__dangerousHaveSettingsChanged,
+
 		/**
 		 * Gets the current settings.
 		 *
@@ -282,23 +302,6 @@ export const createSettingsStore = (
 			return state.settings;
 		},
 
-		/**
-		 * Indicates whether the current settings have changed from what is saved.
-		 *
-		 * @since 1.6.0
-		 * @since 1.77.0 Added ability to filter settings using `keys` argument.
-		 * @since 1.129.0 Changed the approach to use validateHaveSettingsChanged callback.
-		 *
-		 * @param {Object}     state Data store's state.
-		 * @param {Array|null} keys  Settings keys to check; if not provided, all settings are checked.
-		 * @return {boolean} True if the settings have changed, false otherwise.
-		 */
-		haveSettingsChanged: createRegistrySelector(
-			( select ) =>
-				( state, ...args ) =>
-					validateHaveSettingsChanged( select, state, ...args )
-		),
-
 		/**
 		 * Indicates whether the provided setting has changed from what is saved.
 		 *
@@ -513,12 +516,18 @@ export function makeDefaultHaveSettingsChanged() {
 		const { settings, savedSettings } = state;
 
 		if ( keys ) {
-			return ! isEqual(
-				pick( settings, keys ),
-				pick( savedSettings, keys )
+			invariant(
+				! isEqual(
+					pick( settings, keys ),
+					pick( savedSettings, keys )
+				),
+				INVARIANT_SETTINGS_NOT_CHANGED
 			);
 		}
 
-		return ! isEqual( settings, savedSettings );
+		invariant(
+			! isEqual( settings, savedSettings ),
+			INVARIANT_SETTINGS_NOT_CHANGED
+		);
 	};
 }

assets/js/googlesitekit/data/create-settings-store.test.js

@@ -32,6 +32,7 @@ import {
 	untilResolved,
 } from '../../../../tests/js/utils';
 import { createSettingsStore } from './create-settings-store';
+import { CORE_SITE } from '../datastore/site/constants';
 
 const STORE_ARGS = [ 'core', 'site', 'settings' ];
 
@@ -385,6 +386,49 @@ describe( 'createSettingsStore store', () => {
 			} );
 		} );
 
+		test.each( [
+			[ 'haveSettingsChanged' ],
+			[ '__dangerousHaveSettingsChanged' ],
+		] )( 'should have %s selector', ( selector ) => {
+			const selectors = storeDefinition.selectors;
+			expect( typeof selectors[ selector ] ).toBe( 'function' );
+		} );
+
+		describe.each( [
+			[ 'haveSettingsChanged' ],
+			[ '__dangerousHaveSettingsChanged' ],
+		] )( '%s', ( selector ) => {
+			it( 'should use provided validateHaveSettingsChanged function', () => {
+				const validateHaveSettingsChanged = jest.fn();
+				storeDefinition = createSettingsStore( ...STORE_ARGS, {
+					settingSlugs: [ 'isSkyBlue' ],
+					validateHaveSettingsChanged,
+					registry,
+				} );
+
+				storeDefinition.selectors[ selector ]();
+				expect( validateHaveSettingsChanged ).toHaveBeenCalled();
+			} );
+		} );
+
+		describe( '__dangerousHaveSettingsChanged', () => {
+			it( 'should throw an exception from validateHaveSettingsChanged when error occurs', () => {
+				const validateHaveSettingsChanged = null;
+
+				createSettingsStore( ...STORE_ARGS, {
+					settingSlugs: [ 'isSkyBlue' ],
+					validateHaveSettingsChanged,
+					registry,
+				} );
+
+				expect( () =>
+					registry
+						.select( CORE_SITE )
+						.__dangerousHaveSettingsChanged()
+				).toThrow();
+			} );
+		} );
+
 		describe( 'haveSettingsChanged', () => {
 			it( 'informs whether client-side settings differ from server-side ones', async () => {
 				// Initially false.
@@ -489,6 +533,21 @@ describe( 'createSettingsStore store', () => {
 				// an `undefined` keys array.
 				expect( select.haveSettingsChanged( [] ) ).toEqual( false );
 			} );
+
+			it( 'should not throw an exception', () => {
+				const validateHaveSettingsChanged = null;
+
+				createSettingsStore( ...STORE_ARGS, {
+					settingSlugs: [ 'isSkyBlue' ],
+					validateHaveSettingsChanged,
+					registry,
+				} );
+
+				// Since selector is invalid, it should return false as exception would be caught by the safeSelector.
+				expect(
+					registry.select( CORE_SITE ).haveSettingsChanged()
+				).toBe( false );
+			} );
 		} );
 
 		describe( 'haveOwnedSettingsChanged', () => {

assets/js/modules/ads/datastore/settings.js

@@ -100,14 +100,16 @@ export function validateHaveSettingsChanged( select, state, keys ) {
 		select( CORE_SITE ).haveConversionTrackingSettingsChanged();
 
 	if ( keys ) {
-		return (
-			! isEqual( pick( settings, keys ), pick( savedSettings, keys ) ) ||
-			haveConversionTrackingSettingsChanged
+		invariant(
+			isEqual( pick( settings, keys ), pick( savedSettings, keys ) ) ||
+				! haveConversionTrackingSettingsChanged,
+			INVARIANT_SETTINGS_NOT_CHANGED
 		);
 	}
 
-	return (
+	invariant(
 		! isEqual( settings, savedSettings ) ||
-		haveConversionTrackingSettingsChanged
+			haveConversionTrackingSettingsChanged,
+		INVARIANT_SETTINGS_NOT_CHANGED
 	);
 }

assets/js/modules/analytics-4/datastore/settings.js

@@ -307,14 +307,16 @@ export function validateHaveSettingsChanged( select, state, keys ) {
 		select( CORE_SITE ).haveConversionTrackingSettingsChanged();
 
 	if ( keys ) {
-		return (
+		invariant(
 			! isEqual( pick( settings, keys ), pick( savedSettings, keys ) ) ||
-			haveConversionTrackingSettingsChanged
+				haveConversionTrackingSettingsChanged,
+			INVARIANT_SETTINGS_NOT_CHANGED
 		);
 	}
 
-	return (
+	invariant(
 		! isEqual( settings, savedSettings ) ||
-		haveConversionTrackingSettingsChanged
+			haveConversionTrackingSettingsChanged,
+		INVARIANT_SETTINGS_NOT_CHANGED
 	);
 }