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

Throw SupportError when instantiating components where GOV.UK Frontend is not supported #4030

Merged
merged 5 commits into from
Aug 11, 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
34 changes: 34 additions & 0 deletions docs/contributing/coding-standards/js.md
Original file line number Diff line number Diff line change
Expand Up @@ -136,6 +136,40 @@ Avoid using namespace imports (`import * as namespace`) in code bundled for Comm

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

## Throwing errors
Copy link
Member Author

Choose a reason for hiding this comment

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

This is just a starter for 10 listing the spirit in which the errors have been implemented in this PR. Happy to discuss and update.


### Error types

First, check if one of the [native errors provides](https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Global_Objects/Error#error_types) the right semantic for the error you're looking to throw, if so, use this class.

If none of the native errors have the right semantics, create a new class for this error. This class should:

- have a name ending in `Error` to match the native conventions
- extend `GOVUKFrontendError`, so users can separate our custom errors from native ones using `instanceof`
- have a `name` property set to its class name, as extending classes doesn't set this automatically and grabbing the constructor's name risks being affected by mangling during minification

```js
class CustomError extends GOVUKFrontendError {
name = 'CustomError'
}
```

### Error message

Keep the message to the point, but provide the users the information they need to identify the cause of the error.

If the message is the same whatever the situation, you may use the constructor of our custom error to centralise that message, rather than passing it each time an error is thrown.

```js
class SupportError extends GOVUKFrontendError {
name = 'SupportError'

constructor () {
super('GOV.UK Frontend is not supported in this browser')
}
}
```

## Polyfilling

If you need polyfills for features that are not yet included in this project, please see the following guide on [how to add polyfills](../polyfilling.md).
Expand Down
14 changes: 14 additions & 0 deletions packages/govuk-frontend/src/govuk/common/index.mjs
Original file line number Diff line number Diff line change
Expand Up @@ -133,3 +133,17 @@ export function extractConfigByNamespace(configObject, namespace) {
}
return newObject
}

/**
* Checks if GOV.UK Frontend is supported on this page
*
* Some browsers will load and run our JavaScript but GOV.UK Frontend
* won't be supported.
*
* @internal
* @param {HTMLElement} [$scope] - The `<body>` element of the document to check for support
* @returns {boolean} Whether GOV.UK Frontend is supported on this page
*/
export function isSupported($scope = document.body) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Do we have browser support for parameter defaults without Babel polyfills?

Copy link
Member Author

Choose a reason for hiding this comment

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

Yup, Babel leaves that line as is in dist. Support looks a bit older than what we're configured to support. And if transpilation happened, there's no polyfills involved. So all good there 🎉

return $scope.classList.contains('govuk-frontend-supported')
}
24 changes: 23 additions & 1 deletion packages/govuk-frontend/src/govuk/common/index.unit.test.mjs
Original file line number Diff line number Diff line change
@@ -1,4 +1,8 @@
import { mergeConfigs, extractConfigByNamespace } from './index.mjs'
import {
mergeConfigs,
extractConfigByNamespace,
isSupported
} from './index.mjs'

describe('Common JS utilities', () => {
describe('mergeConfigs', () => {
Expand Down Expand Up @@ -112,4 +116,22 @@ describe('Common JS utilities', () => {
expect(() => extractConfigByNamespace(flattenedConfig)).toThrow()
})
})

describe.only('isSupported', () => {
beforeEach(() => {
// Jest does not tidy the JSDOM document between tests
// so we need to take care of that ourselves
document.documentElement.innerHTML = ''
})

it('returns true if the govuk-frontend-supported class is set', () => {
document.body.classList.add('govuk-frontend-supported')

expect(isSupported(document.body)).toBe(true)
})

it('returns false if the govuk-frontend-supported class is not set', () => {
expect(isSupported(document.body)).toBe(false)
})
})
})
Original file line number Diff line number Diff line change
@@ -1,5 +1,6 @@
import { mergeConfigs, extractConfigByNamespace } from '../../common/index.mjs'
import { normaliseDataset } from '../../common/normalise-dataset.mjs'
import { GOVUKFrontendComponent } from '../../govuk-frontend-component.mjs'
import { I18n } from '../../i18n.mjs'

