From 64f72f82f558d3425bcd298a38da4709c0e94de6 Mon Sep 17 00:00:00 2001 From: Colin Rotherham Date: Fri, 23 Feb 2024 15:18:41 +0000 Subject: [PATCH 01/12] Check schema conditions are arrays --- .../govuk-frontend/src/govuk/common/index.mjs | 16 +++++++++------- 1 file changed, 9 insertions(+), 7 deletions(-) diff --git a/packages/govuk-frontend/src/govuk/common/index.mjs b/packages/govuk-frontend/src/govuk/common/index.mjs index c3f21d11e2..b7700147bf 100644 --- a/packages/govuk-frontend/src/govuk/common/index.mjs +++ b/packages/govuk-frontend/src/govuk/common/index.mjs @@ -234,15 +234,17 @@ export function validateConfig(schema, config) { const errors = [] // Check errors for each schema condition - for (const { required, errorMessage } of conditions) { - if (!required.every((key) => !!config[key])) { - errors.push(errorMessage) // Missing config key value + if (Array.isArray(conditions)) { + for (const { required, errorMessage } of conditions) { + if (!required.every((key) => !!config[key])) { + errors.push(errorMessage) // Missing config key value + } } - } - // Check one condition passes or add errors - if (name === 'anyOf' && !(conditions.length - errors.length >= 1)) { - validationErrors.push(...errors) + // Check one condition passes or add errors + if (name === 'anyOf' && !(conditions.length - errors.length >= 1)) { + validationErrors.push(...errors) + } } } From 31efb1b967aa527ce2a18843cc44c384a7f56398 Mon Sep 17 00:00:00 2001 From: Colin Rotherham Date: Fri, 23 Feb 2024 15:13:17 +0000 Subject: [PATCH 02/12] Add config schema to all components This configured allowed types for config fields so we can check them in future --- .../govuk-frontend/src/govuk/common/index.mjs | 8 ++++++++ .../govuk/components/accordion/accordion.mjs | 17 +++++++++++++++++ .../src/govuk/components/button/button.mjs | 16 ++++++++++++++++ .../character-count/character-count.mjs | 6 ++++++ .../components/error-summary/error-summary.mjs | 16 ++++++++++++++++ .../exit-this-page/exit-this-page.mjs | 16 ++++++++++++++++ .../notification-banner/notification-banner.mjs | 16 ++++++++++++++++ 7 files changed, 95 insertions(+) diff --git a/packages/govuk-frontend/src/govuk/common/index.mjs b/packages/govuk-frontend/src/govuk/common/index.mjs index b7700147bf..cc2539a210 100644 --- a/packages/govuk-frontend/src/govuk/common/index.mjs +++ b/packages/govuk-frontend/src/govuk/common/index.mjs @@ -255,9 +255,17 @@ export function validateConfig(schema, config) { * Schema for component config * * @typedef {object} Schema + * @property {{ [field: string]: SchemaProperty }} properties - Schema properties * @property {SchemaCondition[]} [anyOf] - List of schema conditions */ +/** + * Schema property for component config + * + * @typedef {object} SchemaProperty + * @property {'string' | 'boolean' | 'number' | 'object'} type - Property type + */ + /** * Schema condition for component config * diff --git a/packages/govuk-frontend/src/govuk/components/accordion/accordion.mjs b/packages/govuk-frontend/src/govuk/components/accordion/accordion.mjs index 479a048e39..d7b978dd0b 100644 --- a/packages/govuk-frontend/src/govuk/components/accordion/accordion.mjs +++ b/packages/govuk-frontend/src/govuk/components/accordion/accordion.mjs @@ -611,6 +611,19 @@ export class Accordion extends GOVUKFrontendComponent { }, rememberExpanded: true }) + + /** + * Accordion config schema + * + * @constant + * @satisfies {Schema} + */ + static schema = Object.freeze({ + properties: { + i18n: { type: 'object' }, + rememberExpanded: { type: 'boolean' } + } + }) } const helper = { @@ -666,3 +679,7 @@ const helper = { * @property {string} [showSectionAriaLabel] - The text content appended to the * 'Show' button's accessible name when a section is expanded. */ + +/** + * @typedef {import('../../common/index.mjs').Schema} Schema + */ diff --git a/packages/govuk-frontend/src/govuk/components/button/button.mjs b/packages/govuk-frontend/src/govuk/components/button/button.mjs index 23b3b589a4..7a199631b1 100644 --- a/packages/govuk-frontend/src/govuk/components/button/button.mjs +++ b/packages/govuk-frontend/src/govuk/components/button/button.mjs @@ -128,6 +128,18 @@ export class Button extends GOVUKFrontendComponent { static defaults = Object.freeze({ preventDoubleClick: false }) + + /** + * Button config schema + * + * @constant + * @satisfies {Schema} + */ + static schema = Object.freeze({ + properties: { + preventDoubleClick: { type: 'boolean' } + } + }) } /** @@ -137,3 +149,7 @@ export class Button extends GOVUKFrontendComponent { * @property {boolean} [preventDoubleClick=false] - Prevent accidental double * clicks on submit buttons from submitting forms multiple times. */ + +/** + * @typedef {import('../../common/index.mjs').Schema} Schema + */ diff --git a/packages/govuk-frontend/src/govuk/components/character-count/character-count.mjs b/packages/govuk-frontend/src/govuk/components/character-count/character-count.mjs index e1d6905d81..d6554caf3c 100644 --- a/packages/govuk-frontend/src/govuk/components/character-count/character-count.mjs +++ b/packages/govuk-frontend/src/govuk/components/character-count/character-count.mjs @@ -459,6 +459,12 @@ export class CharacterCount extends GOVUKFrontendComponent { * @satisfies {Schema} */ static schema = Object.freeze({ + properties: { + i18n: { type: 'object' }, + maxwords: { type: 'number' }, + maxlength: { type: 'number' }, + threshold: { type: 'number' } + }, anyOf: [ { required: ['maxwords'], diff --git a/packages/govuk-frontend/src/govuk/components/error-summary/error-summary.mjs b/packages/govuk-frontend/src/govuk/components/error-summary/error-summary.mjs index a541123312..685e8244dc 100644 --- a/packages/govuk-frontend/src/govuk/components/error-summary/error-summary.mjs +++ b/packages/govuk-frontend/src/govuk/components/error-summary/error-summary.mjs @@ -196,6 +196,18 @@ export class ErrorSummary extends GOVUKFrontendComponent { static defaults = Object.freeze({ disableAutoFocus: false }) + + /** + * Error summary config schema + * + * @constant + * @satisfies {Schema} + */ + static schema = Object.freeze({ + properties: { + disableAutoFocus: { type: 'boolean' } + } + }) } /** @@ -205,3 +217,7 @@ export class ErrorSummary extends GOVUKFrontendComponent { * @property {boolean} [disableAutoFocus=false] - If set to `true` the error * summary will not be focussed when the page loads. */ + +/** + * @typedef {import('../../common/index.mjs').Schema} Schema + */ diff --git a/packages/govuk-frontend/src/govuk/components/exit-this-page/exit-this-page.mjs b/packages/govuk-frontend/src/govuk/components/exit-this-page/exit-this-page.mjs index 2d0d9c8e37..6170250a70 100644 --- a/packages/govuk-frontend/src/govuk/components/exit-this-page/exit-this-page.mjs +++ b/packages/govuk-frontend/src/govuk/components/exit-this-page/exit-this-page.mjs @@ -437,6 +437,18 @@ export class ExitThisPage extends GOVUKFrontendComponent { pressOneMoreTime: 'Shift, press 1 more time to exit.' } }) + + /** + * Exit this page config schema + * + * @constant + * @satisfies {Schema} + */ + static schema = Object.freeze({ + properties: { + i18n: { type: 'object' } + } + }) } /** @@ -464,3 +476,7 @@ export class ExitThisPage extends GOVUKFrontendComponent { * @property {string} [pressOneMoreTime] - Screen reader announcement informing * the user they must press the activation key one more time. */ + +/** + * @typedef {import('../../common/index.mjs').Schema} Schema + */ diff --git a/packages/govuk-frontend/src/govuk/components/notification-banner/notification-banner.mjs b/packages/govuk-frontend/src/govuk/components/notification-banner/notification-banner.mjs index 95fc72f0f2..facee870dd 100644 --- a/packages/govuk-frontend/src/govuk/components/notification-banner/notification-banner.mjs +++ b/packages/govuk-frontend/src/govuk/components/notification-banner/notification-banner.mjs @@ -75,6 +75,18 @@ export class NotificationBanner extends GOVUKFrontendComponent { static defaults = Object.freeze({ disableAutoFocus: false }) + + /** + * Notification banner config schema + * + * @constant + * @satisfies {Schema} + */ + static schema = Object.freeze({ + properties: { + disableAutoFocus: { type: 'boolean' } + } + }) } /** @@ -86,3 +98,7 @@ export class NotificationBanner extends GOVUKFrontendComponent { * applies if the component has a `role` of `alert` – in other cases the * component will not be focused on page load, regardless of this option. */ + +/** + * @typedef {import('../../common/index.mjs').Schema} Schema + */ From 53024e899fd796f977152e1d960b882f8ccd4502 Mon Sep 17 00:00:00 2001 From: Colin Rotherham Date: Fri, 23 Feb 2024 18:59:58 +0000 Subject: [PATCH 03/12] Update `mergeConfigs()` helper to merge, not flatten See: https://github.com/alphagov/govuk-frontend/issues/4230 --- .../src/govuk/common/index.jsdom.test.mjs | 45 ++++++--- .../govuk-frontend/src/govuk/common/index.mjs | 94 ++++++++----------- 2 files changed, 73 insertions(+), 66 deletions(-) diff --git a/packages/govuk-frontend/src/govuk/common/index.jsdom.test.mjs b/packages/govuk-frontend/src/govuk/common/index.jsdom.test.mjs index 73bbdb73df..bde35f8b5c 100644 --- a/packages/govuk-frontend/src/govuk/common/index.jsdom.test.mjs +++ b/packages/govuk-frontend/src/govuk/common/index.jsdom.test.mjs @@ -30,33 +30,35 @@ describe('Common JS utilities', () => { } } - it('flattens a single object', () => { + it('ignores a single object', () => { const config = mergeConfigs(config1) expect(config).toEqual({ a: 'antelope', - 'c.a': 'camel' + c: { a: 'camel' } }) }) - it('flattens and merges two objects', () => { + it('merges two objects', () => { const config = mergeConfigs(config1, config2) expect(config).toEqual({ a: 'aardvark', b: 'bee', - 'c.a': 'cat', - 'c.o': 'cobra' + c: { a: 'cat', o: 'cobra' } }) }) - it('flattens and merges three objects', () => { + it('merges three objects', () => { const config = mergeConfigs(config1, config2, config3) expect(config).toEqual({ a: 'aardvark', b: 'bat', - 'c.a': 'cat', - 'c.o': 'cow', + c: { a: 'cat', o: 'cow' }, d: 'dog', - 'e.l.e': 'elephant' + e: { + l: { + e: 'elephant' + } + } }) }) @@ -75,10 +77,29 @@ describe('Common JS utilities', () => { expect(config).toEqual({ a: 'antelope', b: 'bat', - 'c.a': 'camel', - 'c.o': 'cow', + c: { a: 'camel', o: 'cow' }, d: 'dog', - 'e.l.e': 'elephant' + e: { + l: { + e: 'elephant' + } + } + }) + }) + + it('prioritises the last parameter provided (different types)', () => { + const config = mergeConfigs(config1, config2, config3, { + c: 'jellyfish', // Replaces top-level object with string + e: { l: 'shark' } // Replaces nested object with string + }) + expect(config).toEqual({ + a: 'aardvark', + b: 'bat', + c: 'jellyfish', + d: 'dog', + e: { + l: 'shark' + } }) }) diff --git a/packages/govuk-frontend/src/govuk/common/index.mjs b/packages/govuk-frontend/src/govuk/common/index.mjs index cc2539a210..cc72b8e665 100644 --- a/packages/govuk-frontend/src/govuk/common/index.mjs +++ b/packages/govuk-frontend/src/govuk/common/index.mjs @@ -7,72 +7,36 @@ */ /** - * Config flattening function + * Config merging function * - * Takes any number of objects, flattens them into namespaced key-value pairs, - * (e.g. \{'i18n.showSection': 'Show section'\}) and combines them together, with + * Takes any number of objects and combines them together, with * greatest priority on the LAST item passed in. * * @internal - * @param {...{ [key: string]: unknown }} configObjects - Config object to merge - * @returns {{ [key: string]: unknown }} A flattened object of key-value pairs. + * @param {...{ [key: string]: unknown }} configObjects - Config objects to merge + * @returns {{ [key: string]: unknown }} A merged config object */ export function mergeConfigs(...configObjects) { - /** - * Function to take nested objects and flatten them to a dot-separated keyed - * object. Doing this means we don't need to do any deep/recursive merging of - * each of our objects, nor transform our dataset from a flat list into a - * nested object. - * - * @internal - * @param {{ [key: string]: unknown }} configObject - Deeply nested object - * @returns {{ [key: string]: unknown }} Flattened object with dot-separated keys - */ - function flattenObject(configObject) { - // Prepare an empty return object - /** @type {{ [key: string]: unknown }} */ - const flattenedObject = {} - - /** - * Our flattening function, this is called recursively for each level of - * depth in the object. At each level we prepend the previous level names to - * the key using `prefix`. - * - * @internal - * @param {Partial<{ [key: string]: unknown }>} obj - Object to flatten - * @param {string} [prefix] - Optional dot-separated prefix - */ - function flattenLoop(obj, prefix) { - for (const [key, value] of Object.entries(obj)) { - const prefixedKey = prefix ? `${prefix}.${key}` : key - - // If the value is a nested object, recurse over that too - if (value && typeof value === 'object') { - flattenLoop(value, prefixedKey) - } else { - // Otherwise, add this value to our return object - flattenedObject[prefixedKey] = value - } - } - } - - // Kick off the recursive loop - flattenLoop(configObject) - return flattenedObject - } - // Start with an empty object as our base /** @type {{ [key: string]: unknown }} */ const formattedConfigObject = {} // Loop through each of the passed objects for (const configObject of configObjects) { - const obj = flattenObject(configObject) - - // Push their keys one-by-one into formattedConfigObject. Any duplicate - // keys will override the existing key with the new value. - for (const [key, value] of Object.entries(obj)) { - formattedConfigObject[key] = value + for (const key of Object.keys(configObject)) { + const option = formattedConfigObject[key] + const override = configObject[key] + + // Push their keys one-by-one into formattedConfigObject. Any duplicate + // keys with object values will be merged, otherwise the new value will + // override the existing value. + if (isObject(option) && isObject(override)) { + // @ts-expect-error Index signature for type 'string' is missing + formattedConfigObject[key] = mergeConfigs(option, override) + } else { + // Apply override + formattedConfigObject[key] = override + } } } @@ -251,6 +215,28 @@ export function validateConfig(schema, config) { return validationErrors } +/** + * Check for an array + * + * @internal + * @param {unknown} option - Option to check + * @returns {boolean} Whether the option is an array + */ +function isArray(option) { + return Array.isArray(option) +} + +/** + * Check for an object + * + * @internal + * @param {unknown} option - Option to check + * @returns {boolean} Whether the option is an object + */ +function isObject(option) { + return !!option && typeof option === 'object' && !isArray(option) +} + /** * Schema for component config * From 9bb03c3a928ef698d72785d1d2434c61de2bf0ee Mon Sep 17 00:00:00 2001 From: Colin Rotherham Date: Fri, 23 Feb 2024 13:50:32 +0000 Subject: [PATCH 04/12] Move `normaliseString()` + tests to separate files This prevents circular dependency issues --- .../src/govuk/common/normalise-dataset.mjs | 39 +------- .../common/normalise-dataset.unit.test.mjs | 95 +------------------ .../src/govuk/common/normalise-string.mjs | 38 ++++++++ .../common/normalise-string.unit.test.mjs | 94 ++++++++++++++++++ 4 files changed, 134 insertions(+), 132 deletions(-) create mode 100644 packages/govuk-frontend/src/govuk/common/normalise-string.mjs create mode 100644 packages/govuk-frontend/src/govuk/common/normalise-string.unit.test.mjs diff --git a/packages/govuk-frontend/src/govuk/common/normalise-dataset.mjs b/packages/govuk-frontend/src/govuk/common/normalise-dataset.mjs index b333208705..c8ab163dc4 100644 --- a/packages/govuk-frontend/src/govuk/common/normalise-dataset.mjs +++ b/packages/govuk-frontend/src/govuk/common/normalise-dataset.mjs @@ -1,41 +1,4 @@ -/** - * Normalise string - * - * 'If it looks like a duck, and it quacks like a duck…' 🦆 - * - * If the passed value looks like a boolean or a number, convert it to a boolean - * or number. - * - * Designed to be used to convert config passed via data attributes (which are - * always strings) into something sensible. - * - * @internal - * @param {string | undefined} value - The value to normalise - * @returns {string | boolean | number | undefined} Normalised data - */ -export function normaliseString(value) { - if (typeof value !== 'string') { - return value - } - - const trimmedValue = value.trim() - - if (trimmedValue === 'true') { - return true - } - - if (trimmedValue === 'false') { - return false - } - - // Empty / whitespace-only strings are considered finite so we need to check - // the length of the trimmed string as well - if (trimmedValue.length > 0 && isFinite(Number(trimmedValue))) { - return Number(trimmedValue) - } - - return value -} +import { normaliseString } from './normalise-string.mjs' /** * Normalise dataset diff --git a/packages/govuk-frontend/src/govuk/common/normalise-dataset.unit.test.mjs b/packages/govuk-frontend/src/govuk/common/normalise-dataset.unit.test.mjs index bad153717e..0e468a125c 100644 --- a/packages/govuk-frontend/src/govuk/common/normalise-dataset.unit.test.mjs +++ b/packages/govuk-frontend/src/govuk/common/normalise-dataset.unit.test.mjs @@ -1,97 +1,4 @@ -import { normaliseDataset, normaliseString } from './normalise-dataset.mjs' - -describe('normaliseString', () => { - it('does not normalise non-strings', () => { - // @ts-expect-error Parameter 'value' not a string - expect(normaliseString(100)).toEqual(100) - // @ts-expect-error Parameter 'value' not a string - expect(normaliseString(false)).toEqual(false) - // @ts-expect-error Parameter 'value' not a string - expect(normaliseString({})).toEqual({}) - // @ts-expect-error Parameter 'value' not a string - expect(normaliseString(NaN)).toEqual(NaN) - }) - - it('normalises the string "true" to boolean true', () => { - expect(normaliseString('true')).toEqual(true) - }) - - it('normalises the string "false" to boolean false', () => { - expect(normaliseString('false')).toEqual(false) - }) - - it('normalises the string " true " to boolean true', () => { - expect(normaliseString(' true ')).toEqual(true) - }) - - it('normalises the string " false " to boolean false', () => { - expect(normaliseString(' false ')).toEqual(false) - }) - - it('does not normalise non-lowercase booleans', () => { - expect(normaliseString('TRUE')).toEqual('TRUE') - expect(normaliseString('True')).toEqual('True') - expect(normaliseString('FALSE')).toEqual('FALSE') - expect(normaliseString('False')).toEqual('False') - }) - - it('does not normalise strings that contain booleans', () => { - expect(normaliseString('true!')).toEqual('true!') - }) - - it('normalises the string "0" to the number 0', () => { - expect(normaliseString('0')).toEqual(0) - }) - - it('normalises strings representing positive numbers to numbers', () => { - expect(normaliseString('1337')).toEqual(1337) - }) - - it('normalises strings representing negative numbers to numbers', () => { - expect(normaliseString('-1337')).toEqual(-1337) - }) - - it('converts strings representing decimal numbers to numbers', () => { - expect(normaliseString('0.5')).toEqual(0.5) - }) - - it('normalises strings representing decimal numbers with extra precision to numbers', () => { - expect(normaliseString('100.500')).toEqual(100.5) - }) - - it('normalises strings representing decimal numbers with no integer-part to numbers', () => { - expect(normaliseString('.5')).toEqual(0.5) - }) - - it('normalises strings representing numbers with whitespace to numbers', () => { - expect(normaliseString(' 1337 ')).toEqual(1337) - }) - - it('does not normalise the string "NaN"', () => { - expect(normaliseString('NaN')).toEqual('NaN') - }) - - it('does not normalise the string "Infinity"', () => { - expect(normaliseString('Infinity')).toEqual('Infinity') - }) - - it('normalises strings that represent very big positive numbers to numbers', () => { - const biggestNumber = Number.MAX_SAFE_INTEGER + 1 - expect(normaliseString(`${biggestNumber}`)).toEqual(biggestNumber) - }) - - it('does not normalise strings that contain numbers', () => { - expect(normaliseString('100%')).toEqual('100%') - }) - - it('does not normalise empty strings', () => { - expect(normaliseString('')).toEqual('') - }) - - it('does not normalise whitespace only strings', () => { - expect(normaliseString(' ')).toEqual(' ') - }) -}) +import { normaliseDataset } from './normalise-dataset.mjs' describe('normaliseDataset', () => { it('normalises the entire dataset', () => { diff --git a/packages/govuk-frontend/src/govuk/common/normalise-string.mjs b/packages/govuk-frontend/src/govuk/common/normalise-string.mjs new file mode 100644 index 0000000000..596a90e2ad --- /dev/null +++ b/packages/govuk-frontend/src/govuk/common/normalise-string.mjs @@ -0,0 +1,38 @@ +/** + * Normalise string + * + * 'If it looks like a duck, and it quacks like a duck…' 🦆 + * + * If the passed value looks like a boolean or a number, convert it to a boolean + * or number. + * + * Designed to be used to convert config passed via data attributes (which are + * always strings) into something sensible. + * + * @internal + * @param {string | undefined} value - The value to normalise + * @returns {string | boolean | number | undefined} Normalised data + */ +export function normaliseString(value) { + if (typeof value !== 'string') { + return value + } + + const trimmedValue = value.trim() + + if (trimmedValue === 'true') { + return true + } + + if (trimmedValue === 'false') { + return false + } + + // Empty / whitespace-only strings are considered finite so we need to check + // the length of the trimmed string as well + if (trimmedValue.length > 0 && isFinite(Number(trimmedValue))) { + return Number(trimmedValue) + } + + return value +} diff --git a/packages/govuk-frontend/src/govuk/common/normalise-string.unit.test.mjs b/packages/govuk-frontend/src/govuk/common/normalise-string.unit.test.mjs new file mode 100644 index 0000000000..abde009509 --- /dev/null +++ b/packages/govuk-frontend/src/govuk/common/normalise-string.unit.test.mjs @@ -0,0 +1,94 @@ +import { normaliseString } from './normalise-string.mjs' + +describe('normaliseString', () => { + it('does not normalise non-strings', () => { + // @ts-expect-error Parameter 'value' not a string + expect(normaliseString(100)).toEqual(100) + // @ts-expect-error Parameter 'value' not a string + expect(normaliseString(false)).toEqual(false) + // @ts-expect-error Parameter 'value' not a string + expect(normaliseString({})).toEqual({}) + // @ts-expect-error Parameter 'value' not a string + expect(normaliseString(NaN)).toEqual(NaN) + }) + + it('normalises the string "true" to boolean true', () => { + expect(normaliseString('true')).toEqual(true) + }) + + it('normalises the string "false" to boolean false', () => { + expect(normaliseString('false')).toEqual(false) + }) + + it('normalises the string " true " to boolean true', () => { + expect(normaliseString(' true ')).toEqual(true) + }) + + it('normalises the string " false " to boolean false', () => { + expect(normaliseString(' false ')).toEqual(false) + }) + + it('does not normalise non-lowercase booleans', () => { + expect(normaliseString('TRUE')).toEqual('TRUE') + expect(normaliseString('True')).toEqual('True') + expect(normaliseString('FALSE')).toEqual('FALSE') + expect(normaliseString('False')).toEqual('False') + }) + + it('does not normalise strings that contain booleans', () => { + expect(normaliseString('true!')).toEqual('true!') + }) + + it('normalises the string "0" to the number 0', () => { + expect(normaliseString('0')).toEqual(0) + }) + + it('normalises strings representing positive numbers to numbers', () => { + expect(normaliseString('1337')).toEqual(1337) + }) + + it('normalises strings representing negative numbers to numbers', () => { + expect(normaliseString('-1337')).toEqual(-1337) + }) + + it('converts strings representing decimal numbers to numbers', () => { + expect(normaliseString('0.5')).toEqual(0.5) + }) + + it('normalises strings representing decimal numbers with extra precision to numbers', () => { + expect(normaliseString('100.500')).toEqual(100.5) + }) + + it('normalises strings representing decimal numbers with no integer-part to numbers', () => { + expect(normaliseString('.5')).toEqual(0.5) + }) + + it('normalises strings representing numbers with whitespace to numbers', () => { + expect(normaliseString(' 1337 ')).toEqual(1337) + }) + + it('does not normalise the string "NaN"', () => { + expect(normaliseString('NaN')).toEqual('NaN') + }) + + it('does not normalise the string "Infinity"', () => { + expect(normaliseString('Infinity')).toEqual('Infinity') + }) + + it('normalises strings that represent very big positive numbers to numbers', () => { + const biggestNumber = Number.MAX_SAFE_INTEGER + 1 + expect(normaliseString(`${biggestNumber}`)).toEqual(biggestNumber) + }) + + it('does not normalise strings that contain numbers', () => { + expect(normaliseString('100%')).toEqual('100%') + }) + + it('does not normalise empty strings', () => { + expect(normaliseString('')).toEqual('') + }) + + it('does not normalise whitespace only strings', () => { + expect(normaliseString(' ')).toEqual(' ') + }) +}) From ff1729e5bbb0d92c1833bb982ea970d6df2b40b1 Mon Sep 17 00:00:00 2001 From: Colin Rotherham Date: Fri, 8 Mar 2024 16:43:53 +0000 Subject: [PATCH 05/12] Filter `normaliseDataset()` to discard unknown data attributes MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Since we only pass in `DOMStringMap` values, this removes all tests for `normaliseDataset()` that don’t pass in strings We also prevent unnecessary data attributes (e.g. `data-module="govuk-accordion"’) from being merged into the config See: https://github.com/alphagov/govuk-frontend/issues/4230 --- .../govuk-frontend/src/govuk/common/index.mjs | 2 +- .../src/govuk/common/normalise-dataset.mjs | 26 ++++++++++---- .../common/normalise-dataset.unit.test.mjs | 34 +++++++++++++++---- .../src/govuk/common/normalise-string.mjs | 8 ++--- .../common/normalise-string.unit.test.mjs | 11 ------ .../govuk/components/accordion/accordion.mjs | 2 +- .../src/govuk/components/button/button.mjs | 2 +- .../character-count/character-count.mjs | 2 +- .../error-summary/error-summary.mjs | 2 +- .../exit-this-page/exit-this-page.mjs | 2 +- .../notification-banner.mjs | 2 +- 11 files changed, 56 insertions(+), 37 deletions(-) diff --git a/packages/govuk-frontend/src/govuk/common/index.mjs b/packages/govuk-frontend/src/govuk/common/index.mjs index cc72b8e665..9e7e1ea8b4 100644 --- a/packages/govuk-frontend/src/govuk/common/index.mjs +++ b/packages/govuk-frontend/src/govuk/common/index.mjs @@ -241,7 +241,7 @@ function isObject(option) { * Schema for component config * * @typedef {object} Schema - * @property {{ [field: string]: SchemaProperty }} properties - Schema properties + * @property {{ [field: string]: SchemaProperty | undefined }} properties - Schema properties * @property {SchemaCondition[]} [anyOf] - List of schema conditions */ diff --git a/packages/govuk-frontend/src/govuk/common/normalise-dataset.mjs b/packages/govuk-frontend/src/govuk/common/normalise-dataset.mjs index c8ab163dc4..9a2af90694 100644 --- a/packages/govuk-frontend/src/govuk/common/normalise-dataset.mjs +++ b/packages/govuk-frontend/src/govuk/common/normalise-dataset.mjs @@ -3,19 +3,33 @@ import { normaliseString } from './normalise-string.mjs' /** * Normalise dataset * - * Loop over an object and normalise each value using normaliseData function + * Loop over an object and normalise each value using {@link normaliseString} * * @internal + * @param {{ schema: Schema }} Component - Component class * @param {DOMStringMap} dataset - HTML element dataset * @returns {{ [key: string]: string | boolean | number | undefined }} Normalised dataset */ -export function normaliseDataset(dataset) { - /** @type {ReturnType} */ - const out = {} +export function normaliseDataset(Component, dataset) { + const out = /** @type {ReturnType} */ ({}) - for (const [key, value] of Object.entries(dataset)) { - out[key] = normaliseString(value) + // Normalise top-level dataset ('data-*') values + for (const [field, property] of Object.entries(Component.schema.properties)) { + if (field in dataset) { + const value = normaliseString(dataset[field]) + + // But skip if type does not match schema + // eslint-disable-next-line valid-typeof + if (property?.type === typeof value) { + out[field] = value + } + } } return out } + +/** + * @internal + * @typedef {import('./index.mjs').Schema} Schema + */ diff --git a/packages/govuk-frontend/src/govuk/common/normalise-dataset.unit.test.mjs b/packages/govuk-frontend/src/govuk/common/normalise-dataset.unit.test.mjs index 0e468a125c..02b4cdfecb 100644 --- a/packages/govuk-frontend/src/govuk/common/normalise-dataset.unit.test.mjs +++ b/packages/govuk-frontend/src/govuk/common/normalise-dataset.unit.test.mjs @@ -3,13 +3,29 @@ import { normaliseDataset } from './normalise-dataset.mjs' describe('normaliseDataset', () => { it('normalises the entire dataset', () => { expect( - normaliseDataset({ - aNumber: '1000', - aDecimalNumber: '100.50', - aBoolean: 'true', - aString: 'Hello!', - anOptionalString: '' - }) + normaliseDataset( + class Component { + /** + * @satisfies {Schema} + */ + static schema = { + properties: { + aNumber: { type: 'number' }, + aDecimalNumber: { type: 'number' }, + aBoolean: { type: 'boolean' }, + aString: { type: 'string' }, + anOptionalString: { type: 'string' } + } + } + }, + { + aNumber: '1000', + aDecimalNumber: '100.50', + aBoolean: 'true', + aString: 'Hello!', + anOptionalString: '' + } + ) ).toEqual({ aNumber: 1000, aDecimalNumber: 100.5, @@ -19,3 +35,7 @@ describe('normaliseDataset', () => { }) }) }) + +/** + * @typedef {import('./index.mjs').Schema} Schema + */ diff --git a/packages/govuk-frontend/src/govuk/common/normalise-string.mjs b/packages/govuk-frontend/src/govuk/common/normalise-string.mjs index 596a90e2ad..e23764795b 100644 --- a/packages/govuk-frontend/src/govuk/common/normalise-string.mjs +++ b/packages/govuk-frontend/src/govuk/common/normalise-string.mjs @@ -10,15 +10,11 @@ * always strings) into something sensible. * * @internal - * @param {string | undefined} value - The value to normalise + * @param {DOMStringMap[string]} value - The value to normalise * @returns {string | boolean | number | undefined} Normalised data */ export function normaliseString(value) { - if (typeof value !== 'string') { - return value - } - - const trimmedValue = value.trim() + const trimmedValue = value ? value.trim() : '' if (trimmedValue === 'true') { return true diff --git a/packages/govuk-frontend/src/govuk/common/normalise-string.unit.test.mjs b/packages/govuk-frontend/src/govuk/common/normalise-string.unit.test.mjs index abde009509..f3e81b2ae6 100644 --- a/packages/govuk-frontend/src/govuk/common/normalise-string.unit.test.mjs +++ b/packages/govuk-frontend/src/govuk/common/normalise-string.unit.test.mjs @@ -1,17 +1,6 @@ import { normaliseString } from './normalise-string.mjs' describe('normaliseString', () => { - it('does not normalise non-strings', () => { - // @ts-expect-error Parameter 'value' not a string - expect(normaliseString(100)).toEqual(100) - // @ts-expect-error Parameter 'value' not a string - expect(normaliseString(false)).toEqual(false) - // @ts-expect-error Parameter 'value' not a string - expect(normaliseString({})).toEqual({}) - // @ts-expect-error Parameter 'value' not a string - expect(normaliseString(NaN)).toEqual(NaN) - }) - it('normalises the string "true" to boolean true', () => { expect(normaliseString('true')).toEqual(true) }) diff --git a/packages/govuk-frontend/src/govuk/components/accordion/accordion.mjs b/packages/govuk-frontend/src/govuk/components/accordion/accordion.mjs index d7b978dd0b..d3c157d87b 100644 --- a/packages/govuk-frontend/src/govuk/components/accordion/accordion.mjs +++ b/packages/govuk-frontend/src/govuk/components/accordion/accordion.mjs @@ -132,7 +132,7 @@ export class Accordion extends GOVUKFrontendComponent { this.config = mergeConfigs( Accordion.defaults, config, - normaliseDataset($module.dataset) + normaliseDataset(Accordion, $module.dataset) ) this.i18n = new I18n(extractConfigByNamespace(this.config, 'i18n')) diff --git a/packages/govuk-frontend/src/govuk/components/button/button.mjs b/packages/govuk-frontend/src/govuk/components/button/button.mjs index 7a199631b1..7dde40b11e 100644 --- a/packages/govuk-frontend/src/govuk/components/button/button.mjs +++ b/packages/govuk-frontend/src/govuk/components/button/button.mjs @@ -46,7 +46,7 @@ export class Button extends GOVUKFrontendComponent { this.config = mergeConfigs( Button.defaults, config, - normaliseDataset($module.dataset) + normaliseDataset(Button, $module.dataset) ) this.$module.addEventListener('keydown', (event) => diff --git a/packages/govuk-frontend/src/govuk/components/character-count/character-count.mjs b/packages/govuk-frontend/src/govuk/components/character-count/character-count.mjs index d6554caf3c..31795267f8 100644 --- a/packages/govuk-frontend/src/govuk/components/character-count/character-count.mjs +++ b/packages/govuk-frontend/src/govuk/components/character-count/character-count.mjs @@ -92,7 +92,7 @@ export class CharacterCount extends GOVUKFrontendComponent { } // Read config set using dataset ('data-' values) - const datasetConfig = normaliseDataset($module.dataset) + const datasetConfig = normaliseDataset(CharacterCount, $module.dataset) // To ensure data-attributes take complete precedence, even if they change // the type of count, we need to reset the `maxlength` and `maxwords` from diff --git a/packages/govuk-frontend/src/govuk/components/error-summary/error-summary.mjs b/packages/govuk-frontend/src/govuk/components/error-summary/error-summary.mjs index 685e8244dc..51a8b4144f 100644 --- a/packages/govuk-frontend/src/govuk/components/error-summary/error-summary.mjs +++ b/packages/govuk-frontend/src/govuk/components/error-summary/error-summary.mjs @@ -45,7 +45,7 @@ export class ErrorSummary extends GOVUKFrontendComponent { this.config = mergeConfigs( ErrorSummary.defaults, config, - normaliseDataset($module.dataset) + normaliseDataset(ErrorSummary, $module.dataset) ) /** diff --git a/packages/govuk-frontend/src/govuk/components/exit-this-page/exit-this-page.mjs b/packages/govuk-frontend/src/govuk/components/exit-this-page/exit-this-page.mjs index 6170250a70..afc8f58ad8 100644 --- a/packages/govuk-frontend/src/govuk/components/exit-this-page/exit-this-page.mjs +++ b/packages/govuk-frontend/src/govuk/components/exit-this-page/exit-this-page.mjs @@ -102,7 +102,7 @@ export class ExitThisPage extends GOVUKFrontendComponent { this.config = mergeConfigs( ExitThisPage.defaults, config, - normaliseDataset($module.dataset) + normaliseDataset(ExitThisPage, $module.dataset) ) this.i18n = new I18n(extractConfigByNamespace(this.config, 'i18n')) diff --git a/packages/govuk-frontend/src/govuk/components/notification-banner/notification-banner.mjs b/packages/govuk-frontend/src/govuk/components/notification-banner/notification-banner.mjs index facee870dd..089fd0438b 100644 --- a/packages/govuk-frontend/src/govuk/components/notification-banner/notification-banner.mjs +++ b/packages/govuk-frontend/src/govuk/components/notification-banner/notification-banner.mjs @@ -38,7 +38,7 @@ export class NotificationBanner extends GOVUKFrontendComponent { this.config = mergeConfigs( NotificationBanner.defaults, config, - normaliseDataset($module.dataset) + normaliseDataset(NotificationBanner, $module.dataset) ) /** From a748e7465d2750620f17e057431d2e28b16d2c49 Mon Sep 17 00:00:00 2001 From: Colin Rotherham Date: Thu, 7 Mar 2024 21:10:48 +0000 Subject: [PATCH 06/12] =?UTF-8?q?Update=20`normaliseDataset()`=20to=20outp?= =?UTF-8?q?ut=20=E2=80=9Cunflattened=E2=80=9D=20nested=20configs?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit We can reduce the code we use by expanding dot-separated data-attributes at the point we read them This lets us remove `flattenConfigs()` since we can use nested configs everywhere --- .../src/govuk/common/index.jsdom.test.mjs | 204 ++++++++++++++++-- .../govuk-frontend/src/govuk/common/index.mjs | 74 +++++-- .../src/govuk/common/normalise-dataset.mjs | 15 +- .../common/normalise-dataset.unit.test.mjs | 15 +- 4 files changed, 267 insertions(+), 41 deletions(-) diff --git a/packages/govuk-frontend/src/govuk/common/index.jsdom.test.mjs b/packages/govuk-frontend/src/govuk/common/index.jsdom.test.mjs index bde35f8b5c..1525bb0203 100644 --- a/packages/govuk-frontend/src/govuk/common/index.jsdom.test.mjs +++ b/packages/govuk-frontend/src/govuk/common/index.jsdom.test.mjs @@ -110,26 +110,198 @@ describe('Common JS utilities', () => { }) describe('extractConfigByNamespace', () => { - const flattenedConfig = { - a: 'aardvark', - 'b.a': 'bat', - 'b.e': 'bear', - 'b.o': 'boar', - 'c.a': 'camel', - 'c.o': 'cow', - d: 'dog', - e: 'elephant' + class Component { + /** + * @satisfies {Schema} + */ + static schema = { + properties: { + a: { type: 'string' }, + b: { type: 'object' }, + c: { type: 'object' }, + d: { type: 'string' }, + e: { type: 'string' }, + f: { type: 'object' } + } + } } - it('can extract single key-value pairs', () => { - const result = extractConfigByNamespace(flattenedConfig, 'a') - expect(result).toEqual({ a: 'aardvark' }) + /** @type {HTMLElement} */ + let $element + + beforeEach(() => { + document.body.outerHTML = outdent` +
+
+ ` + + $element = document.getElementById('app-example') + }) + + it('defaults to empty config for known namespaces only', () => { + const { dataset } = $element + + const nonObject1 = extractConfigByNamespace(Component, dataset, 'a') + const nonObject2 = extractConfigByNamespace(Component, dataset, 'd') + const nonObject3 = extractConfigByNamespace(Component, dataset, 'e') + + const namespaceKnown = extractConfigByNamespace(Component, dataset, 'f') + const namespaceUnknown = extractConfigByNamespace( + Component, + dataset, + 'unknown' + ) + + // With known namespace but non-object type, default to no config + expect(nonObject1).toEqual(undefined) + expect(nonObject2).toEqual(undefined) + expect(nonObject3).toEqual(undefined) + + // With known namespace, default to empty config + expect(namespaceKnown).toEqual({}) + + // With unknown namespace, default to no config + expect(namespaceUnknown).toEqual(undefined) }) - it('can extract multiple key-value pairs', () => { - const result = extractConfigByNamespace(flattenedConfig, 'b') + it('can extract config from key-value pairs', () => { + const result = extractConfigByNamespace(Component, $element.dataset, 'b') expect(result).toEqual({ a: 'bat', e: 'bear', o: 'boar' }) }) + + it('can extract config from key-value pairs (with invalid namespace, first)', () => { + document.body.outerHTML = outdent` +
+
+ ` + + const { dataset } = document.getElementById('app-example2') + const result = extractConfigByNamespace( + class Component { + /** + * @satisfies {Schema} + */ + static schema = { + properties: { + i18n: { type: 'object' } + } + } + }, + dataset, + 'i18n' + ) + + expect(result).toEqual({ key1: 'One', key2: 'Two', key3: 'Three' }) + }) + + it('can extract config from key-value pairs (with invalid namespace, last)', () => { + document.body.outerHTML = outdent` +
+
+ ` + + const { dataset } = document.getElementById('app-example2') + const result = extractConfigByNamespace( + class Component { + /** + * @satisfies {Schema} + */ + static schema = { + properties: { + i18n: { type: 'object' } + } + } + }, + dataset, + 'i18n' + ) + + expect(result).toEqual({ key1: 'One', key2: 'Two', key3: 'Three' }) + }) + + it('can handle multiple levels of nesting', () => { + document.body.outerHTML = outdent` +
+
+ ` + + const { dataset } = document.getElementById('app-example2') + const result = extractConfigByNamespace( + class Component { + /** + * @satisfies {Schema} + */ + static schema = { + properties: { + i18n: { type: 'object' } + } + } + }, + dataset, + 'i18n' + ) + + expect(result).toEqual({ + key1: 'This, That', + key2: { + one: 'The', + other: 'Other' + } + }) + }) + + it('can handle multiple levels of nesting (prioritises the last parameter provided)', () => { + document.body.outerHTML = outdent` +
+
+ ` + + const { dataset } = document.getElementById('app-example2') + const result = extractConfigByNamespace( + class Component { + /** + * @satisfies {Schema} + */ + static schema = { + properties: { + i18n: { type: 'object' } + } + } + }, + dataset, + 'i18n' + ) + + expect(result).toEqual({ + key1: 'This, That', + key2: 'The Other' + }) + }) }) describe('isSupported', () => { @@ -240,3 +412,7 @@ describe('Common JS utilities', () => { }) }) }) + +/** + * @typedef {import('./index.mjs').Schema} Schema + */ diff --git a/packages/govuk-frontend/src/govuk/common/index.mjs b/packages/govuk-frontend/src/govuk/common/index.mjs index 9e7e1ea8b4..dcd7982f59 100644 --- a/packages/govuk-frontend/src/govuk/common/index.mjs +++ b/packages/govuk-frontend/src/govuk/common/index.mjs @@ -1,3 +1,5 @@ +import { normaliseString } from './normalise-string.mjs' + /** * Common helpers which do not require polyfill. * @@ -44,39 +46,56 @@ export function mergeConfigs(...configObjects) { } /** - * Extracts keys starting with a particular namespace from a flattened config - * object, removing the namespace in the process. + * Extracts keys starting with a particular namespace from dataset ('data-*') + * object, removing the namespace in the process, normalising all values * * @internal - * @param {{ [key: string]: unknown }} configObject - The object to extract key-value pairs from. - * @param {string} namespace - The namespace to filter keys with. - * @returns {{ [key: string]: unknown }} Flattened object with dot-separated key namespace removed + * @param {{ schema: Schema }} Component - Component class + * @param {DOMStringMap} dataset - The object to extract key-value pairs from + * @param {string} namespace - The namespace to filter keys with + * @returns {ObjectNested | undefined} Nested object with dot-separated key namespace removed */ -export function extractConfigByNamespace(configObject, namespace) { - /** @type {{ [key: string]: unknown }} */ - const newObject = {} +export function extractConfigByNamespace(Component, dataset, namespace) { + const property = Component.schema.properties[namespace] + + // Only extract configs for object schema properties + if (property?.type !== 'object') { + return + } + + // Add default empty config + const newObject = { + [namespace]: /** @type {ObjectNested} */ ({}) + } + + for (const [key, value] of Object.entries(dataset)) { + /** @type {ObjectNested | ObjectNested[NestedKey]} */ + let current = newObject - for (const [key, value] of Object.entries(configObject)) { // Split the key into parts, using . as our namespace separator const keyParts = key.split('.') - // Check if the first namespace matches the configured namespace - if (keyParts[0] === namespace) { - // Remove the first item (the namespace) from the parts array, - // but only if there is more than one part (we don't want blank keys!) - if (keyParts.length > 1) { - keyParts.shift() + /** + * Create new level per part + * + * e.g. 'i18n.textareaDescription.other' becomes + * `{ i18n: { textareaDescription: { other } } }` + */ + for (const [index, name] of keyParts.entries()) { + if (typeof current === 'object') { + // Drop down to what we assume is a nested object + // but check for object type on the next loop + if (index < keyParts.length - 1) { + current = current[name] = current[name] ?? {} + } else if (name !== namespace) { + // Add normalised value to overwrite default + current[name] = normaliseString(value) + } } - - // Join the remaining parts back together - const newKey = keyParts.join('.') - - // Add them to our new object - newObject[newKey] = value } } - return newObject + return newObject[namespace] } /** @@ -185,6 +204,11 @@ export function isSupported($scope = document.body) { /** * Validate component config by schema * + * Follows limited examples in JSON schema for wider support in future + * + * {@link https://ajv.js.org/json-schema.html#compound-keywords} + * {@link https://ajv.js.org/packages/ajv-errors.html#single-message} + * * @internal * @param {Schema} schema - Config schema * @param {{ [key: string]: unknown }} config - Component config @@ -259,3 +283,9 @@ function isObject(option) { * @property {string[]} required - List of required config fields * @property {string} errorMessage - Error message when required config fields not provided */ + +/** + * @internal + * @typedef {keyof ObjectNested} NestedKey + * @typedef {{ [key: string]: string | boolean | number | ObjectNested | undefined }} ObjectNested + */ diff --git a/packages/govuk-frontend/src/govuk/common/normalise-dataset.mjs b/packages/govuk-frontend/src/govuk/common/normalise-dataset.mjs index 9a2af90694..517743c815 100644 --- a/packages/govuk-frontend/src/govuk/common/normalise-dataset.mjs +++ b/packages/govuk-frontend/src/govuk/common/normalise-dataset.mjs @@ -1,14 +1,16 @@ +import { extractConfigByNamespace } from './index.mjs' import { normaliseString } from './normalise-string.mjs' /** * Normalise dataset * - * Loop over an object and normalise each value using {@link normaliseString} + * Loop over an object and normalise each value using {@link normaliseString}, + * optionally expanding nested `i18n.field` * * @internal * @param {{ schema: Schema }} Component - Component class * @param {DOMStringMap} dataset - HTML element dataset - * @returns {{ [key: string]: string | boolean | number | undefined }} Normalised dataset + * @returns {ObjectNested} Normalised dataset */ export function normaliseDataset(Component, dataset) { const out = /** @type {ReturnType} */ ({}) @@ -24,6 +26,14 @@ export function normaliseDataset(Component, dataset) { out[field] = value } } + + /** + * Extract and normalise nested object values automatically using + * {@link normaliseString} but only schema object types are allowed + */ + if (property?.type === 'object') { + out[field] = extractConfigByNamespace(Component, dataset, field) + } } return out @@ -31,5 +41,6 @@ export function normaliseDataset(Component, dataset) { /** * @internal + * @typedef {import('./index.mjs').ObjectNested} ObjectNested * @typedef {import('./index.mjs').Schema} Schema */ diff --git a/packages/govuk-frontend/src/govuk/common/normalise-dataset.unit.test.mjs b/packages/govuk-frontend/src/govuk/common/normalise-dataset.unit.test.mjs index 02b4cdfecb..809fdc00d7 100644 --- a/packages/govuk-frontend/src/govuk/common/normalise-dataset.unit.test.mjs +++ b/packages/govuk-frontend/src/govuk/common/normalise-dataset.unit.test.mjs @@ -14,7 +14,8 @@ describe('normaliseDataset', () => { aDecimalNumber: { type: 'number' }, aBoolean: { type: 'boolean' }, aString: { type: 'string' }, - anOptionalString: { type: 'string' } + anOptionalString: { type: 'string' }, + anObject: { type: 'object' } } } }, @@ -23,7 +24,10 @@ describe('normaliseDataset', () => { aDecimalNumber: '100.50', aBoolean: 'true', aString: 'Hello!', - anOptionalString: '' + anOptionalString: '', + 'anObject.one': '111', + 'anObject.two': '222', + 'anObject.three': '333' } ) ).toEqual({ @@ -31,7 +35,12 @@ describe('normaliseDataset', () => { aDecimalNumber: 100.5, aBoolean: true, aString: 'Hello!', - anOptionalString: '' + anOptionalString: '', + anObject: { + one: 111, + two: 222, + three: 333 + } }) }) }) From 7458577fbe2d83f519230ab830578920a58ccf0c Mon Sep 17 00:00:00 2001 From: Colin Rotherham Date: Fri, 8 Mar 2024 14:29:45 +0000 Subject: [PATCH 07/12] Update `normaliseString()` to use schema property types Do you want a `data-example="2024"` to stay as a string? Data attributes now understand component config schema types Extra checks added to guard against objects, arrays and non-finite numbers (NaN, Infinity) --- .../src/govuk/common/normalise-dataset.mjs | 10 +---- .../common/normalise-dataset.unit.test.mjs | 6 +++ .../src/govuk/common/normalise-string.mjs | 42 ++++++++++++++----- 3 files changed, 39 insertions(+), 19 deletions(-) diff --git a/packages/govuk-frontend/src/govuk/common/normalise-dataset.mjs b/packages/govuk-frontend/src/govuk/common/normalise-dataset.mjs index 517743c815..d912e8b3cf 100644 --- a/packages/govuk-frontend/src/govuk/common/normalise-dataset.mjs +++ b/packages/govuk-frontend/src/govuk/common/normalise-dataset.mjs @@ -15,16 +15,10 @@ import { normaliseString } from './normalise-string.mjs' export function normaliseDataset(Component, dataset) { const out = /** @type {ReturnType} */ ({}) - // Normalise top-level dataset ('data-*') values + // Normalise top-level dataset ('data-*') values using schema types for (const [field, property] of Object.entries(Component.schema.properties)) { if (field in dataset) { - const value = normaliseString(dataset[field]) - - // But skip if type does not match schema - // eslint-disable-next-line valid-typeof - if (property?.type === typeof value) { - out[field] = value - } + out[field] = normaliseString(dataset[field], property) } /** diff --git a/packages/govuk-frontend/src/govuk/common/normalise-dataset.unit.test.mjs b/packages/govuk-frontend/src/govuk/common/normalise-dataset.unit.test.mjs index 809fdc00d7..a5f1bd4b48 100644 --- a/packages/govuk-frontend/src/govuk/common/normalise-dataset.unit.test.mjs +++ b/packages/govuk-frontend/src/govuk/common/normalise-dataset.unit.test.mjs @@ -14,6 +14,8 @@ describe('normaliseDataset', () => { aDecimalNumber: { type: 'number' }, aBoolean: { type: 'boolean' }, aString: { type: 'string' }, + aStringBoolean: { type: 'string' }, + aStringNumber: { type: 'string' }, anOptionalString: { type: 'string' }, anObject: { type: 'object' } } @@ -24,6 +26,8 @@ describe('normaliseDataset', () => { aDecimalNumber: '100.50', aBoolean: 'true', aString: 'Hello!', + aStringBoolean: 'false', + aStringNumber: '2024', anOptionalString: '', 'anObject.one': '111', 'anObject.two': '222', @@ -35,6 +39,8 @@ describe('normaliseDataset', () => { aDecimalNumber: 100.5, aBoolean: true, aString: 'Hello!', + aStringBoolean: 'false', + aStringNumber: '2024', anOptionalString: '', anObject: { one: 111, diff --git a/packages/govuk-frontend/src/govuk/common/normalise-string.mjs b/packages/govuk-frontend/src/govuk/common/normalise-string.mjs index e23764795b..b5940dda33 100644 --- a/packages/govuk-frontend/src/govuk/common/normalise-string.mjs +++ b/packages/govuk-frontend/src/govuk/common/normalise-string.mjs @@ -11,24 +11,44 @@ * * @internal * @param {DOMStringMap[string]} value - The value to normalise + * @param {SchemaProperty} [property] - Component schema property * @returns {string | boolean | number | undefined} Normalised data */ -export function normaliseString(value) { +export function normaliseString(value, property) { const trimmedValue = value ? value.trim() : '' - if (trimmedValue === 'true') { - return true - } + let output + let outputType = property?.type + + // No schema type set? Determine automatically + if (!outputType) { + if (['true', 'false'].includes(trimmedValue)) { + outputType = 'boolean' + } - if (trimmedValue === 'false') { - return false + // Empty / whitespace-only strings are considered finite so we need to check + // the length of the trimmed string as well + if (trimmedValue.length > 0 && isFinite(Number(trimmedValue))) { + outputType = 'number' + } } - // Empty / whitespace-only strings are considered finite so we need to check - // the length of the trimmed string as well - if (trimmedValue.length > 0 && isFinite(Number(trimmedValue))) { - return Number(trimmedValue) + switch (outputType) { + case 'boolean': + output = trimmedValue === 'true' + break + + case 'number': + output = Number(trimmedValue) + break + + default: + output = value } - return value + return output } + +/** + * @typedef {import('./index.mjs').SchemaProperty} SchemaProperty + */ From 5f7d55ef611944014a10cac980992e032240b883 Mon Sep 17 00:00:00 2001 From: Colin Rotherham Date: Mon, 26 Feb 2024 17:14:57 +0000 Subject: [PATCH 08/12] =?UTF-8?q?Update=20`I18n`=20to=20use=20=E2=80=9Cunf?= =?UTF-8?q?lattened=E2=80=9D=20nested=20configs?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit --- .../src/views/examples/translated/index.njk | 8 +-- .../govuk/components/accordion/accordion.mjs | 4 +- .../character-count/character-count.mjs | 8 +-- .../exit-this-page/exit-this-page.mjs | 4 +- .../src/govuk/i18n.jsdom.test.mjs | 40 +++++++++----- packages/govuk-frontend/src/govuk/i18n.mjs | 53 +++++++++++-------- 6 files changed, 68 insertions(+), 49 deletions(-) diff --git a/packages/govuk-frontend-review/src/views/examples/translated/index.njk b/packages/govuk-frontend-review/src/views/examples/translated/index.njk index b8fc294af3..3ba09e3c52 100644 --- a/packages/govuk-frontend-review/src/views/examples/translated/index.njk +++ b/packages/govuk-frontend-review/src/views/examples/translated/index.njk @@ -954,11 +954,11 @@ i18n: { showAllSections: 'Dangos adrannau', hideAllSections: 'Cuddio adrannau', + showSection: 'Dangos', + showSectionAriaLabel: 'Dangos adran', + hideSection: 'Cuddio', + hideSectionAriaLabel: 'Cuddio adran' }, - 'i18n.showSection': 'Dangos', - 'i18n.showSectionAriaLabel': 'Dangos adran', - 'i18n.hideSection': 'Cuddio', - 'i18n.hideSectionAriaLabel': 'Cuddio adran' } }) diff --git a/packages/govuk-frontend/src/govuk/components/accordion/accordion.mjs b/packages/govuk-frontend/src/govuk/components/accordion/accordion.mjs index d3c157d87b..5ca6bb5741 100644 --- a/packages/govuk-frontend/src/govuk/components/accordion/accordion.mjs +++ b/packages/govuk-frontend/src/govuk/components/accordion/accordion.mjs @@ -1,4 +1,4 @@ -import { mergeConfigs, extractConfigByNamespace } from '../../common/index.mjs' +import { mergeConfigs } from '../../common/index.mjs' import { normaliseDataset } from '../../common/normalise-dataset.mjs' import { ElementError } from '../../errors/index.mjs' import { GOVUKFrontendComponent } from '../../govuk-frontend-component.mjs' @@ -135,7 +135,7 @@ export class Accordion extends GOVUKFrontendComponent { normaliseDataset(Accordion, $module.dataset) ) - this.i18n = new I18n(extractConfigByNamespace(this.config, 'i18n')) + this.i18n = new I18n(this.config.i18n) const $sections = this.$module.querySelectorAll(`.${this.sectionClass}`) if (!$sections.length) { diff --git a/packages/govuk-frontend/src/govuk/components/character-count/character-count.mjs b/packages/govuk-frontend/src/govuk/components/character-count/character-count.mjs index 31795267f8..6c434b4dcf 100644 --- a/packages/govuk-frontend/src/govuk/components/character-count/character-count.mjs +++ b/packages/govuk-frontend/src/govuk/components/character-count/character-count.mjs @@ -1,9 +1,5 @@ import { closestAttributeValue } from '../../common/closest-attribute-value.mjs' -import { - extractConfigByNamespace, - mergeConfigs, - validateConfig -} from '../../common/index.mjs' +import { mergeConfigs, validateConfig } from '../../common/index.mjs' import { normaliseDataset } from '../../common/normalise-dataset.mjs' import { ConfigError, ElementError } from '../../errors/index.mjs' import { GOVUKFrontendComponent } from '../../govuk-frontend-component.mjs' @@ -122,7 +118,7 @@ export class CharacterCount extends GOVUKFrontendComponent { throw new ConfigError(`Character count: ${errors[0]}`) } - this.i18n = new I18n(extractConfigByNamespace(this.config, 'i18n'), { + this.i18n = new I18n(this.config.i18n, { // Read the fallback if necessary rather than have it set in the defaults locale: closestAttributeValue($module, 'lang') }) diff --git a/packages/govuk-frontend/src/govuk/components/exit-this-page/exit-this-page.mjs b/packages/govuk-frontend/src/govuk/components/exit-this-page/exit-this-page.mjs index afc8f58ad8..a9983bf60a 100644 --- a/packages/govuk-frontend/src/govuk/components/exit-this-page/exit-this-page.mjs +++ b/packages/govuk-frontend/src/govuk/components/exit-this-page/exit-this-page.mjs @@ -1,4 +1,4 @@ -import { mergeConfigs, extractConfigByNamespace } from '../../common/index.mjs' +import { mergeConfigs } from '../../common/index.mjs' import { normaliseDataset } from '../../common/normalise-dataset.mjs' import { ElementError } from '../../errors/index.mjs' import { GOVUKFrontendComponent } from '../../govuk-frontend-component.mjs' @@ -105,7 +105,7 @@ export class ExitThisPage extends GOVUKFrontendComponent { normaliseDataset(ExitThisPage, $module.dataset) ) - this.i18n = new I18n(extractConfigByNamespace(this.config, 'i18n')) + this.i18n = new I18n(this.config.i18n) this.$module = $module this.$button = $button diff --git a/packages/govuk-frontend/src/govuk/i18n.jsdom.test.mjs b/packages/govuk-frontend/src/govuk/i18n.jsdom.test.mjs index 8c68a952f7..5b8a26a991 100644 --- a/packages/govuk-frontend/src/govuk/i18n.jsdom.test.mjs +++ b/packages/govuk-frontend/src/govuk/i18n.jsdom.test.mjs @@ -208,8 +208,10 @@ describe('I18n', () => { it('interpolates the count variable into the correct plural form', () => { const i18n = new I18n( { - 'test.one': '%{count} test', - 'test.other': '%{count} tests' + test: { + one: '%{count} test', + other: '%{count} tests' + } }, { locale: 'en' @@ -245,8 +247,10 @@ describe('I18n', () => { const i18n = new I18n( { - 'test.one': 'test', - 'test.other': 'test' + test: { + one: 'test', + other: 'test' + } }, { locale: 'en' @@ -260,8 +264,10 @@ describe('I18n', () => { it('falls back to internal fallback rules', () => { const i18n = new I18n( { - 'test.one': 'test', - 'test.other': 'test' + test: { + one: 'test', + other: 'test' + } }, { locale: 'en' @@ -284,8 +290,10 @@ describe('I18n', () => { it('returns the preferred plural form for the locale if a translation exists', () => { const i18n = new I18n( { - 'test.one': 'test', - 'test.other': 'test' + test: { + one: 'test', + other: 'test' + } }, { locale: 'en' @@ -304,7 +312,9 @@ describe('I18n', () => { ({ count }) => { const i18n = new I18n( { - 'test.other': 'test' + test: { + other: 'test' + } }, { locale: 'cy' @@ -318,7 +328,9 @@ describe('I18n', () => { it('logs a console warning when falling back to `other`', () => { const i18n = new I18n( { - 'test.other': 'test' + test: { + other: 'test' + } }, { locale: 'en' @@ -348,7 +360,9 @@ describe('I18n', () => { it('throws an error if a plural form is not provided and neither is `other`', () => { const i18n = new I18n( { - 'test.one': 'test' + test: { + one: 'test' + } }, { locale: 'en' @@ -363,7 +377,9 @@ describe('I18n', () => { it('returns `other` for non-numbers', () => { const i18n = new I18n( { - 'test.other': 'test' + test: { + other: 'test' + } }, { locale: 'en' diff --git a/packages/govuk-frontend/src/govuk/i18n.mjs b/packages/govuk-frontend/src/govuk/i18n.mjs index dcac3063c6..2c1200feda 100644 --- a/packages/govuk-frontend/src/govuk/i18n.mjs +++ b/packages/govuk-frontend/src/govuk/i18n.mjs @@ -10,7 +10,7 @@ export class I18n { /** * @internal - * @param {{ [key: string]: unknown }} translations - Key-value pairs of the translation strings to use. + * @param {{ [key: string]: string | TranslationPluralForms }} translations - Key-value pairs of the translation strings to use. * @param {object} [config] - Configuration options for the function. * @param {string | null} [config.locale] - An overriding locale for the PluralRules functionality. */ @@ -39,33 +39,35 @@ export class I18n { throw new Error('i18n: lookup key missing') } + // Fetch the translation for that lookup key + let translation = this.translations[lookupKey] + // If the `count` option is set, determine which plural suffix is needed and // change the lookupKey to match. We check to see if it's numeric instead of // falsy, as this could legitimately be 0. - if (typeof options?.count === 'number') { - // Get the plural suffix - lookupKey = `${lookupKey}.${this.getPluralSuffix( - lookupKey, - options.count - )}` - } + if (typeof options?.count === 'number' && typeof translation === 'object') { + const translationPluralForm = + translation[this.getPluralSuffix(lookupKey, options.count)] - // Fetch the translation string for that lookup key - const translationString = this.translations[lookupKey] + // Update translation with plural suffix + if (translationPluralForm) { + translation = translationPluralForm + } + } - if (typeof translationString === 'string') { + if (typeof translation === 'string') { // Check for ${} placeholders in the translation string - if (translationString.match(/%{(.\S+)}/)) { + if (translation.match(/%{(.\S+)}/)) { if (!options) { throw new Error( 'i18n: cannot replace placeholders in string if no option data provided' ) } - return this.replacePlaceholders(translationString, options) + return this.replacePlaceholders(translation, options) } - return translationString + return translation } // If the key wasn't found in our translations object, @@ -174,6 +176,9 @@ export class I18n { return 'other' } + // Fetch the translation for that lookup key + const translation = this.translations[lookupKey] + // Check to verify that all the requirements for Intl.PluralRules are met. // If so, we can use that instead of our custom implementation. Otherwise, // use the hardcoded fallback. @@ -182,16 +187,18 @@ export class I18n { : this.selectPluralFormUsingFallbackRules(count) // Use the correct plural form if provided - if (`${lookupKey}.${preferredForm}` in this.translations) { - return preferredForm - // Fall back to `other` if the plural form is missing, but log a warning - // to the console - } else if (`${lookupKey}.other` in this.translations) { - console.warn( - `i18n: Missing plural form ".${preferredForm}" for "${this.locale}" locale. Falling back to ".other".` - ) + if (typeof translation === 'object') { + if (preferredForm in translation) { + return preferredForm + // Fall back to `other` if the plural form is missing, but log a warning + // to the console + } else if ('other' in translation) { + console.warn( + `i18n: Missing plural form ".${preferredForm}" for "${this.locale}" locale. Falling back to ".other".` + ) - return 'other' + return 'other' + } } // If the required `other` plural form is missing, all we can do is error From c847853edb6a32017bb06657bace8c8076d6d4e3 Mon Sep 17 00:00:00 2001 From: Colin Rotherham Date: Fri, 8 Mar 2024 13:00:07 +0000 Subject: [PATCH 09/12] Fix for namespace key name clashing with child key name --- packages/govuk-frontend/src/govuk/common/index.mjs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/packages/govuk-frontend/src/govuk/common/index.mjs b/packages/govuk-frontend/src/govuk/common/index.mjs index dcd7982f59..a1715d1d67 100644 --- a/packages/govuk-frontend/src/govuk/common/index.mjs +++ b/packages/govuk-frontend/src/govuk/common/index.mjs @@ -87,7 +87,7 @@ export function extractConfigByNamespace(Component, dataset, namespace) { // but check for object type on the next loop if (index < keyParts.length - 1) { current = current[name] = current[name] ?? {} - } else if (name !== namespace) { + } else if (key !== namespace) { // Add normalised value to overwrite default current[name] = normaliseString(value) } From 9358b7219de0e9fd7c2a7e77ef4cc2e3693d8d41 Mon Sep 17 00:00:00 2001 From: Colin Rotherham Date: Fri, 8 Mar 2024 13:03:20 +0000 Subject: [PATCH 10/12] Fix for objects (with nested values) not overwriting string values --- packages/govuk-frontend/src/govuk/common/index.mjs | 13 +++++++++---- 1 file changed, 9 insertions(+), 4 deletions(-) diff --git a/packages/govuk-frontend/src/govuk/common/index.mjs b/packages/govuk-frontend/src/govuk/common/index.mjs index a1715d1d67..d2e18910b1 100644 --- a/packages/govuk-frontend/src/govuk/common/index.mjs +++ b/packages/govuk-frontend/src/govuk/common/index.mjs @@ -83,12 +83,17 @@ export function extractConfigByNamespace(Component, dataset, namespace) { */ for (const [index, name] of keyParts.entries()) { if (typeof current === 'object') { - // Drop down to what we assume is a nested object - // but check for object type on the next loop + // Drop down to nested object until the last part if (index < keyParts.length - 1) { - current = current[name] = current[name] ?? {} + // New nested object (optionally) replaces existing value + if (!isObject(current[name])) { + current[name] = {} + } + + // Drop down into new or existing nested object + current = current[name] } else if (key !== namespace) { - // Add normalised value to overwrite default + // Normalised value (optionally) replaces existing value current[name] = normaliseString(value) } } From 8ce1dc419602429e95b6cfc4ca37b0ba702d8d0c Mon Sep 17 00:00:00 2001 From: Romaric Date: Fri, 8 Mar 2024 14:53:14 +0000 Subject: [PATCH 11/12] Add tests illustrating the behaviour of merging when both deep and shallow keys are set --- .../src/govuk/common/index.jsdom.test.mjs | 85 +++++++++++++++++++ 1 file changed, 85 insertions(+) diff --git a/packages/govuk-frontend/src/govuk/common/index.jsdom.test.mjs b/packages/govuk-frontend/src/govuk/common/index.jsdom.test.mjs index 1525bb0203..ce2cb7b097 100644 --- a/packages/govuk-frontend/src/govuk/common/index.jsdom.test.mjs +++ b/packages/govuk-frontend/src/govuk/common/index.jsdom.test.mjs @@ -235,6 +235,91 @@ describe('Common JS utilities', () => { expect(result).toEqual({ key1: 'One', key2: 'Two', key3: 'Three' }) }) + it('handles when both shallow and deep keys are set (namespace collision)', () => { + document.body.outerHTML = outdent` +
+
+ ` + + const { dataset } = document.getElementById('app-example') + const result = extractConfigByNamespace(Component, dataset, 'c') + + expect(result).toEqual({ a: 'cat', o: 'cow' }) + }) + + it('handles when both shallow and deep keys are set (namespace collision + key collision in namespace)', () => { + document.body.outerHTML = outdent` +
+
+ ` + + const { dataset } = document.getElementById('app-example') + const result = extractConfigByNamespace(Component, dataset, 'c') + + expect(result).toEqual({ a: 'cat', c: 'crow', o: 'cow' }) + }) + + it('handles when both shallow and deep keys are set (namespace collision + key collision in namespace after shallow)', () => { + document.body.outerHTML = outdent` +
+
+ ` + + const { dataset } = document.getElementById('app-example') + const result = extractConfigByNamespace(Component, dataset, 'c') + + expect(result).toEqual({ a: 'cat', c: 'crow', o: 'cow' }) + }) + + it('handles when both shallow and deep keys are set (deeper collision)', () => { + document.body.outerHTML = outdent` +
+
+ ` + + const { dataset } = document.getElementById('app-example') + const result = extractConfigByNamespace(Component, dataset, 'f') + + expect(result).toEqual({ e: { l: 'elephant' } }) + }) + it('can handle multiple levels of nesting', () => { document.body.outerHTML = outdent`
Date: Fri, 23 Feb 2024 09:07:05 +0000 Subject: [PATCH 12/12] Support component config schema `allOf` condition --- packages/govuk-frontend/src/govuk/common/index.mjs | 8 +++++++- 1 file changed, 7 insertions(+), 1 deletion(-) diff --git a/packages/govuk-frontend/src/govuk/common/index.mjs b/packages/govuk-frontend/src/govuk/common/index.mjs index d2e18910b1..2e2452117a 100644 --- a/packages/govuk-frontend/src/govuk/common/index.mjs +++ b/packages/govuk-frontend/src/govuk/common/index.mjs @@ -234,6 +234,11 @@ export function validateConfig(schema, config) { } } + // Check all conditions pass or add errors + if (name === 'allOf' && errors.length) { + validationErrors.push(...errors) + } + // Check one condition passes or add errors if (name === 'anyOf' && !(conditions.length - errors.length >= 1)) { validationErrors.push(...errors) @@ -271,7 +276,8 @@ function isObject(option) { * * @typedef {object} Schema * @property {{ [field: string]: SchemaProperty | undefined }} properties - Schema properties - * @property {SchemaCondition[]} [anyOf] - List of schema conditions + * @property {SchemaCondition[]} [allOf] - List of schema conditions, all must pass + * @property {SchemaCondition[]} [anyOf] - List of schema conditions, any must pass */ /**