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

Add JSDoc to document plugin configs etc #2386

Merged
merged 7 commits into from
Mar 8, 2024
Merged
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
7 changes: 5 additions & 2 deletions __tests__/utils/index.js
Original file line number Diff line number Diff line change
Expand Up @@ -41,10 +41,13 @@ function mkdtempSync () {
* @param {string} prototypePath
* @param {Object} [options]
* @param {string} [options.kitPath] - Path to the kit to use when creating prototype, if not provided uses mkReleaseArchive
* @param {bool} [options.allowTracking] - If undefined no usage-data-config.json is created,
* @param {boolean} [options.overwrite] - Allow existing prototype to be overwritten (optional)
* @param {boolean} [options.allowTracking] - If undefined no usage-data-config.json is created (optional),
* if true a usage-data-config.json is created allowing tracking,
* if false a usage-data-config.json is crated disallowing tracking
* @returns {void}
* @param {boolean} [options.npmInstallLinks] - Set value for npm config install-links (optional)
* @param {string} [options.commandLineParameters] - Command line parameters (optional)
* @returns {Promise<void>}
*/
async function mkPrototype (prototypePath, {
kitPath,
Expand Down
23 changes: 21 additions & 2 deletions bin/utils/argv-parser.js
Original file line number Diff line number Diff line change
@@ -1,11 +1,23 @@
/**
*
* @param {NodeJS.Process["argv"]} argvInput
* @param {{ booleans?: string[] }} config
*/
function parse (argvInput, config = {}) {
const args = [...argvInput].splice(2)
const options = {}
const paths = []
const options = /** @type {Object<string, string | boolean>} */ ({})
const paths = /** @type {string[]} */ ([])
const booleanOptions = config.booleans || []

/** @type {string | undefined} */
let command

/** @type {{ option: string } | undefined} */
let contextFromPrevious

/**
* @param {string} unprocessed
*/
const processOptionName = (unprocessed) => {
if (unprocessed.startsWith('--')) {
return unprocessed.substring(2)
Expand All @@ -15,6 +27,9 @@ function parse (argvInput, config = {}) {
}
}

/**
* @param {string} arg
*/
const prepareArg = (arg) => {
if ((arg.startsWith('"') && arg.endsWith('"')) || (arg.startsWith('\'') && arg.endsWith('\''))) {
return arg.substring(1, arg.length - 1)
Expand Down Expand Up @@ -64,3 +79,7 @@ function parse (argvInput, config = {}) {
module.exports = {
parse
}

/**
* @typedef {ReturnType<typeof parse>} ArgvParsed
*/
2 changes: 1 addition & 1 deletion lib/govukFrontendPaths.js
Original file line number Diff line number Diff line change
Expand Up @@ -47,5 +47,5 @@ module.exports = {
* @property {string} baseDir - GOV.UK Frontend directory path
* @property {URL["pathname"]} includePath - URL path to GOV.UK Frontend includes
* @property {URL["pathname"]} assetPath - URL path to GOV.UK Frontend assets
* @property {{ [key: string]: unknown }} config - GOV.UK Frontend plugin config
* @property {import('./plugins/plugins').ConfigManifest} config - GOV.UK Frontend plugin config
*/
77 changes: 67 additions & 10 deletions lib/plugins/plugin-validator.js
Original file line number Diff line number Diff line change
Expand Up @@ -21,18 +21,32 @@ const knownKeys = [
'meta'
]

/**
* @param {string} executionPath
* @param {string} pathToValidate
* @param {string} key
*/
function checkPathExists (executionPath, pathToValidate, key) {
const absolutePathToValidate = path.join(executionPath, pathToValidate)
if (!fse.existsSync(absolutePathToValidate)) errors.push(`In section ${key}, the path '${pathToValidate}' does not exist`)
}

/**
* @param {string} executionPath
* @param {string} nunjucksFileName
* @param {string | string[]} nunjucksPaths
Copy link
Contributor

Choose a reason for hiding this comment

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

I can't see any logic to support nunjucksPath being a string rather than an array of strings – I think we'd end up iterating over each character in the string on line 48?

It looks like this might also be optional?

Should it be…

Suggested change
* @param {string | string[]} nunjucksPaths
* @param {string[]} [nunjucksPaths]

Copy link
Contributor Author

@colinrotherham colinrotherham Mar 4, 2024

Choose a reason for hiding this comment

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

Ah sorry, the cypress tests use strings and a few places in the wild too:

UKHomeOffice/home_office_design_kit/blob/master/govuk-prototype-kit.config.json
that-firefly/testing-personal-prototypes/blob/testing-eg/govuk-prototype-kit.config.json

Possibly because we documented below (bit out of date) that strings were allowed?

* "nunjucksPaths": string | string[],
* "nunjucksFilters": string | string[],
* "nunjucksFunctions": string | string[],
* "pluginDependencies": [{"packageName": string, "minVersion": string, "maxVersion": string}],

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Definitely give #2354 a try to see lots more red squiggles

Similar to 77b7943 you'll see it's still possible to access configEntry.path and configEntry.importFrom even though configEntry might be a string etc

Or sometimes we just check the key name but not the type:

} else if (key === 'nunjucksMacros') {
checkNunjucksMacroExists(executionPath, configEntry.importFrom, pluginConfig.nunjucksPaths)
} else if (typeof configEntry === 'string' && configEntry[0] === '/') {

Copy link
Contributor

Choose a reason for hiding this comment

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

Looks like possibly the kit supports nunjucksPaths being a string but this validator doesn't allow for it?

I mean, technically you can pass a string, but it'll try and iterate over it character by character and result in errors – what are we trying to document in the JSDoc? Technical compatibility or how it's designed to be used?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Looks like possibly the kit supports nunjucksPaths being a string but this validator doesn't allow for it?

Yeah that's it exactly

Just tried it with GOV.UK and everything works. The code was simplified recently but it's flat() that does the magic:

nunjucksViews.push(...[config.nunjucksPaths].flat()
  .map(nunjucksPath => path.join(baseDir, nunjucksPath))

Copy link
Contributor Author

@colinrotherham colinrotherham Mar 5, 2024

Choose a reason for hiding this comment

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

what are we trying to document in the JSDoc? Technical compatibility or how it's designed to be used?

I added JSDoc to document how the kit is currently used

That way (in theory) the compiler can error or show red squiggles on all the code that doesn't support the current usage. Except for looping strings, which is valid 🤦‍♂️

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I've pushed up a tweak to handle undefined and strings at the same time in 5e534e7

Ideally we should leave broken things broken and triage them?

*/
function checkNunjucksMacroExists (executionPath, nunjucksFileName, nunjucksPaths) {
// set up a flag for the existance of a nunjucks path
let nunjucksPathExists = false

if (nunjucksPaths === undefined) {
// Check if the nunjucksMacros are at the root level of the project
if (fse.existsSync(path.join(executionPath, nunjucksFileName))) nunjucksPathExists = true
if (!nunjucksPaths || typeof nunjucksPaths === 'string') {
const pathToCheck = typeof nunjucksPaths === 'string'
? path.join(executionPath, nunjucksPaths, nunjucksFileName)
: path.join(executionPath, nunjucksFileName) // root level

// Check if the nunjucksMacros are at a single path in the project
if (fse.existsSync(pathToCheck)) nunjucksPathExists = true
} else {
// Combine the file path name for each nunjucks path and check if any one of them exists
for (const nunjucksPath of nunjucksPaths) {
Expand All @@ -47,14 +61,22 @@ function checkNunjucksMacroExists (executionPath, nunjucksFileName, nunjucksPath
if (!nunjucksPathExists) errors.push(`The nunjucks file '${nunjucksFileName}' does not exist`)
}

/**
* @param {string[]} invalidKeys
*/
function reportInvalidKeys (invalidKeys) {
errors.push(`The following invalid keys exist in your config: ${invalidKeys}`)
}

/**
* @param {ConfigManifest} pluginConfig
* @param {ArgvParsed} argv
* @returns {ConfigManifest}
*/
function validateConfigKeys (pluginConfig, argv) {
console.log('Config file exists, validating contents.')
const keysToAllowThrough = argv?.options?.keysToIgnoreIfUnknown || ''
const allowedKeys = knownKeys.concat(keysToAllowThrough.split(',').filter(key => !!key))
const keysToAllowThrough = `${argv?.options?.keysToIgnoreIfUnknown || ''}`
const allowedKeys = [...knownKeys, ...keysToAllowThrough.split(',').filter(key => !!key)]
const invalidKeys = []

const validKeysPluginConfig = Object.fromEntries(Object.entries(pluginConfig).filter(([key]) => {
Expand All @@ -73,13 +95,22 @@ function validateConfigKeys (pluginConfig, argv) {
return validKeysPluginConfig
}

/**
* @template {ConfigEntry | ConfigURLs} ObjectType
* @param {ObjectType} objectToEvaluate
* @param {string[]} allowedKeys
* @param {string} [keyPath]
*/
function reportUnknownKeys (objectToEvaluate, allowedKeys, keyPath) {
const invalidMetaUrlKeys = Object.keys(objectToEvaluate).filter(key => !allowedKeys.includes(key)).map(key => `${keyPath || ''}${key}`)
if (invalidMetaUrlKeys.length > 0) {
reportInvalidKeys(invalidMetaUrlKeys)
}
}

/**
* @param {string} url
*/
function isValidUrl (url) {
if (!url.startsWith('https://') && !url.startsWith('http://')) {
return false
Expand All @@ -90,6 +121,9 @@ function isValidUrl (url) {
return true
}

/**
* @param {ConfigURLs} [metaUrls]
*/
function validateMetaUrls (metaUrls) {
if (typeof metaUrls === 'undefined') {
return
Expand Down Expand Up @@ -121,6 +155,9 @@ function validateMetaUrls (metaUrls) {
})
}

/**
* @param {ConfigEntry} meta
*/
function validateMeta (meta) {
const metaKeys = ['urls', 'description']

Expand All @@ -142,6 +179,10 @@ function validateMeta (meta) {
validateMetaUrls(meta.urls)
}

/**
* @param {string} key
* @param {ConfigEntry} configEntry
*/
function validatePluginDependency (key, configEntry) {
if (typeof configEntry !== 'object' || Array.isArray(configEntry)) {
return
Expand All @@ -165,16 +206,20 @@ function validatePluginDependency (key, configEntry) {
}
}

/**
* @param {ConfigManifest} pluginConfig
* @param {string} executionPath
*/
function validateConfigurationValues (pluginConfig, executionPath) {
console.log('Validating whether config paths meet criteria.')
const keysToValidate = Object.keys(pluginConfig)

keysToValidate.forEach(key => {
// Convert any strings to an array so that they can be processed
let criteriaConfig = pluginConfig[key]
if (!Array.isArray(criteriaConfig)) {
criteriaConfig = [criteriaConfig]
}
/**
* Convert any strings to an array so that they can be processed
* @type {ConfigEntry[]}
*/
const criteriaConfig = [pluginConfig[key]].flat()

criteriaConfig.forEach((configEntry) => {
try {
Expand Down Expand Up @@ -203,11 +248,16 @@ function validateConfigurationValues (pluginConfig, executionPath) {
})
}

/**
* @param {string} executionPath
* @param {ArgvParsed} argv
*/
async function validatePlugin (executionPath, argv) {
console.log()
const configPath = path.join(executionPath, 'govuk-prototype-kit.config.json')
await fse.exists(configPath).then(exists => {
if (exists) {
/** @type {ConfigManifest | undefined} */
let pluginConfig
try {
pluginConfig = JSON.parse(fs.readFileSync(configPath, 'utf8'))
Expand Down Expand Up @@ -267,3 +317,10 @@ module.exports = {
validateMeta,
validatePluginDependency
}

/**
* @typedef {import('./plugins').ConfigManifest} ConfigManifest
* @typedef {import('./plugins').ConfigURLs} ConfigURLs
* @typedef {import('./plugins').ConfigEntry} ConfigEntry
* @typedef {import('../../bin/utils/argv-parser').ArgvParsed} ArgvParsed
*/
130 changes: 95 additions & 35 deletions lib/plugins/plugins.js
Original file line number Diff line number Diff line change
Expand Up @@ -9,37 +9,8 @@
* The kit code retrieves the paths as and when needed; this module just
* contains the code to find and list paths defined by plugins.
*
* A schema for an example manifest file follows:
*
* // govuk-prototype-kit.config.json
* {
* "assets": string | string[],
* "importNunjucksMacrosInto": string | string[],
* "meta": {
* "description": string,
* "urls": {
* "documentation": string,
* "versionHistory": string,
* "releaseNotes": string
* }
* },
* "nunjucksMacros": {"importFrom": string, "macroName": string} | {"importFrom": string, "macroName": string}[],
* "nunjucksPaths": string | string[],
* "nunjucksFilters": string | string[],
* "nunjucksFunctions": string | string[],
* "pluginDependencies": [{"packageName": string, "minVersion": string, "maxVersion": string}],
* "sass": string | string[],
* "scripts": string | string[] | {"path": string, "type": string} | {"path": string, "type": string}[],
* "stylesheets": string | string[],
* "templates": {
* "name": string,
* "path": string,
* "type": string
* }[]
* }
*
* Note that all the top-level keys are optional.
*
* See JSDoc `ConfigManifest` for example govuk-prototype-kit.json manifiest
* {@link ConfigManifest}
*/

// core dependencies
Expand Down Expand Up @@ -313,8 +284,9 @@ const getByType = type => getList(type)

/**
* Gets public urls for all plugins of type
* @param {string} listType - (scripts, stylesheets, nunjucks etc)
* @return {string[]} A list of urls
* @template {'assets' | 'scripts' | 'stylesheets'} ListType
* @param {ListType} listType - (scripts, stylesheets, nunjucks etc)
* @return {ListType extends 'scripts' ? AppScript[] | string[] : string[]} A list of urls or script objects
*/
const getPublicUrls = listType => getList(listType).map(({ packageName, item }) => {
// item will either be the plugin path or will be an object containing the plugin path within the src property
Expand Down Expand Up @@ -343,8 +315,8 @@ const getPublicUrlAndFileSystemPaths = type => getList(type).map(getPublicUrlAnd

/**
* This is used in the views to output links and scripts for each file
* @param {{scripts: string[], stylesheets: string[]}} additionalConfig
* @return {{scripts: {src: string, type: string}[], stylesheets: string[]}} Returns an object containing two keys(scripts & stylesheets),
* @param {Partial<{ scripts: (AppScript | string)[], stylesheets: string[] }>} [additionalConfig]
* @return {{ scripts: AppScript[], stylesheets: string[] }} Returns an object containing two keys(scripts & stylesheets),
* each item contains an array of full paths to specific files.
*/
function getAppConfig (additionalConfig) {
Expand Down Expand Up @@ -385,3 +357,91 @@ const self = module.exports = {
setPluginsByType,
watchPlugins
}

/**
* Prototype Kit plugin config
*
* Schema for govuk-prototype-kit.json manifiest
* Note: All top-level keys are optional
*
* @typedef {object} ConfigManifest
* @property {string | string[]} [assets] - Static asset paths
* @property {string | string[]} [importNunjucksMacrosInto] - Templates to import Nunjucks macros into
* @property {string | string[]} [nunjucksPaths] - Nunjucks paths
* @property {ConfigNunjucksMacro[]} [nunjucksMacros] - Nunjucks macros to include
* @property {string | string[]} [nunjucksFilters] - Nunjucks filters to include
* @property {string | string[]} [nunjucksFunctions] - Nunjucks functions to include
* @property {string | string[]} [sass] - Sass stylesheets to compile
* @property {ConfigScript[] | string[]} [scripts] - JavaScripts to serve
* @property {string | string[]} [stylesheets] - Stylesheets to serve
* @property {ConfigTemplate[]} [templates] - Templates available
* @property {ConfigDependency[]} [pluginDependencies] - Plugin dependencies
* @property {ConfigMeta} [meta] - Plugin metadata
*/

/**
* Prototype Kit plugin Nunjucks macro
*
* @typedef {object} ConfigNunjucksMacro
* @property {string} macroName - Nunjucks macro name
* @property {string} importFrom - Path to import Nunjucks macro from
*/

/**
* Prototype Kit plugin script
*
* @typedef {object} ConfigScript
* @property {string} path - Path to script
* @property {string} [type] - Type attribute for script
*/

/**
* Prototype Kit plugin template
*
* @typedef {object} ConfigTemplate
* @property {string} name - Template name
* @property {string} path - Path to template
* @property {string} type - Template type
*/

/**
* Prototype Kit plugin dependency
*
* @typedef {object} ConfigDependency
* @property {string} packageName - Package name
* @property {string} minVersion - Package minimum version
* @property {string} maxVersion - Package maximum version
*/

/**
* Prototype Kit plugin metadata
*
* @typedef {object} ConfigMeta
* @property {string} description - Plugin description
* @property {ConfigURLs} urls - Plugin URLs
*/

/**
* Prototype Kit plugin URLs
*
* @typedef {object} ConfigURLs
* @property {string} documentation - Documentation URL
* @property {string} releaseNotes - Release notes URL
* @property {string} versionHistory - Version history URL
*/

/**
* Prototype Kit application script
*
* Plugin {@link ConfigScript} objects use `path` keys but
* these keys are renamed to `src` once imported into the
* application by `plugins.getAppConfig()`
*
* @typedef {object} AppScript
* @property {string} src - Path to script
* @property {string} [type] - Type attribute for script
*/

/**
* @typedef {string | string[] | ConfigNunjucksMacro | ConfigScript | ConfigTemplate | ConfigDependency | ConfigMeta} ConfigEntry
*/
Loading
Loading