Review and guard all missing fields/attributes

[colinrotherham]

Ahead of #4036 I've spent some time reviewing all optional things accessed without guards

We should follow this PR up with any extra errors we'd like to throw, although I've already split out:

1. Assert Exit this page HTMLAnchorElement button and clearly guard all dynamic fields #4321
2. Move getFragmentFromUrl() to common utilities #4320

For example, in PR #4320 we've identified tab links without hash fragments (already handled in Skip link)

Missing elements, attributes etc

• Class fields with incorrect null default
• Selector null return values not handled
• Timeout and interval IDs cleared when null
• Confg allows null as typeof value === 'object'

[github-actions]

📋 Stats

File sizes

File	Size
dist/govuk-frontend-4.7.0.min.css	120.66 KiB
dist/govuk-frontend-4.7.0.min.js	51.56 KiB
dist/govuk-frontend-ie8-4.7.0.min.css	81.59 KiB
packages/govuk-frontend/dist/govuk/all.bundle.js	76.76 KiB
packages/govuk-frontend/dist/govuk/all.bundle.mjs	72.07 KiB
packages/govuk-frontend/dist/govuk/all.mjs	3.8 KiB
packages/govuk-frontend/dist/govuk/govuk-frontend-component.mjs	359 B
packages/govuk-frontend/dist/govuk/govuk-frontend.min.css	113.99 KiB
packages/govuk-frontend/dist/govuk/govuk-frontend.min.js	37.83 KiB
packages/govuk-frontend/dist/govuk/i18n.mjs	5.3 KiB

Modules

File	Size
all.mjs	68.4 KiB
components/accordion/accordion.mjs	21.05 KiB
components/button/button.mjs	4.18 KiB
components/character-count/character-count.mjs	20.49 KiB
components/checkboxes/checkboxes.mjs	5.31 KiB
components/error-summary/error-summary.mjs	5.49 KiB
components/exit-this-page/exit-this-page.mjs	15.49 KiB
components/header/header.mjs	3.39 KiB
components/notification-banner/notification-banner.mjs	4.02 KiB
components/radios/radios.mjs	4.31 KiB
components/skip-link/skip-link.mjs	3.3 KiB
components/tabs/tabs.mjs	9.07 KiB

View stats and visualisations on the review app

---

Action run for 50eb86e

[romaricpascal]

Most of the changes look good to me 🙌🏻 After a deeper look, this adds a lot of extra ifs and || so thinking this warrants a wider discussion across @alphagov/design-system-developers.

In quite a few places it looks like we're adding code branches that'll never run, though, so I've added a few questions to try and understand why they're needed. I'm worried that to please TypeScript's strict mode (in #4106), we're having to add ifs (or ||) of which we know the result for sure (but TypeScript doesn't) 😬

[romaricpascal]

question Given how we validate the configuration, we'll never reach the || Infinity, I think. What's requiring us to keep it?

[colinrotherham]

Ah it's only moved it from the class field to here to make the compiler happy there's no undefined

Perhaps when we pick up #4230 we can look at CharacterCountConfig becoming Required<CharacterCountConfig> when all fields are validated?

Then the compiler will know a fallback is no longer needed

[romaricpascal]

question We're creating these elements ourselves, what's requiring us to add this early return?

[colinrotherham]

Ah we discussed via Slack thread to use silent returns (not errors) for self-created elements

I've been running ESLint @typescript-eslint/stylistic-type-checked with @domoscargin in #4106 so we can teach ESLint to be aware of types. These three are all null at instantiation

[romaricpascal]

Returning rather than throwing is fine as we're creating these elements, as discussed on the Slack thread.

Shame TypeScript needs an extra nudge leading to us checking things we do know exist 😔

[colinrotherham]

Yeah it's a shame, but good to be consistent

Alternatively we could consider getters and avoid null defaults entirely?

Here's a diff of what that would look like:
null-checks...null-checks-getters

[romaricpascal]

question I think this will never happen:

• We early return if this.$menu is falsy
• We throw if this.$menu exists and this.$menuButton is falsy
• And if we haven't thrown, we create this.mql ourselves

What's requiring us to add this early return?

[colinrotherham]

Same as we discussed via Slack thread again

Don't forget that this.$menu being falsy causes this.mql to stay as null

Unlike in Tabs, the compiler spotted that this.mql isn't always set in the constructor

Can see all these with strict mode turned on in packages/govuk-frontend/tsconfig.build.json

{
  "extends": "../../tsconfig.base.json",
  "include": ["./src/govuk/**/*.mjs"],
  "exclude": ["**/*.test.*"],
  "compilerOptions": {
    "lib": ["ESNext", "DOM"],
+  "strict": true,
    "target": "ES2015",
    "types": ["node"]
  }
}

[colinrotherham]

As an alternative, I've mentioned getters in #4323 (comment)

Changing this.mql to a getter (without a setter) ensures it only runs when first accessed:

get mql() {
  if (!this._mql) {
    this._mql = window.matchMedia('(min-width: 48.0625em)')
  }

  return this._mql
}

[colinrotherham]

@romaricpascal I've split this PR into separate commits to show what each change is for

Agree that textContent being null is frustrating so I've suggested an alternative below:

- ariaLabelParts.push(($summary.textContent || '').trim())
+ ariaLabelParts.push(`${$summary.textContent}`.trim())

But this is only necessary when running the TypeScript compiler with strict or strictNullChecks enabled since the WHATWG DOM Living Standard shows the Node interface textContent property can be null

From the mdn Node page:

In addition, every kind of DOM node is represented by an interface based on Node. These include Attr, CharacterData (which Text, Comment, CDATASection and ProcessingInstruction are all based on), and DocumentType.

It's a shame the compiler can't filter out other Node types that aren't possible here

Update: I've moved these ones to the strict mode PR

• Enable ESLint stylistic typed linting in source code #4106

[colinrotherham]

@romaricpascal @domoscargin I've updated this PR to move anything "new" or for discussion into:

• Allow ES2015+ supported features #4286
• Enable ESLint stylistic typed linting in source code #4106

This PR now includes only guards we've used elsewhere

[romaricpascal]

Cheers for reducing the scope of it to the less controversial changes. Happy with that ⛵ .

I'm warming up to that getter pattern as well, it's quite neat. On your branch, it make the build of the elements of the accordion neatly encapsulated, on top of fixing the null issue.