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

romaricpascal · 2023-07-31T17:20:30Z

This PR updates all our JavaScript components to throw a GOVUKFrontendNotSupportedError if they get instantiated where GOV.UK Frontend is not supported.

Besides updating the components to throw this new error the PR:

introduces a GOVUKFrontendError for our errors to subclass. It allows users to easily separate our custom errors from regular JavaScript errors using instanceof.
updates renderAndInitialise to make running code before the component's initialisation more reliable (had issues where the initialiser – now renamed beforeInitialisation for clarity – didn't execute when run in the middle of the block instantiating the component).

Closes #4009

romaricpascal · 2023-08-01T17:01:13Z

packages/govuk-frontend/src/govuk/all.mjs

@@ -103,6 +104,7 @@ export {
  Checkboxes,
  ErrorSummary,
  ExitThisPage,
+  GOVUKFrontendError,


Only export our base GOVUKFrontendError rather than all error classes with the aim of:

allowing separating our own errors from native ones in bulk, which users can do running instanceof GOVUKFrontendError

but not crowding our export with each error we throw, as users can check the type of error through their name property

Fab work in this PR Romaric

Do we want to discuss if we're ready for errors to be part of our "public" API?

Bit like the I18n class where we chose not to export it

(Gives us freedom to keep iterating without breaking changes)

Up for discussion, I'll revive the thread on Slack where I proposed the approach I implemented here.

Let's default to not exporting it yet, can always add it in another PR

romaricpascal · 2023-08-01T17:04:43Z

packages/govuk-frontend/src/govuk/errors/index.mjs

+ * @abstract
+ */
+export class GOVUKFrontendError extends Error {
+  name = 'GOVUKFrontendError'


Annoying, but necessary. Simply extending Error doesn't give the error a new name, unfortunately.

And going for something automated like this.name = this.constructor.name in GOVUKFrontendError's constructor means the error gets a mangled name once minifier have run over the code and rename the class to say n. While we may prevent the mangling in our files, we don't control how services minify theirs, so hardcoding the name limits risks of having an error with the name of n.

romaricpascal · 2023-08-01T17:06:06Z

docs/contributing/coding-standards/js.md

@@ -133,6 +133,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


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.

colinrotherham · 2023-08-04T09:45:58Z

packages/govuk-frontend/src/govuk/components/accordion/accordion.mjs

@@ -113,7 +114,11 @@ export class Accordion {
   * @param {AccordionConfig} [config] - Accordion config
   */
  constructor ($module, config) {
-    if (!($module instanceof HTMLElement) || !document.body.classList.contains('govuk-frontend-supported')) {
+    if (!document.body.classList.contains('govuk-frontend-supported')) {
+      throw new GOVUKFrontendNotSupportedError()


Just a random thought

Similar to how TypeError isn't NotCorrectTypeError, should we flip out the Not?

Suggested change

throw new GOVUKFrontendNotSupportedError()

throw new GOVUKFrontendSupportError()

Fair, and that makes it shorter, I'll rename 😊

colinrotherham · 2023-08-04T09:46:40Z

packages/govuk-frontend/src/govuk/components/accordion/accordion.test.js

+              renderAndInitialise(page, 'accordion', {
+                params: examples.default,
+                beforeInitialisation () {
+                  document.body.classList.remove('govuk-frontend-supported')


colinrotherham · 2023-08-04T09:49:06Z

packages/govuk-frontend/src/govuk/components/character-count/character-count.test.js

@@ -648,7 +670,7 @@ describe('Character count', () => {
          // Override maxlength to 10
          maxlength: 10
        },
-        initialiser ($module) {
+        beforeInitialisation ($module) {


As it's no longer with us, as a sign of respect for .init() shall we rename this? 😆

Suppose it matches initAll() a bit better too

Suggested change

beforeInitialisation ($module) {

beforeInit ($module) {

We're not calling initAll as a way to intialise and init is gone, so I'm not sure we should go for the abreviated version. Could shorten it to before but I'd rather be verbose in case we add other before moments (though that'd probably be a hint for refactoring).

colinrotherham · 2023-08-04T09:58:32Z

packages/govuk-frontend/src/govuk/components/error-summary/error-summary.mjs

-    // for example if they initialise the component
-    // on their own by directly passing the result
-    // of `document.querySelector`.
-    // To avoid breaking further JavaScript initialisation


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

Thoughts on adding a little disclaimer to every component?

Error throws

Error console shows stack trace

Stack trace uses original source via source maps

Developer clicks to these lines and reads helpful message

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.

colinrotherham · 2023-08-04T10:02:04Z

packages/govuk-frontend/src/govuk/components/globals.test.js

@@ -37,6 +37,7 @@ describe('GOV.UK Frontend', () => {
        'Checkboxes',
        'ErrorSummary',
        'ExitThisPage',
+        'GOVUKFrontendError',


Related to https://github.com/alphagov/govuk-frontend/pull/4030/files#r1284215663

Should we not export our errors in the default export, we'll need to manage Terser mangling:

govuk-frontend/packages/govuk-frontend/rollup.release.config.mjs

Lines 31 to 33 in 91d0a5d

terser({

format: { comments: false },

mangle: { reserved: Object.keys(GOVUKFrontend) },

Can you expand on what we'll need to manage on the Terser side if we don't export the errors internally?

If it's about the error naming after mangling, the hardcoded name property is there to handle the situation (especially as we can't guarantee that library users will take the same step as us towards not mangling the name of the errors/our exports).

Yeah that mangle.reserved option

But if the mangled error class name doesn't appear in stack traces then all good

Added a comment about mangling on this PR: #4075.

The name is clear in the logged message, but mangled in the stacktrace. I think mangling is worth its own discussion, as it may be worth disabling it altogether rather than keep maintaining an evergrowing list of names to not mangle for our min.js file.

And opened #4076 to explore our options.

colinrotherham · 2023-08-04T10:04:22Z

packages/govuk-frontend/src/govuk/components/notification-banner/notification-banner.test.js

-  it('has the correct tabindex attribute to be focused with JavaScript', async () => {
-    await goToComponent(page, 'notification-banner', {
-      exampleName: 'with-type-as-success'
+  describe('when type is set to "success"', () => {


Nice little test refactor with the describe() nesting, mind if we split it into another commit?

No probs, I'll split the extra describes in their own commit to separate them from the implementation of the error throwing.

colinrotherham · 2023-08-04T10:05:27Z

packages/govuk-frontend/src/govuk/components/radios/radios.test.js

-      await page.setJavaScriptEnabled(false)
-    })
+describe('Radios', () => {
+  describe('with conditional reveals', () => {


Same with this one, separate commit?

Jest output is improved too ⭐

colinrotherham · 2023-08-04T10:09:21Z

packages/govuk-frontend/src/govuk/errors/index.mjs

@@ -0,0 +1,33 @@
+/**
+ * A base class for `Error`s thrown by GOV.UK Frontend.


Mind if we keep a short and sweet "title" at the top for any generated docs?

Suggested change

* A base class for `Error`s thrown by GOV.UK Frontend.

* GOV.UK Frontend error

*

* A base class for errors thrown by GOV.UK Frontend.

colinrotherham · 2023-08-04T10:10:18Z

packages/govuk-frontend/src/govuk/errors/index.mjs

+ * to be thrown by our code.
+ *
+ * @example
+ * ```js


Just a reminder to use mjs over js for ES modules code blocks

Can't remember if ESLint is set up for these (like it is for Markdown) but it might matter one day

The example doesn't use anything module specific (import/export) so plain js is fine there 😊

Regarding ESLint, it doesn't seem we've enabled jsdoc/check-examples. We can discuss its enabling in a separate part as it may have some tricky side effects (eg. requiring this class to have its own JSDoc block inside the example).

Yeah not so much the module specific stuff

We tend to use *.mjs as the default now, so some rules like the TypeScript parser are only for *.mjs

govuk-frontend/packages/govuk-frontend/.eslintrc.js

Lines 11 to 13 in e1ef6d2

files: ['src/govuk/**/*.mjs'],

excludedFiles: ['**/*.test.mjs'],

parser: '@typescript-eslint/parser',

colinrotherham · 2023-08-04T10:12:11Z

packages/govuk-frontend/src/govuk/errors/index.mjs

+ *   name = "MissingRootError"
+ * }
+ * ```
+ * @abstract


Ah do we not want to use this class directly, never ever?

Yup, that's the intent. If we don't create subclasses with their own names, that means updating an error's message becomes a breaking change, as the message would be the only way to distinguish one error from another. Subclassing (with a specific name) ensures we can adjust the wording of the messages freely as the name (or class itself should we/when we export it) is the public API.

Worth enforcing this in the constructor too?

export class GOVUKFrontendError extends Error { name = 'GOVUKFrontendError' constructor () { if (this.constructor == GOVUKFrontendError) { throw new Error("GOVUKFrontendError is an abstract class and can't be instantiated"); } } }

Or is errors throwing errors too confusing?

I'm not sure it's worth enforcing the @abstract in code. I'd rather lean towards shipping less code and if we get evidence that people misuse it, clamp down on it rather than be pre-emptively defensive 😊

colinrotherham · 2023-08-04T10:12:33Z

packages/govuk-frontend/src/govuk/errors/index.mjs

+export class GOVUKFrontendNotSupportedError extends GOVUKFrontendError {
+  name = 'GOVUKFrontendNotSupportedError'
+
+  /** */


Suggested change

/** */

Whoops, good spot! 🦅

Oh, no, that wasn't whoops, it's making ESLint happy as there's not much to document on that constructor 🤔 I'll replace with a more explicit disabling of ESLint for that line 😊

colinrotherham · 2023-08-10T16:17:42Z

packages/govuk-frontend/src/govuk/common/index.mjs

+ * @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) {


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

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 🎉

…ialise` - Clarify naming as it's actually code that runs before initialisation rather than does any kind of initialisation of the component itself - Update how the hook is run to be directly called by `page.evaluate`. Having it run as part of the function that did the initialisation didn't run reliably for updating the `govuk-frontend-supported` class, or even other simpler scenarios like logging or throwing. Calling it directly with `page.evaluate` seems to run more reliably.

Adds a couple of extra `describe` to group existing tests in anticipation of new tests sections for the errors

Allows components to indicate they didn't inistantiate because GOV.UK Frontend is not supported

It's the same code for all components and the architecture we're looking to implement so we may as well start introducing it, keeping it internal

Remove stray `.only()` from PR #4030

govuk-design-system-ci temporarily deployed to govuk-frontend-pr-4030 July 31, 2023 17:20 Inactive

romaricpascal force-pushed the component-throw-errors branch from d6e292d to 85c50f6 Compare July 31, 2023 17:39

govuk-design-system-ci temporarily deployed to govuk-frontend-pr-4030 July 31, 2023 17:39 Inactive

romaricpascal force-pushed the component-throw-errors branch from 85c50f6 to 41813df Compare August 1, 2023 13:15

govuk-design-system-ci temporarily deployed to govuk-frontend-pr-4030 August 1, 2023 13:15 Inactive

romaricpascal force-pushed the component-throw-errors branch from 41813df to a533c7e Compare August 1, 2023 13:15

govuk-design-system-ci temporarily deployed to govuk-frontend-pr-4030 August 1, 2023 13:16 Inactive

romaricpascal force-pushed the component-throw-errors branch from a533c7e to 8ecc243 Compare August 1, 2023 14:43

govuk-design-system-ci temporarily deployed to govuk-frontend-pr-4030 August 1, 2023 14:44 Inactive

romaricpascal force-pushed the component-throw-errors branch from 8ecc243 to 9fd3f16 Compare August 1, 2023 15:01

govuk-design-system-ci temporarily deployed to govuk-frontend-pr-4030 August 1, 2023 15:01 Inactive

romaricpascal force-pushed the component-throw-errors branch from 9fd3f16 to 216600f Compare August 1, 2023 15:32

govuk-design-system-ci temporarily deployed to govuk-frontend-pr-4030 August 1, 2023 15:33 Inactive

romaricpascal changed the title ~~[WIP] Throw errors during our components' instantiation~~ Throw error when instantiating components where GOV.UK Frontend is not supported Aug 1, 2023

romaricpascal force-pushed the component-throw-errors branch from 216600f to 439da96 Compare August 1, 2023 15:39

govuk-design-system-ci temporarily deployed to govuk-frontend-pr-4030 August 1, 2023 15:40 Inactive

romaricpascal marked this pull request as ready for review August 1, 2023 15:40

govuk-design-system-ci temporarily deployed to govuk-frontend-pr-4030 August 1, 2023 16:57 Inactive

romaricpascal commented Aug 1, 2023

View reviewed changes

colinrotherham reviewed Aug 4, 2023

View reviewed changes

romaricpascal requested a review from colinrotherham August 10, 2023 13:55

romaricpascal force-pushed the component-throw-errors branch from 8530b5f to dc09a42 Compare August 10, 2023 15:04

govuk-design-system-ci temporarily deployed to govuk-frontend-pr-4030 August 10, 2023 15:04 Inactive

romaricpascal force-pushed the component-throw-errors branch from dc09a42 to d683de5 Compare August 10, 2023 15:38

govuk-design-system-ci temporarily deployed to govuk-frontend-pr-4030 August 10, 2023 15:39 Inactive

romaricpascal force-pushed the component-throw-errors branch from d683de5 to 702d383 Compare August 10, 2023 15:50

govuk-design-system-ci temporarily deployed to govuk-frontend-pr-4030 August 10, 2023 15:50 Inactive

romaricpascal force-pushed the component-throw-errors branch from 702d383 to 7aa2be2 Compare August 10, 2023 15:59

govuk-design-system-ci temporarily deployed to govuk-frontend-pr-4030 August 10, 2023 15:59 Inactive

romaricpascal force-pushed the component-throw-errors branch from 7aa2be2 to 37713cf Compare August 10, 2023 16:04

govuk-design-system-ci temporarily deployed to govuk-frontend-pr-4030 August 10, 2023 16:04 Inactive

colinrotherham reviewed Aug 10, 2023

View reviewed changes

colinrotherham approved these changes Aug 11, 2023

View reviewed changes

romaricpascal added 5 commits August 11, 2023 10:20

Improve tests structure

143dc77

Adds a couple of extra `describe` to group existing tests in anticipation of new tests sections for the errors

Add SupportError

b544d53

Allows components to indicate they didn't inistantiate because GOV.UK Frontend is not supported

Document guidelines for throwing errors

a64c0f5

Refactor validation of GOV.UK Frontend support in a base class

98eaeca

It's the same code for all components and the architecture we're looking to implement so we may as well start introducing it, keeping it internal

romaricpascal force-pushed the component-throw-errors branch from 37713cf to 98eaeca Compare August 11, 2023 09:24

govuk-design-system-ci temporarily deployed to govuk-frontend-pr-4030 August 11, 2023 09:24 Inactive

romaricpascal merged commit dc1d5c9 into main Aug 11, 2023

romaricpascal deleted the component-throw-errors branch August 11, 2023 09:50

romaricpascal mentioned this pull request Aug 11, 2023

Add example demonstrating the SupportError in the browser #4075

Merged

colinrotherham changed the title ~~Throw error when instantiating components where GOV.UK Frontend is not supported~~ Throw SupportError when instantiating components where GOV.UK Frontend is not supported Sep 7, 2023

colinrotherham added a commit that referenced this pull request Sep 8, 2023

Remove stray .only() from PR #4030

072453f

colinrotherham mentioned this pull request Sep 8, 2023

Remove stray .only() from PR #4030 #4182

Merged

colinrotherham added a commit that referenced this pull request Sep 8, 2023

Merge pull request #4182 from alphagov/describe-only

009f077

Remove stray `.only()` from PR #4030

colinrotherham added a commit that referenced this pull request Sep 27, 2023

Remove stray .only() from PR #4030

3466735

colinrotherham added a commit that referenced this pull request Sep 27, 2023

Merge pull request #4182 from alphagov/describe-only

f861aa3

Remove stray `.only()` from PR #4030

romaricpascal mentioned this pull request Dec 8, 2023

Release v5.0.0 #4540

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

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

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

romaricpascal commented Jul 31, 2023 •

edited

Loading

romaricpascal Aug 1, 2023

colinrotherham Aug 4, 2023 •

edited

Loading

romaricpascal Aug 7, 2023

colinrotherham Aug 10, 2023

romaricpascal Aug 1, 2023

romaricpascal Aug 1, 2023

colinrotherham Aug 4, 2023

romaricpascal Aug 7, 2023

colinrotherham Aug 4, 2023

colinrotherham Aug 4, 2023

romaricpascal Aug 10, 2023

colinrotherham Aug 4, 2023

romaricpascal Aug 7, 2023

colinrotherham Aug 4, 2023

romaricpascal Aug 7, 2023

colinrotherham Aug 7, 2023

romaricpascal Aug 11, 2023

romaricpascal Aug 11, 2023

colinrotherham Aug 4, 2023

romaricpascal Aug 7, 2023

colinrotherham Aug 4, 2023

colinrotherham Aug 4, 2023

colinrotherham Aug 4, 2023

romaricpascal Aug 7, 2023

colinrotherham Aug 7, 2023

colinrotherham Aug 4, 2023

romaricpascal Aug 7, 2023

36degrees Aug 7, 2023

romaricpascal Aug 10, 2023

colinrotherham Aug 4, 2023

romaricpascal Aug 7, 2023

romaricpascal Aug 7, 2023

colinrotherham Aug 10, 2023

romaricpascal Aug 10, 2023

		@@ -133,6 +133,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

	throw new GOVUKFrontendNotSupportedError()
	throw new GOVUKFrontendSupportError()

	terser({
	format: { comments: false },
	mangle: { reserved: Object.keys(GOVUKFrontend) },

		@@ -0,0 +1,33 @@
		/**
		* A base class for `Error`s thrown by GOV.UK Frontend.

- * A base class for `Error`s thrown by GOV.UK Frontend.
+ * GOV.UK Frontend error
+ *
+ * A base class for errors thrown by GOV.UK Frontend.

	files: ['src/govuk/*/.mjs'],
	excludedFiles: ['*/.test.mjs'],
	parser: '@typescript-eslint/parser',

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

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

Conversation

romaricpascal commented Jul 31, 2023 • edited Loading

Choose a reason for hiding this comment

colinrotherham Aug 4, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

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

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

romaricpascal commented Jul 31, 2023 •

edited

Loading

colinrotherham Aug 4, 2023 •

edited

Loading