/**
Expand All @@ -14,7 +15,7 @@ import { I18n } from '../../i18n.mjs'
* The state of each section is saved to the DOM via the `aria-expanded`
* attribute, which also provides accessibility.
*/
export class Accordion {
export class Accordion extends GOVUKFrontendComponent {
/** @private */
$module

Expand Down Expand Up @@ -113,10 +114,9 @@ export class Accordion {
* @param {AccordionConfig} [config] - Accordion config
*/
constructor($module, config) {
if (
!($module instanceof HTMLElement) ||
!document.body.classList.contains('govuk-frontend-supported')
) {
super()

if (!($module instanceof HTMLElement)) {
return this
}

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -712,6 +712,28 @@ describe('/components/accordion', () => {
)
})
})

describe('errors at instantiation', () => {
let examples

beforeAll(async () => {
examples = await getExamples('accordion')
})

it('throws when GOV.UK Frontend is not supported', async () => {
await expect(
renderAndInitialise(page, 'accordion', {
params: examples.default,
beforeInitialisation() {
document.body.classList.remove('govuk-frontend-supported')
Copy link
Contributor

Choose a reason for hiding this comment

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

🎉

}
})
).rejects.toEqual({
name: 'SupportError',
message: 'GOV.UK Frontend is not supported in this browser'
})
})
})
})
})
})
Expand Down
10 changes: 5 additions & 5 deletions packages/govuk-frontend/src/govuk/components/button/button.mjs
Original file line number Diff line number Diff line change
@@ -1,13 +1,14 @@
import { mergeConfigs } from '../../common/index.mjs'
import { normaliseDataset } from '../../common/normalise-dataset.mjs'
import { GOVUKFrontendComponent } from '../../govuk-frontend-component.mjs'

const KEY_SPACE = 32
const DEBOUNCE_TIMEOUT_IN_SECONDS = 1

/**
* JavaScript enhancements for the Button component
*/
export class Button {
export class Button extends GOVUKFrontendComponent {
/** @private */
$module

Expand All @@ -29,10 +30,9 @@ export class Button {
* @param {ButtonConfig} [config] - Button config
*/
constructor($module, config) {
if (
!($module instanceof HTMLElement) ||
!document.body.classList.contains('govuk-frontend-supported')
) {
super()

if (!($module instanceof HTMLElement)) {
return this
}

Expand Down
22 changes: 22 additions & 0 deletions packages/govuk-frontend/src/govuk/components/button/button.test.js
Original file line number Diff line number Diff line change
Expand Up @@ -338,5 +338,27 @@ describe('/components/button', () => {
expect(button2Counts.debounce).toBe(0)
})
})

describe('errors at instantiation', () => {
let examples

beforeAll(async () => {
examples = await getExamples('button')
})

it('throws when GOV.UK Frontend is not supported', async () => {
await expect(
renderAndInitialise(page, 'button', {
params: examples.default,
beforeInitialisation() {
document.body.classList.remove('govuk-frontend-supported')
}
})
).rejects.toEqual({
name: 'SupportError',
message: 'GOV.UK Frontend is not supported in this browser'
})
})
})
})
})
Original file line number Diff line number Diff line change
@@ -1,6 +1,7 @@
import { closestAttributeValue } from '../../common/closest-attribute-value.mjs'
import { extractConfigByNamespace, mergeConfigs } from '../../common/index.mjs'
import { normaliseDataset } from '../../common/normalise-dataset.mjs'
import { GOVUKFrontendComponent } from '../../govuk-frontend-component.mjs'
import { I18n } from '../../i18n.mjs'

/**
Expand All @@ -13,7 +14,7 @@ import { I18n } from '../../i18n.mjs'
* You can configure the message to only appear after a certain percentage
* of the available characters/words has been entered.
*/
export class CharacterCount {
export class CharacterCount extends GOVUKFrontendComponent {
/** @private */
$module

Expand Down Expand Up @@ -64,10 +65,9 @@ export class CharacterCount {
* @param {CharacterCountConfig} [config] - Character count config
*/
constructor($module, config) {
if (
!($module instanceof HTMLElement) ||
!document.body.classList.contains('govuk-frontend-supported')
) {
super()

if (!($module instanceof HTMLElement)) {
return this
}

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -763,6 +763,28 @@ describe('Character count', () => {
)
})
})

describe('errors at instantiation', () => {
let examples

beforeAll(async () => {
examples = await getExamples('character-count')
})

it('throws when GOV.UK Frontend is not supported', async () => {
await expect(
renderAndInitialise(page, 'character-count', {
params: examples.default,
beforeInitialisation() {
document.body.classList.remove('govuk-frontend-supported')
}
})
).rejects.toEqual({
name: 'SupportError',
message: 'GOV.UK Frontend is not supported in this browser'
})
})
})
})

