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

Various JSDoc + type checking fixes #2987

Merged
merged 15 commits into from
Feb 6, 2023
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
6 changes: 3 additions & 3 deletions app/app.js
Original file line number Diff line number Diff line change
Expand Up @@ -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) {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

TypeScript flagged this bit :exampleName*? not matching req.params.exampleName

  • /components/:componentName/:exampleName*?

But we could have changed it to either:

  1. /components/:componentName/*?
  2. /components/:componentName/:exampleName?

Which would have matched:

/components/accordion
/components/accordion/example1
/components/accordion/example2
/components/accordion/example3

So kept the one with :exampleName 👍

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Note: It worked fine before, but didn't match the documented type declarations

// 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'
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Sometimes componentData can be nullish


const exampleConfig = res.locals.componentData.examples.find(
const exampleConfig = res.locals.componentData?.examples.find(
example => example.name.replace(/ /g, '-') === exampleName
)

Expand Down
2 changes: 1 addition & 1 deletion config/jest/environment/jsdom.mjs
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down
2 changes: 1 addition & 1 deletion config/jest/environment/puppeteer.mjs
Original file line number Diff line number Diff line change
Expand Up @@ -19,7 +19,7 @@ class BrowserAutomationEnvironment extends PuppeteerEnvironment {
delete error.stack

// Ensure test fails
process.emit('error', error)
process.emit('uncaughtException', error)
romaricpascal marked this conversation as resolved.
Show resolved Hide resolved
Copy link
Contributor Author

Choose a reason for hiding this comment

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

The tsc compiler has flagged that the 2nd argument can only be type Error when emitting uncaughtException or uncaughtExceptionMonitor

I was a bit wary about this one due to:

It's the delete error.stack line above that's vital, as we were unable to do this before Jest Puppeteer's exitOnPageError handler fired previously.

Appears to be due to Jest's handling of error stack traces in: https://github.com/facebook/jest/pull/6281/files#diff-ec0ee442213b8202b52930e84121ed8a150142a0a8d8f8edff37ccf5d7029a34R56

})

// Add shared test globals
Expand Down
96 changes: 59 additions & 37 deletions docs/contributing/coding-standards/js.md
Original file line number Diff line number Diff line change
Expand Up @@ -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
Copy link
Contributor

Choose a reason for hiding this comment

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

My understanding this is the default behaviour when calling new Example anyway, which is how we can already chain init without the explicit return – is there a benefit in making it explicit?

I know our existing behaviour is to 'silently swallow' the error if the constructor is called with something that is not an element, but returning this feels a bit weird here. It's possibly even weirder because we only explicitly return to allow for chaining if things go wrong?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ahh sorry, I thought it was the preferred newer approach?

button.mjs#L18
character-count.mjs#L58
error-summary.mjs#LL27

I picked what looked like the most sensible constructor + init pattern

Happy to pick another flavour. Let me know what you'd prefer to do

Copy link
Contributor

Choose a reason for hiding this comment

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

@romaricpascal it looks like you might have added the return this as part of 08abe72 – is this something we need to be careful of with ES6 classes?

MDN just says:

If the constructor function returns a non-primitive, this return value becomes the result of the whole new expression. Otherwise, if the constructor function doesn't return anything or returns a primitive, newInstance is returned instead.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Will see what @romaricpascal says (maybe transpiler related??)

To confirm a few things this ES6 class demo shows we'll start seeing TypeError with anything except:

return      // ✅ implicit `this` class instance
return this // ✅ explicit `this` class instance
return {}   // ✅ explicit custom object return (why??)

But with extended classes try and do an early return false and get:

Safari

TypeError: Cannot return a non-object type in the constructor of a derived class

Chrome

Uncaught TypeError: Derived constructors may only return object or undefined

Firefox

TypeError: derived class constructor returned invalid value false

Copy link
Contributor Author

@colinrotherham colinrotherham Dec 6, 2022

Choose a reason for hiding this comment

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

Just done a deep dive into the WebKit code to check for legacy support

Safari 9

ES6: Classes: Early return in sub-class constructor results in returning undefined instead of instance
https://bugs.webkit.org/show_bug.cgi?id=143012 (fixed WebKit/WebKit@7ae9a15)

Notice how their tests cover functions too?

// Same behavior for Functions.
debug(''); debug('Function constructor (non-class)');
function FunctionNoReturn() { };
function FunctionReturnImplicit() { return; };
function FunctionReturnUndefined() { return undefined; };
function FunctionReturnThis() { return this; };

Safari 10

Same release <script type="module"> was controversially added without nomodule

[JSC] "return this" in a constructor does not need a branch on isObject(this)
https://bugs.webkit.org/show_bug.cgi?id=157775 (fixed WebKit/WebKit@d9e129c)

☝️ We'd probably want to keep return this but other variations did soon stabilise

Copy link
Member

Choose a reason for hiding this comment

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

@romaricpascal it looks like you might have added the return this as part of 08abe72 – is this something we need to be careful of with ES6 classes?

I just hadn't dug deep enough to realise that undefined was actually fine as a return value, sorry. I was mostly aware that return values matered in ES6 (as it's an opportunity to do cool stuff with Proxys by returning them in the constructor instead of letting things through). I prefered to err on the cautious side at the time.

If it makes old Safari happy as well, though, sounds like we should keep it (until we move to ES classes) 😄

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@romaricpascal it looks like you might have added the return this as part of 08abe72 – is this something we need to be careful of with ES6 classes?

@36degrees Any last thoughts on return versus return this?

Copy link
Contributor

Choose a reason for hiding this comment

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

Not really… I'd be tempted to try and maintain the status quo as much as possible as I think it's likely we're going to make changes to this in v5 anyway. No point sweating things like this if we can potentially make the problem go away entirely in the near future 🪄

Copy link
Contributor Author

Choose a reason for hiding this comment

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

You might have noticed I've pushed up a new version

I've kept return this for new Component() as per these examples:

button.mjs#L20
character-count.mjs#L61
error-summary.mjs#L29

But for .init() I've removed all trace of return this and kept things as-is

Also removed it from the existing (incorrect) example:

var myExample = new Example().init()
var myExample = new Example()

}

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
Expand All @@ -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)
}
```

Expand All @@ -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

Expand Down
62 changes: 41 additions & 21 deletions lib/file-helper.js
Original file line number Diff line number Diff line change
@@ -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'))
Expand All @@ -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 || {}

Expand All @@ -27,7 +32,6 @@ const cache = global.cache || {}
*/
const getListing = async (directoryPath, pattern = '**/*', options = {}) => {
const listing = await glob(pattern, {
allowEmpty: true,
Copy link
Contributor

Choose a reason for hiding this comment

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

Just to check – this has been removed because it's a non-existent option?

(glob's options docs suggest this might be the case)

Copy link
Member

Choose a reason for hiding this comment

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

Yup that's it indeed 😄

cwd: directoryPath,
matchBase: true,
nodir: true,
Expand All @@ -51,7 +55,7 @@ const getDirectories = (directoryPath) => {

// Retrieve from cache
if (directories) {
return directories
return Promise.resolve(directories)
}

// Read from disk
Expand All @@ -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, {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Shuffled this around only to avoid adding JSDoc @param {string} pattern

matchBase: true
})

// Return true for files matching every pattern
return patterns.every(isMatch)
return patterns.every(
(pattern) => minimatch(entryPath, pattern, {
matchBase: true
})
)
}

/**
Expand Down Expand Up @@ -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)
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Removed the try/catch as it wraps and re-throws the error that would have thrown anyway

return {
name: componentName,
...yamlData
}
}

Expand Down Expand Up @@ -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)
*/

Expand All @@ -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
*
Expand Down
2 changes: 1 addition & 1 deletion lib/file-helper.unit.test.js
Original file line number Diff line number Diff line change
Expand Up @@ -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/)
Copy link
Contributor Author

Choose a reason for hiding this comment

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

We've removed a try/catch wrapping new Error() so the full error message is now coming through

})

it('outputs objects with an array of params and examples', async () => {
Expand Down
10 changes: 3 additions & 7 deletions lib/helper-functions.js
Original file line number Diff line number Diff line change
Expand Up @@ -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 = {
Expand Down
Loading