From fa02f521507d6b252d584af92d141cf5cedacedc Mon Sep 17 00:00:00 2001
From: Romaric Pascal <romaric.pascal@digital.cabinet-office.gov.uk>
Date: Fri, 18 Aug 2023 17:03:55 +0100
Subject: [PATCH] Refactor what needs overriding when a component needs a
 specific type 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.
---
 .../govuk/components/skip-link/skip-link.mjs  | 23 +++--------
 .../src/govuk/govuk-frontend-component.mjs    | 41 ++++++++++++-------
 2 files changed, 31 insertions(+), 33 deletions(-)

diff --git a/packages/govuk-frontend/src/govuk/components/skip-link/skip-link.mjs b/packages/govuk-frontend/src/govuk/components/skip-link/skip-link.mjs
index 0392f35fe8..ef96827a18 100644
--- a/packages/govuk-frontend/src/govuk/components/skip-link/skip-link.mjs
+++ b/packages/govuk-frontend/src/govuk/components/skip-link/skip-link.mjs
@@ -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
@@ -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.
    */
diff --git a/packages/govuk-frontend/src/govuk/govuk-frontend-component.mjs b/packages/govuk-frontend/src/govuk/govuk-frontend-component.mjs
index 358232b625..3a7baf76c4 100644
--- a/packages/govuk-frontend/src/govuk/govuk-frontend-component.mjs
+++ b/packages/govuk-frontend/src/govuk/govuk-frontend-component.mjs
@@ -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 `@augments`,
+   *   setting the required type in the generic part of `GOVUKFrontendComponent`:
+   *   For example: `@augments 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
 
@@ -41,26 +59,19 @@ 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
-   * @returns {HTMLElement} `$module`, cast as the type expected by the class
+   * @returns {ModuleType} `$module`, cast as the type expected by the class
    */
   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))
   }
 }