Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Spike into Config class if defined by child class of GOVUKFrontendComponent #5427

Draft
wants to merge 2 commits into
base: main
Choose a base branch
from
Draft
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
133 changes: 133 additions & 0 deletions packages/govuk-frontend/src/govuk/common/config.mjs
Original file line number Diff line number Diff line change
@@ -0,0 +1,133 @@
import { ConfigError } from '../errors/index.mjs'

import { isObject } from './index.mjs'
import { normaliseDataset } from './normalise-dataset.mjs'

/**
* Config
*
* Instance of configuration for a component
*
* @template {{[key:string]: unknown}} [ConfigType=ObjectNested]
*/
class Config {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nitpick Playing around in VSCode, the name config leads to all .config.js file appearing when looking for the file with Ctrl+P. Configuration would avoid this.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

issue We prefer named exports, which limit the risks of name being changed when imported, making it easier to debug.

Suggested change
class Config {
export class Config {

/**
* @type {ConfigType}
*/
configObject

/**
* @type {CompatibleClass<ConfigType>}
*/
component

/**
* Merge configuration objects into a single config
*
* I think this makes sense to go in here rather then
* as utility function because it is used each time a
* configuration is created in the constructor of a component.
* So it would not be removed during tree-shaking.
*
* @param {...{[key:string]: unknown}} configObjects - configuration objects passed
* @returns {{[key:string]: unknown}} - merged configuration object
*/
static mergeConfigs(...configObjects) {
Comment on lines +24 to +35
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

note: It's neat that it's been regrouped with the Config class rather than be in common/index.mjs. However, having it as a function in the module would allow its name to be mangled by minifiers (they leave methods alone as they may be called by outside code).

It'd likely need to be exported for unit tests, but we can document it as @internal to keep its use to ourselves.

// 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) {
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] = Config.mergeConfigs(option, override)
} else {
// Apply override
formattedConfigObject[key] = override
}
}
}

return formattedConfigObject
}

/**
* @param {ComponentClass} component - Class of component using config
* @param {DOMStringMap} dataset - dataset of root component
* @param {...ConfigType} configObjects - Config objects to merge
*/
constructor(component, dataset, ...configObjects) {
if (typeof component.defaults === 'undefined') {
throw new ConfigError('No defaults specified in component')
}

if (typeof component.schema === 'undefined') {
throw new ConfigError('No schema specified in component')
}
Comment on lines +72 to +74
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

note It doesn't seem the class does anything of the schema beyond storing it. Thinking we can leave that out of the class for now (for simplicity), at least until we look at building validation inside the class).


this.component = /** @type {CompatibleClass<ConfigType>} */ (component)

const normalisedDataset = /** @type {ConfigType} */ (
normaliseDataset(this.component, dataset)
)

this.configObject = /** @type {ConfigType} */ (
Config.mergeConfigs(
this.component.defaults,
...configObjects,
this.component.configOverride
? this.component.configOverride(normalisedDataset)
: {},
normalisedDataset
)
)

// have to assign to a separate variable
// to get the Proxy to work
const configObject = this.configObject

return new Proxy(this, {
get(target, name, receiver) {
if (!Reflect.has(target, name)) {
return configObject[String(name)]
}
return Reflect.get(target, name, receiver)
}
})
}
}

/**
* @typedef {import("./index.mjs").ObjectNested} ObjectNested
*/

/* eslint-disable jsdoc/valid-types --
* `{new(...args: any[] ): object}` is not recognised as valid
* https://github.com/gajus/eslint-plugin-jsdoc/issues/145#issuecomment-1308722878
* https://github.com/jsdoc-type-pratt-parser/jsdoc-type-pratt-parser/issues/131
**/

/**
* @typedef {{new (...args: any[]): any, moduleName: string, schema?: {[key:string]: unknown}, defaults?: {[key:string]: unknown} }} ComponentClass
*/
Comment on lines +118 to +120
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

question Do you think we need to review the types around what makes a component class? Thinking we have that in a couple of places now (around createAll, around the GOVUKFrontendComponent class) and we may want to organise this a little bit into proper concepts: a constructor + having a moduleName + having configuration static properties (defaults at least).


/* eslint-disable jsdoc/valid-types --
* `{new(...args: any[] ): object}` is not recognised as valid
* https://github.com/gajus/eslint-plugin-jsdoc/issues/145#issuecomment-1308722878
* https://github.com/jsdoc-type-pratt-parser/jsdoc-type-pratt-parser/issues/131
**/

