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

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 3 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
33 changes: 26 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 Down Expand Up @@ -31,28 +32,46 @@ 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 (cause) {
throw new MissingElementError(
colinrotherham marked this conversation as resolved.
Show resolved Hide resolved
'Skip link: the linked HTML element does not exist',
{
cause: cause instanceof Error ? cause : undefined
Copy link
Contributor

Choose a reason for hiding this comment

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

For JavaScript built-in: Error: cause browser support are we happy treating this as optional?

If so, do you mind adding a bit of safety to the GOVUKFrontendError constructor?

i.e. Use feature detection, not a polyfill

export class GOVUKFrontendError extends Error {
  name = 'GOVUKFrontendError'

  /**
   * @param {string} message 
   * @param {ErrorOptions} options
   */
  constructor(message, { cause, ...options }) {
    super(message, options)

    // Add cause property where supported
    if (cause && new Error('test', { cause }).cause === cause) {
      this.cause = cause
    }
  }
}

We'll need to:

  1. ESLint ignore "ES2022 Error Cause is forbidden" for feature detection purposes
  2. ESLint ignore "ES2018 rest/spread properties are forbidden" if Babel handles it already
  3. Triple check my feature detection idea, maybe it needs a try/catch?

For 3) it's mainly because 'cause' in Error.prototype didn't work 🙈

Copy link
Contributor

Choose a reason for hiding this comment

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

Or we remove { cause } for another day?

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'm leaning toward leaving { cause } for now - feels like p'raps we need a bit more thinking about how we make it work and can do that as part of iterating on these errors.

I've pushed a change to use the error messages to convey the error to the user, whaddya reckon?

Copy link
Contributor

Choose a reason for hiding this comment

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

Very clever, absolutely love it

}
)
}

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 {Error} If the element with the specified ID does not exist
* @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(
`Skip link: $module "href" attribute does not contain a hash`
)
}

const linkedElement = document.getElementById(linkedElementId)

if (!linkedElement) {
throw new Error(
colinrotherham marked this conversation as resolved.
Show resolved Hide resolved
`Skip link: Target selector "#${linkedElementId}" not found`
)
}

return document.getElementById(linkedElementId)
return linkedElement
}

/**
Expand Down
14 changes: 14 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,19 @@ 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: the linked HTML element does not exist'
})
})
})
})
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