Skip to content

Commit

Permalink
Refactor what needs overriding when a component needs a specific type…
Browse files Browse the repository at this point in the history
… for $module

Switching to generic typing and a getter makes components that need a custom type for $module
require smaller changes with only:
- a comment
- a method override

Smaller impact and it systemises the text of the error message as well.

On top of that, TypeScript was not complaining if a component needing custom type
didn't redefine `checkModuleType`, only our test for a custom message caught that issue.

The price of this refactoring is a heavy line of casting in `checkModuleType` to help TypeScript
follow the dynamic type for the moduleType needing to be defined both as a type and as an actual value for use with `instanceof`.

Given the simple code of the function, I think we're fine here, especially as we're reducing the risk of human errors by having a less heavy method to override.
  • Loading branch information
romaricpascal committed Aug 18, 2023
1 parent 45e90c8 commit 8797d32
Show file tree
Hide file tree
Showing 2 changed files with 30 additions and 32 deletions.
Original file line number Diff line number Diff line change
Expand Up @@ -4,13 +4,13 @@ import { GOVUKFrontendComponent } from '../../govuk-frontend-component.mjs'
* Skip link component
*
* @preserve
* @augments GOVUKFrontendComponent<HTMLAnchorElement>
*/
export class SkipLink extends GOVUKFrontendComponent {
/**
* @override
* @type {HTMLAnchorElement}
*/
$module = this.$module
/** @override */
get moduleType() {
return HTMLAnchorElement
}

/**
* @private
Expand Down Expand Up @@ -108,19 +108,6 @@ export class SkipLink extends GOVUKFrontendComponent {
return this.$module.hash.split('#').pop()
}

/**
* @override
*/
checkModuleType($module) {
if (!($module instanceof HTMLAnchorElement)) {
throw new TypeError(
'Expected `$module` to be an instance of `HTMLAnchorElement`'
)
}

return $module
}

/**
* Name for the component used when initialising using data-module attributes.
*/
Expand Down
39 changes: 25 additions & 14 deletions packages/govuk-frontend/src/govuk/govuk-frontend-component.mjs
Original file line number Diff line number Diff line change
Expand Up @@ -8,11 +8,29 @@ import { SupportError } from './errors/index.mjs'
*
* @internal
* @abstract
* @template {Element} [ModuleType=HTMLElement]
*/
export class GOVUKFrontendComponent {
/**
* Defines the type of `Element` the components expects at instantiation
*
* If the component requires a more specialised type:
* - document the child class using `@extends`,
* setting the required type in the generic part of `GOVUKFrontendComponent`.
* For example: `GOVUKFrontendComponent<HTMLAnchorElement>`
* - override this method in the child class to return the same class
* set in the generic part
*
* @protected
* @returns {typeof HTMLElement} The type expected of $module
*/
get moduleType() {
return HTMLElement
}

/**
* @protected
* @type {HTMLElement}
* @type {ModuleType}
*/
$module

Expand Down Expand Up @@ -42,25 +60,18 @@ export class GOVUKFrontendComponent {
/**
* Validates the type of $module and returns it (so TypeScript can follow)
*
* If a component requires a more specialised type for `$module`:
* - override `$module` in the child class to set the required type.
* Don't forget to intialise it to `this.$module` to avoid it
* being reset to `undefined` after the component's constructor
* calls `super()`. See:
* https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Classes/Public_class_fields#examples
* - override this function to check for the relevant type
* and raise an error mentionning the appropriate type
*
* @protected
* @private
* @param {unknown} $module
*/
checkModuleType($module) {
if (!($module instanceof HTMLElement)) {
if (!($module instanceof this.moduleType)) {
throw new TypeError(
'Expected `$module` to be an instance of `HTMLElement`'
`Expected \`$module\` to be an instance of \`${this.moduleType.name}\``
)
}

return $module
// We have to help TypeScript a little bit here as it can't infer
// the type of `$module` due to it being dynamic
return /** @type {ModuleType} */ (/** @type {unknown} */ ($module))
}
}

0 comments on commit 8797d32

Please sign in to comment.