Merge pull request #2987 from alphagov/fix-types

Various JSDoc + type checking fixes

Author: colinrotherham

app/app.js

@@ -155,14 +155,14 @@ module.exports = async (options) => {
   })
 
   // Component example preview
-  app.get('/components/:componentName/:exampleName*?/preview', function (req, res, next) {
+  app.get('/components/:componentName/:exampleName?/preview', function (req, res, next) {
     // Find the data for the specified example (or the default example)
     const componentName = req.params.componentName
     const exampleName = req.params.exampleName || 'default'
 
-    const previewLayout = res.locals.componentData.previewLayout || 'layout'
+    const previewLayout = res.locals.componentData?.previewLayout || 'layout'
 
-    const exampleConfig = res.locals.componentData.examples.find(
+    const exampleConfig = res.locals.componentData?.examples.find(
       example => example.name.replace(/ /g, '-') === exampleName
     )
 

config/jest/environment/jsdom.mjs

@@ -14,7 +14,7 @@ class BrowserVirtualEnvironment extends TestEnvironment {
     const { virtualConsole } = this.dom
 
     // Ensure test fails for browser exceptions
-    virtualConsole.on('jsdomError', (error) => process.emit('error', error))
+    virtualConsole.on('jsdomError', (error) => process.emit('uncaughtException', error))
 
     // Add shared test globals
     // componentsData, componentsDirectory, examplesDirectory

config/jest/environment/puppeteer.mjs

@@ -19,7 +19,7 @@ class BrowserAutomationEnvironment extends PuppeteerEnvironment {
       delete error.stack
 
       // Ensure test fails
-      process.emit('error', error)
+      process.emit('uncaughtException', error)
     })
 
     // Add shared test globals

docs/contributing/coding-standards/js.md

@@ -5,25 +5,45 @@
 JavaScript files have the same name as the component's folder name. Test files have a `.test` suffix placed before the file extension.
 
 ```
-checkboxes
-├── checkboxes.mjs
-└── checkboxes.test.js
+component
+├── component.mjs
+└── component.test.js
 ```
 
 ## Skeleton
 
 ```js
-import { nodeListForEach } from '../vendor/common.mjs'
+import '../../vendor/polyfills/Element.mjs'
 
-function Checkboxes ($module) {
-  // code goes here
+/**
+ * Component name
+ *
+ * @class
+ * @param {Element} $module - HTML element to use for component
+ */
+function Example ($module) {
+  if (!$module) {
+    return this
+  }
+
+  this.$module = $module
+
+  // Code goes here
 }
 
-Checkboxes.prototype.init = function () {
-  // code goes here
+/**
+ * Initialise component
+ */
+Example.prototype.init = function () {
+  // Check that required elements are present
+  if (!this.$module) {
+    return
+  }
+
+  // Code goes here
 }
 
-export default Checkboxes
+export default Example
 ```
 
 ## Use data attributes to initialise component JavaScript
@@ -48,15 +68,15 @@ Use `/** ... */` for multi-line comments. Include a description, and specify typ
 
 ```js
 /**
-* Get the nearest ancestor element of a node that matches a given tag name
-* @param {object} node element
-* @param {string} match tag name (e.g. div)
-* @return {object} ancestor element
-*/
-
-function (node, match) {
-  // code goes here
-  return ancestor
+ * Get the first descendent (child) of an HTML element that matches a given tag name
+ *
+ * @param {Element} $element - HTML element
+ * @param {string} tagName - Tag name (for example 'div')
+ * @returns {Element} Ancestor element
+ */
+function ($element, tagName) {
+  // Code goes here
+  return $element.querySelector(tagName)
 }
 ```
 
@@ -73,52 +93,54 @@ Use the prototype design pattern to structure your code.
 Create a constructor and define any variables that the object needs.
 
 ```js
-function Checkboxes ($module) {
-  // code goes here
+function Example ($module) {
+  // Code goes here
 }
 ```
 
 Assign methods to the prototype object. Do not overwrite the prototype with a new object as this makes inheritance impossible.
 
 ```js
-// bad
-Checkboxes.prototype = {
+// Bad
+Example.prototype = {
   init: function () {
-    // code goes here
+    // Code goes here
   }
 }
 
-// good
-Checkboxes.prototype.init = function () {
-  // code goes here
+// Good
+Example.prototype.init = function () {
+  // Code goes here
 }
 ```
 
 When initialising an object, use the `new` keyword.
 
 ```js
-// bad
-var myCheckbox = Checkbox().init()
+// Bad
+var myExample = Example()
 
-// good
-var myCheckbox = new Checkbox().init()
+// Good
+var myExample = new Example()
 ```
 
 ## Modules
 
-Use ES6 modules (`import`/`export`) over a non-standard module system. You can always transpile to your preferred module system.
+Use ECMAScript modules (`import`/`export`) over CommonJS and other formats. You can always transpile to your preferred module system.
 
 ```js
-import { nodeListForEach } from '../vendor/common.mjs'
-// code goes here
-export default Checkboxes
+import { closestAttributeValue } from '../common/index.mjs'
+
+// Code goes here
+export function exampleHelper1 () {}
+export function exampleHelper2 () {}
 ```
 
-Avoid using wildcard (`import * as nodeListForEach`) imports.
+You must specify the file extension when using the import keyword.
 
-You must specify the file extension for a file when importing it.
+Avoid using namespace imports (`import * as namespace`) in code transpiled to CommonJS (or AMD) bundled code as this can prevent "tree shaking" optimisations.
 
-Use default export over named export.
+Prefer named exports over default exports to avoid compatibility issues with transpiler "synthetic default" as discussed in: https://github.com/alphagov/govuk-frontend/issues/2829
 
 ## Polyfilling
 

lib/file-helper.js

@@ -1,5 +1,5 @@
 const { readFile } = require('fs/promises')
-const { join, parse, ParsedPath, relative } = require('path')
+const { join, parse, relative } = require('path')
 const { promisify } = require('util')
 
 const glob = promisify(require('glob'))
@@ -14,6 +14,11 @@ const configPaths = require('../config/paths')
  * Used to cache slow operations
  *
  * See `config/jest/globals.mjs`
+ *
+ * @type {{
+ *  directories?: Map<string, string[]>,
+ *  componentsData?: ComponentData[]
+ * }}
  */
 const cache = global.cache || {}
 
@@ -27,7 +32,6 @@ const cache = global.cache || {}
  */
 const getListing = async (directoryPath, pattern = '**/*', options = {}) => {
   const listing = await glob(pattern, {
-    allowEmpty: true,
     cwd: directoryPath,
     matchBase: true,
     nodir: true,
@@ -51,7 +55,7 @@ const getDirectories = (directoryPath) => {
 
   // Retrieve from cache
   if (directories) {
-    return directories
+    return Promise.resolve(directories)
   }
 
   // Read from disk
@@ -66,12 +70,11 @@ const getDirectories = (directoryPath) => {
  * @returns {function(string): boolean} Returns true for files matching every pattern
  */
 const filterPath = (patterns) => (entryPath) => {
-  const isMatch = (pattern) => minimatch(entryPath, pattern, {
-    matchBase: true
-  })
-
-  // Return true for files matching every pattern
-  return patterns.every(isMatch)
+  return patterns.every(
+    (pattern) => minimatch(entryPath, pattern, {
+      matchBase: true
+    })
+  )
 }
 
 /**
@@ -107,16 +110,12 @@ const getComponentData = async (componentName) => {
   }
 
   // Read from disk
-  try {
-    const yamlPath = join(configPaths.components, componentName, `${componentName}.yaml`)
-    const yamlData = yaml.load(await readFile(yamlPath, 'utf8'), { json: true })
+  const yamlPath = join(configPaths.components, componentName, `${componentName}.yaml`)
+  const yamlData = yaml.load(await readFile(yamlPath, 'utf8'), { json: true })
 
-    return {
-      name: componentName,
-      ...yamlData
-    }
-  } catch (error) {
-    throw new Error(error)
+  return {
+    name: componentName,
+    ...yamlData
   }
 }
 
@@ -177,7 +176,7 @@ module.exports = {
  * Directory entry path mapper callback
  *
  * @callback mapPathToCallback
- * @param {ParsedPath} file - Parsed file
+ * @param {import('path').ParsedPath} file - Parsed file
  * @returns {string[]} Returns path (or array of paths)
  */
 
@@ -186,12 +185,33 @@ module.exports = {
  *
  * @typedef {object} ComponentData
  * @property {string} name - Component name
- * @property {unknown[]} [params] - Nunjucks macro options
- * @property {unknown[]} [examples] - Example Nunjucks macro options
+ * @property {ComponentOption[]} [params] - Nunjucks macro options
+ * @property {ComponentExample[]} [examples] - Example Nunjucks macro options
  * @property {string} [previewLayout] - Nunjucks layout for component preview
  * @property {string} [accessibilityCriteria] - Accessibility criteria
  */
 
+/**
+ * Component option from YAML
+ *
+ * @typedef {object} ComponentOption
+ * @property {string} name - Option name
+ * @property {string} type - Option type
+ * @property {boolean} required - Option required
+ * @property {string} description - Option description
+ * @property {boolean} [isComponent] - Option is another component
+ * @property {ComponentOption[]} [params] - Nested Nunjucks macro options
+ */
+
+/**
+ * Component example from YAML
+ *
+ * @typedef {object} ComponentExample
+ * @property {string} name - Example name
+ * @property {object} data - Example data
+ * @property {boolean} [hidden] - Example hidden from review app
+ */
+
 /**
  * Full page example from front matter
  *

lib/file-helper.unit.test.js

@@ -4,7 +4,7 @@ describe('getComponentData', () => {
   it('rejects if unable to load component data', async () => {
     await expect(fileHelper.getComponentData('not-a-real-component'))
       .rejects
-      .toThrow('Error: ENOENT: no such file or directory')
+      .toThrow(/ENOENT: no such file or directory/)
   })
 
   it('outputs objects with an array of params and examples', async () => {

lib/helper-functions.js

@@ -67,15 +67,11 @@ function componentPathToModuleName (componentPath) {
  * the project root node_modules
  *
  * @param {string} packageName - Installed npm package name
- * @param {...string} [paths] - Optional child directory paths
+ * @param {string} [childPath] - Optional child directory path
  * @returns {string} Path to installed npm package
  */
-function packageNameToPath (packageName, ...paths) {
-  const packagePath = dirname(require.resolve(`${packageName}/package.json`))
-
-  return paths?.length
-    ? join(packagePath, ...paths)
-    : packagePath
+function packageNameToPath (packageName, childPath = '') {
+  return join(dirname(require.resolve(`${packageName}/package.json`)), childPath)
 }
 
 module.exports = {