/**
* @template {{[key:string]: unknown}} [ConfigType=ObjectNested]
* @typedef {{new (...args: any[]): any, moduleName: string, schema: import('./index.mjs').Schema, defaults: ConfigType, configOverride?: (config: ConfigType) => ConfigType }} CompatibleClass
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nitpick Thinking CompatibleClass is a little generic here (it's also the case for the one used with createAll), can we maybe name it ConfigurableComponent or similar?

*/

export default Config
12 changes: 6 additions & 6 deletions packages/govuk-frontend/src/govuk/common/index.mjs
Original file line number Diff line number Diff line change
Expand Up @@ -229,21 +229,22 @@ export function isSupported($scope = document.body) {
* {@link https://ajv.js.org/packages/ajv-errors.html#single-message}
*
* @internal
* @param {Schema} schema - Config schema
* @param {{ [key: string]: unknown }} config - Component config
* @param {import('./config.mjs').default<{[key:string]: unknown}>} config - instance of config
* @returns {string[]} List of validation errors
*/
export function validateConfig(schema, config) {
export function validateConfig(config) {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

issue I'd be keen to leave the validation completely outside the work on Config for now, until we figure whether we want to validate the configuration as part of our public API. That would avoid the Config object needing to store the schema.

const validationErrors = []

const schema = config.component.schema

// Check errors for each schema
for (const [name, conditions] of Object.entries(schema)) {
const errors = []

// Check errors for each schema condition
if (Array.isArray(conditions)) {
for (const { required, errorMessage } of conditions) {
if (!required.every((key) => !!config[key])) {
if (!required.every((key) => config.configObject[key])) {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

question Given the Config is a Proxy, what prevents us from accessing the key via config directly? Is it TypeScript?

errors.push(errorMessage) // Missing config key value
}
}
Expand All @@ -254,7 +255,6 @@ export function validateConfig(schema, config) {
}
}
}

return validationErrors
}

Expand All @@ -276,7 +276,7 @@ function isArray(option) {
* @param {unknown} option - Option to check
* @returns {boolean} Whether the option is an object
*/
function isObject(option) {
export function isObject(option) {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

note Nice call leaving that one in index.mjs.

return !!option && typeof option === 'object' && !isArray(option)
}

Expand Down
Original file line number Diff line number Diff line change
@@ -1,5 +1,4 @@
import { mergeConfigs } from '../../common/index.mjs'
import { normaliseDataset } from '../../common/normalise-dataset.mjs'
import Config from '../../common/config.mjs'
import { ElementError } from '../../errors/index.mjs'
import { GOVUKFrontendComponent } from '../../govuk-frontend-component.mjs'
import { I18n } from '../../i18n.mjs'
Expand All @@ -21,7 +20,7 @@ import { I18n } from '../../i18n.mjs'
export class Accordion extends GOVUKFrontendComponent {
/**
* @private
* @type {AccordionConfig}
* @type {Config<AccordionConfig> & AccordionConfig}
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

question I'm not understanding the combination of types on this one, sorry. What's requiring us to combine those?

*/
config

Expand Down Expand Up @@ -113,11 +112,7 @@ export class Accordion extends GOVUKFrontendComponent {
constructor($root, config = {}) {
super($root)

this.config = mergeConfigs(
Accordion.defaults,
config,
normaliseDataset(Accordion, this.$root.dataset)
)
this.config = new Config(Accordion, this.$root.dataset, config)

this.i18n = new I18n(this.config.i18n)

Expand Down
11 changes: 3 additions & 8 deletions packages/govuk-frontend/src/govuk/components/button/button.mjs
Original file line number Diff line number Diff line change
@@ -1,5 +1,4 @@
import { mergeConfigs } from '../../common/index.mjs'
import { normaliseDataset } from '../../common/normalise-dataset.mjs'
import Config from '../../common/config.mjs'
import { GOVUKFrontendComponent } from '../../govuk-frontend-component.mjs'

const DEBOUNCE_TIMEOUT_IN_SECONDS = 1
Expand All @@ -12,7 +11,7 @@ const DEBOUNCE_TIMEOUT_IN_SECONDS = 1
export class Button extends GOVUKFrontendComponent {
/**
* @private
* @type {ButtonConfig}
* @type {Config<ButtonConfig> & ButtonConfig}
*/
config

Expand All @@ -29,11 +28,7 @@ export class Button extends GOVUKFrontendComponent {
constructor($root, config = {}) {
super($root)

this.config = mergeConfigs(
Button.defaults,
config,
normaliseDataset(Button, this.$root.dataset)
)
this.config = new Config(Button, this.$root.dataset, config)

this.$root.addEventListener('keydown', (event) => this.handleKeyDown(event))
this.$root.addEventListener('click', (event) => this.debounce(event))
Expand Down
Original file line number Diff line number Diff line change
@@ -1,10 +1,6 @@
import { closestAttributeValue } from '../../common/closest-attribute-value.mjs'
import {
formatErrorMessage,
mergeConfigs,
validateConfig
} from '../../common/index.mjs'
import { normaliseDataset } from '../../common/normalise-dataset.mjs'
import Config from '../../common/config.mjs'
import { formatErrorMessage, validateConfig } from '../../common/index.mjs'
import { ConfigError, ElementError } from '../../errors/index.mjs'
import { GOVUKFrontendComponent } from '../../govuk-frontend-component.mjs'
import { I18n } from '../../i18n.mjs'
Expand Down Expand Up @@ -48,7 +44,7 @@ export class CharacterCount extends GOVUKFrontendComponent {

/**
* @private
* @type {CharacterCountConfig}
* @type {Config<CharacterCountConfig> & CharacterCountConfig}
*/
config

Expand Down Expand Up @@ -80,33 +76,28 @@ export class CharacterCount extends GOVUKFrontendComponent {
})
}

// Read config set using dataset ('data-' values)
const datasetConfig = normaliseDataset(CharacterCount, this.$root.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
// the JavaScript config.
//
// We can't mutate `config`, though, as it may be shared across multiple
// components inside `initAll`.
/** @type {CharacterCountConfig} */
let configOverrides = {}
if ('maxwords' in datasetConfig || 'maxlength' in datasetConfig) {
configOverrides = {
maxlength: undefined,
maxwords: undefined
}
}

this.config = mergeConfigs(
CharacterCount.defaults,
config,
configOverrides,
datasetConfig
)
// // Read config set using dataset ('data-' values)
// const datasetConfig = normaliseDataset(CharacterCount, this.$root.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
// // the JavaScript config.
// //
// // We can't mutate `config`, though, as it may be shared across multiple
// // components inside `initAll`.
// /** @type {CharacterCountConfig} */
// let configOverrides = {}
// if ('maxwords' in datasetConfig || 'maxlength' in datasetConfig) {
// configOverrides = {
// maxlength: undefined,
// maxwords: undefined
// }
// }

this.config = new Config(CharacterCount, this.$root.dataset, config)

// Check for valid config
const errors = validateConfig(CharacterCount.schema, this.config)
const errors = validateConfig(this.config)
if (errors[0]) {
throw new ConfigError(formatErrorMessage(CharacterCount, errors[0]))
}
Expand Down Expand Up @@ -440,6 +431,25 @@ export class CharacterCount extends GOVUKFrontendComponent {
}
})

/**
* Override configuration
*
* @param {CharacterCountConfig} config - config to override
* @returns {CharacterCountConfig} - overidden config
*/
static configOverride = (config) => {
/** @type {CharacterCountConfig} */
let configOverrides = {}
if ('maxwords' in config || 'maxlength' in config) {
configOverrides = {
maxlength: undefined,
maxwords: undefined
}
}
Comment on lines +440 to +448
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

note I like the way you've solved the issue here. 🙌🏻

Thinking we should keep that feature @internal for now, though, as it's mostly due to the current set of options in this component. Then if people come asking for such feature for their components, we can look at offering it as part of the public API (and consider naming...). What are your thoughts?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

suggestion Static methods are natively supported in browsers we transpile to, while static fields will require extra code.

Suggested change
static configOverride = (config) => {
/** @type {CharacterCountConfig} */
let configOverrides = {}
if ('maxwords' in config || 'maxlength' in config) {
configOverrides = {
maxlength: undefined,
maxwords: undefined
}
}
static configOverride(config) {
/** @type {CharacterCountConfig} */
let configOverrides = {}
if ('maxwords' in config || 'maxlength' in config) {
configOverrides = {
maxlength: undefined,
maxwords: undefined
}
}


return configOverrides
}

/**
* Character count config schema
*
Expand Down