describe('in mismatched locale', () => {
Expand All @@ -777,7 +799,7 @@ describe('Character count', () => {
// Override maxlength to 10
maxlength: 10
},
initialiser($module) {
beforeInitialisation($module) {
// Set locale to Welsh, which expects translations for 'one', 'two',
// 'few' 'many' and 'other' forms – with the default English strings
// provided we only have translations for 'one' and 'other'.
Expand Down
Original file line number Diff line number Diff line change
@@ -1,7 +1,9 @@
import { GOVUKFrontendComponent } from '../../govuk-frontend-component.mjs'

/**
* Checkboxes component
*/
export class Checkboxes {
export class Checkboxes extends GOVUKFrontendComponent {
/** @private */
$module

Expand All @@ -23,10 +25,9 @@ export class Checkboxes {
* @param {Element} $module - HTML element to use for checkboxes
*/
constructor($module) {
if (
!($module instanceof HTMLElement) ||
!document.body.classList.contains('govuk-frontend-supported')
) {
super()

if (!($module instanceof HTMLElement)) {
return this
}

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -3,8 +3,10 @@ const {
goToExample,
getAttribute,
getProperty,
isVisible
isVisible,
renderAndInitialise
} = require('govuk-frontend-helpers/puppeteer')
const { getExamples } = require('govuk-frontend-lib/files.js')

describe('Checkboxes with conditional reveals', () => {
describe('when JavaScript is unavailable or fails', () => {
Expand Down Expand Up @@ -316,5 +318,27 @@ describe('Checkboxes with multiple groups and a "None" checkbox and conditional
// Expect conditional content to have been collapsed
expect(await isVisible($conditionalPrimary)).toBe(false)
})

describe('errors at instantiation', () => {
let examples

beforeAll(async () => {
examples = await getExamples('checkboxes')
})

it('throws when GOV.UK Frontend is not supported', async () => {
await expect(
renderAndInitialise(page, 'checkboxes', {
params: examples.default,
beforeInitialisation() {
document.body.classList.remove('govuk-frontend-supported')
}
})
).rejects.toEqual({
name: 'SupportError',
message: 'GOV.UK Frontend is not supported in this browser'
})
})
})
})
})
Original file line number Diff line number Diff line change
@@ -1,12 +1,13 @@
import { mergeConfigs } from '../../common/index.mjs'
import { normaliseDataset } from '../../common/normalise-dataset.mjs'
import { GOVUKFrontendComponent } from '../../govuk-frontend-component.mjs'

/**
* Error summary component
*
* Takes focus on initialisation for accessible announcement, unless disabled in configuration.
*/
export class ErrorSummary {
export class ErrorSummary extends GOVUKFrontendComponent {
/** @private */
$module

Expand All @@ -22,17 +23,9 @@ export class ErrorSummary {
* @param {ErrorSummaryConfig} [config] - Error summary config
*/
constructor($module, config) {
// Some consuming code may not be passing a module,
// for example if they initialise the component
// on their own by directly passing the result
// of `document.querySelector`.
// To avoid breaking further JavaScript initialisation
Copy link
Contributor

Choose a reason for hiding this comment

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

I was quite fond of this message, all on its own in ErrorSummary

Thoughts on adding a little disclaimer to every component?

  1. Error throws
  2. Error console shows stack trace
  3. Stack trace uses original source via source maps
  4. Developer clicks to these lines and reads helpful message

Copy link
Member Author

Choose a reason for hiding this comment

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

I think most of that info should be summarised in the error message when implementing https://github.com/alphagov/govuk-frontend/issues/4035.

Regarding adding things to each components, I think this would rather be the role of a base Component class. I think with the error throwing for GOV.UK Frontend support and the type check of $module there's enough ground for creating it already (only to add new features to it, not refactor existing code into it) and centralise these checks and possible comments.

// we need to safeguard against this so things keep
// working the same now we read the elements data attributes
if (
!($module instanceof HTMLElement) ||
!document.body.classList.contains('govuk-frontend-supported')
) {
super()

if (!($module instanceof HTMLElement)) {
return this
}

Expand Down
Loading