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.

Sign up for GitHub

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

Jump to bottom

Add MissingElementError and use it within the Skip Link #4177

Merged
merged 4 commits into from
Sep 7, 2023
Merged

Add MissingElementError and use it within the Skip Link #4177

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
462146a
Add MissingElementError
domoscargin Sep 5, 2023
b56cebf
Throw MissingElementError if key HTML missing
domoscargin Sep 5, 2023
c61fb5a
Throw more useful errors
domoscargin Sep 5, 2023
e1eff14
Use error message strings instead of cause
domoscargin Sep 7, 2023
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
31 changes: 24 additions & 7 deletions packages/govuk-frontend/src/govuk/components/skip-link/skip-link.mjs
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,3 +1,4 @@
import { MissingElementError } from '../../errors/index.mjs'
import { GOVUKFrontendComponent } from '../../govuk-frontend-component.mjs'

/**
Expand All @@ -20,6 +21,7 @@ export class SkipLink extends GOVUKFrontendComponent {

/**
* @param {Element} $module - HTML element to use for skip link
* @throws {MissingElementError} If the element with the specified ID is not found
*/
constructor($module) {
super()
Expand All @@ -31,28 +33,43 @@ export class SkipLink extends GOVUKFrontendComponent {
this.$module = $module

// Check for linked element
const $linkedElement = this.getLinkedElement()
if (!$linkedElement) {
return
try {
const $linkedElement = this.getLinkedElement()
this.$linkedElement = $linkedElement
} catch (error) {
throw new MissingElementError(
colinrotherham marked this conversation as resolved.
Show resolved Hide resolved
`Skip link: ${
error instanceof Error ? error.message : 'Linked element not found'
}`
)
}

this.$linkedElement = $linkedElement
this.$module.addEventListener('click', () => this.focusLinkedElement())
}

/**
* Get linked element
*
* @private
* @returns {HTMLElement | null} $linkedElement - DOM element linked to from the skip link
* @throws {Error} If the "href" attribute does not contain a hash
* @throws {TypeError} If the element with the specified ID is not found
* @returns {HTMLElement} $linkedElement - DOM element linked to from the skip link
domoscargin marked this conversation as resolved.
Show resolved Hide resolved
*/
getLinkedElement() {
const linkedElementId = this.getFragmentFromUrl()
if (!linkedElementId) {
return null
throw new Error(`$module "href" attribute does not contain a hash`)
}

const linkedElement = document.getElementById(linkedElementId)

if (!linkedElement) {
throw new TypeError(
`Linked element selector "#${linkedElementId}" not found`
)
}

return document.getElementById(linkedElementId)
return linkedElement
}

/**
Expand Down
29 changes: 29 additions & 0 deletions packages/govuk-frontend/src/govuk/components/skip-link/skip-link.test.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -81,5 +81,34 @@ describe('Skip Link', () => {
message: 'GOV.UK Frontend is not supported in this browser'
})
})

it('throws when the linked element is missing', async () => {
await expect(
renderAndInitialise(page, 'skip-link', {
params: {
text: 'Skip to main content',
href: '#this-element-does-not-exist'
}
})
).rejects.toEqual({
name: 'MissingElementError',
message:
'Skip link: Linked element selector "#this-element-does-not-exist" not found'
})
})

it('throws when the href does not contain a hash', async () => {
await expect(
renderAndInitialise(page, 'skip-link', {
params: {
text: 'Skip to main content',
href: 'this-element-does-not-exist'
}
})
).rejects.toEqual({
name: 'MissingElementError',
message: 'Skip link: $module "href" attribute does not contain a hash'
})
})
})
})
7 changes: 7 additions & 0 deletions packages/govuk-frontend/src/govuk/errors/index.mjs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -22,6 +22,13 @@ export class GOVUKFrontendError extends Error {
name = 'GOVUKFrontendError'
}

/**
* Indicates that a Component's required HTML element is missing
*/
export class MissingElementError extends GOVUKFrontendError {
name = 'MissingElementError'
}
Comment on lines +28 to +30
Copy link
Contributor

@colinrotherham colinrotherham Sep 7, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

Not a blocker, but it's triggered a discussion

To keep file size down, could we use Error { cause } to keep our custom errors to a minimum? Otherwise our published package will grow as custom errors are added

E.g. Using TypeError "…not of the expected type" for null or missing things

Component errors

Here's an example with nested { cause } to make the error more informative:

throw new ComponentError('I has died', {
  cause: new TypeError("Argh, 'ere be no link target #pirates"),
  component: 'Skip link'
}

But using the nested { cause } approach to surface all that useful information. It's handy for monitoring tools like Sentry which links chained errors + stack traces

For extra user friendliness a custom toString() (see below) could log the component name:

ComponentError: "Skip link: I has died"
 → Cause: TypeError: "Argh, 'ere be no link target #pirates"

Suggested change
export class MissingElementError extends GOVUKFrontendError {
name = 'MissingElementError'
}
export class ComponentError extends GOVUKFrontendError {
name = 'ComponentError'
component = 'Component name'
toString() {
return `${this.component}: ${this.message}`;
}
}

Copy link
Contributor

Choose a reason for hiding this comment

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

There's also a gotcha

We'd need to confirm Babel transforms or a polyfill for JavaScript built-in: Error: cause


/**
* Indicates that GOV.UK Frontend is not supported
*/
Expand Down