Add onError to createAll
#5252
Merged
Add onError to createAll
#5252
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
govuk-design-system-ci
temporarily deployed
to
govuk-frontend-pr-5252
📋 StatsFile sizes
Modules
View stats and visualisations on the review app Action run for b771200 |
JavaScript changes to npm packagediff --git a/packages/govuk-frontend/dist/govuk/govuk-frontend.min.js b/packages/govuk-frontend/dist/govuk/govuk-frontend.min.js
index 69492a902..dd7574fa1 100644
--- a/packages/govuk-frontend/dist/govuk/govuk-frontend.min.js
+++ b/packages/govuk-frontend/dist/govuk/govuk-frontend.min.js
@@ -1104,15 +1104,26 @@ function initAll(e) {
}))
}
-function createAll(e, t, n = document) {
- const i = n.querySelectorAll(`[data-module="${e.moduleName}"]`);
- return isSupported() ? Array.from(i).map((n => {
+function createAll(e, t, n) {
+ let i, s = document;
+ var o;
+ "object" == typeof n && (s = null != (o = n.scope) ? o : s, i = n.onError);
+ "function" == typeof n && (i = n), n instanceof HTMLElement && (s = n);
+ const r = s.querySelectorAll(`[data-module="${e.moduleName}"]`);
+ return isSupported() ? Array.from(r).map((n => {
try {
return "defaults" in e && void 0 !== t ? new e(n, t) : new e(n)
- } catch (i) {
- return console.log(i), null
+ } catch (s) {
+ return i && s instanceof Error ? i(s, {
+ element: n,
+ component: e,
+ config: t
+ }) : console.log(s), null
}
- })).filter(Boolean) : (console.log(new SupportError), [])
+ })).filter(Boolean) : (i ? i(new SupportError, {
+ component: e,
+ config: t
+ }) : console.log(new SupportError), [])
}
Tabs.moduleName = "govuk-tabs";
export {
Action run for b771200 |
Other changes to npm packagediff --git a/packages/govuk-frontend/dist/govuk/all.bundle.js b/packages/govuk-frontend/dist/govuk/all.bundle.js
index 3b86ffe36..057ec0db2 100644
--- a/packages/govuk-frontend/dist/govuk/all.bundle.js
+++ b/packages/govuk-frontend/dist/govuk/all.bundle.js
@@ -2383,21 +2383,50 @@
*
* @template {CompatibleClass} T
* @param {T} Component - class of the component to create
- * @param {T["defaults"]} [config] - config for the component
- * @param {Element|Document} [$scope] - scope of the document to search within
+ * @param {T["defaults"]} [config] - Config supplied to component
+ * @param {OnErrorCallback<T> | Element | Document | CreateAllOptions<T> } [createAllOptions] - options for createAll including scope of the document to search within and callback function if error throw by component on init
* @returns {Array<InstanceType<T>>} - array of instantiated components
*/
- function createAll(Component, config, $scope = document) {
+ function createAll(Component, config, createAllOptions) {
+ let $scope = document;
+ let onError;
+ if (typeof createAllOptions === 'object') {
+ var _createAllOptions$sco;
+ createAllOptions = createAllOptions;
+ $scope = (_createAllOptions$sco = createAllOptions.scope) != null ? _createAllOptions$sco : $scope;
+ onError = createAllOptions.onError;
+ }
+ if (typeof createAllOptions === 'function') {
+ onError = createAllOptions;
+ }
+ if (createAllOptions instanceof HTMLElement) {
+ $scope = createAllOptions;
+ }
const $elements = $scope.querySelectorAll(`[data-module="${Component.moduleName}"]`);
if (!isSupported()) {
- console.log(new SupportError());
+ if (onError) {
+ onError(new SupportError(), {
+ component: Component,
+ config
+ });
+ } else {
+ console.log(new SupportError());
+ }
return [];
}
return Array.from($elements).map($element => {
try {
return 'defaults' in Component && typeof config !== 'undefined' ? new Component($element, config) : new Component($element);
} catch (error) {
- console.log(error);
+ if (onError && error instanceof Error) {
+ onError(error, {
+ element: $element,
+ component: Component,
+ config
+ });
+ } else {
+ console.log(error);
+ }
return null;
}
}).filter(Boolean);
@@ -2436,6 +2465,25 @@
*
* @typedef {keyof Config} ConfigKey
*/
+ /**
+ * @template {CompatibleClass} T
+ * @typedef {object} ErrorContext
+ * @property {Element} [element] - Element used for component module initialisation
+ * @property {T} component - Class of component
+ * @property {T["defaults"]} config - Config supplied to component
+ */
+ /**
+ * @template {CompatibleClass} T
+ * @callback OnErrorCallback
+ * @param {Error} error - Thrown error
+ * @param {ErrorContext<T>} context - Object containing the element, component class and configuration
+ */
+ /**
+ * @template {CompatibleClass} T
+ * @typedef {object} CreateAllOptions
+ * @property {Element | Document} [scope] - scope of the document to search within
+ * @property {OnErrorCallback<T>} [onError] - callback function if error throw by component on init
+ */
exports.Accordion = Accordion;
exports.Button = Button;
diff --git a/packages/govuk-frontend/dist/govuk/all.bundle.mjs b/packages/govuk-frontend/dist/govuk/all.bundle.mjs
index 8725b363d..15a7feada 100644
--- a/packages/govuk-frontend/dist/govuk/all.bundle.mjs
+++ b/packages/govuk-frontend/dist/govuk/all.bundle.mjs
@@ -2377,21 +2377,50 @@ function initAll(config) {
*
* @template {CompatibleClass} T
* @param {T} Component - class of the component to create
- * @param {T["defaults"]} [config] - config for the component
- * @param {Element|Document} [$scope] - scope of the document to search within
+ * @param {T["defaults"]} [config] - Config supplied to component
+ * @param {OnErrorCallback<T> | Element | Document | CreateAllOptions<T> } [createAllOptions] - options for createAll including scope of the document to search within and callback function if error throw by component on init
* @returns {Array<InstanceType<T>>} - array of instantiated components
*/
-function createAll(Component, config, $scope = document) {
+function createAll(Component, config, createAllOptions) {
+ let $scope = document;
+ let onError;
+ if (typeof createAllOptions === 'object') {
+ var _createAllOptions$sco;
+ createAllOptions = createAllOptions;
+ $scope = (_createAllOptions$sco = createAllOptions.scope) != null ? _createAllOptions$sco : $scope;
+ onError = createAllOptions.onError;
+ }
+ if (typeof createAllOptions === 'function') {
+ onError = createAllOptions;
+ }
+ if (createAllOptions instanceof HTMLElement) {
+ $scope = createAllOptions;
+ }
const $elements = $scope.querySelectorAll(`[data-module="${Component.moduleName}"]`);
if (!isSupported()) {
- console.log(new SupportError());
+ if (onError) {
+ onError(new SupportError(), {
+ component: Component,
+ config
+ });
+ } else {
+ console.log(new SupportError());
+ }
return [];
}
return Array.from($elements).map($element => {
try {
return 'defaults' in Component && typeof config !== 'undefined' ? new Component($element, config) : new Component($element);
} catch (error) {
- console.log(error);
+ if (onError && error instanceof Error) {
+ onError(error, {
+ element: $element,
+ component: Component,
+ config
+ });
+ } else {
+ console.log(error);
+ }
return null;
}
}).filter(Boolean);
@@ -2430,6 +2459,25 @@ function createAll(Component, config, $scope = document) {
*
* @typedef {keyof Config} ConfigKey
*/
+/**
+ * @template {CompatibleClass} T
+ * @typedef {object} ErrorContext
+ * @property {Element} [element] - Element used for component module initialisation
+ * @property {T} component - Class of component
+ * @property {T["defaults"]} config - Config supplied to component
+ */
+/**
+ * @template {CompatibleClass} T
+ * @callback OnErrorCallback
+ * @param {Error} error - Thrown error
+ * @param {ErrorContext<T>} context - Object containing the element, component class and configuration
+ */
+/**
+ * @template {CompatibleClass} T
+ * @typedef {object} CreateAllOptions
+ * @property {Element | Document} [scope] - scope of the document to search within
+ * @property {OnErrorCallback<T>} [onError] - callback function if error throw by component on init
+ */
export { Accordion, Button, CharacterCount, Checkboxes, ErrorSummary, ExitThisPage, Header, NotificationBanner, PasswordInput, Radios, SkipLink, Tabs, createAll, initAll, isSupported, version };
//# sourceMappingURL=all.bundle.mjs.map
diff --git a/packages/govuk-frontend/dist/govuk/init.mjs b/packages/govuk-frontend/dist/govuk/init.mjs
index c8b4ab490..2de2329d5 100644
--- a/packages/govuk-frontend/dist/govuk/init.mjs
+++ b/packages/govuk-frontend/dist/govuk/init.mjs
@@ -46,21 +46,50 @@ function initAll(config) {
*
* @template {CompatibleClass} T
* @param {T} Component - class of the component to create
- * @param {T["defaults"]} [config] - config for the component
- * @param {Element|Document} [$scope] - scope of the document to search within
+ * @param {T["defaults"]} [config] - Config supplied to component
+ * @param {OnErrorCallback<T> | Element | Document | CreateAllOptions<T> } [createAllOptions] - options for createAll including scope of the document to search within and callback function if error throw by component on init
* @returns {Array<InstanceType<T>>} - array of instantiated components
*/
-function createAll(Component, config, $scope = document) {
+function createAll(Component, config, createAllOptions) {
+ let $scope = document;
+ let onError;
+ if (typeof createAllOptions === 'object') {
+ var _createAllOptions$sco;
+ createAllOptions = createAllOptions;
+ $scope = (_createAllOptions$sco = createAllOptions.scope) != null ? _createAllOptions$sco : $scope;
+ onError = createAllOptions.onError;
+ }
+ if (typeof createAllOptions === 'function') {
+ onError = createAllOptions;
+ }
+ if (createAllOptions instanceof HTMLElement) {
+ $scope = createAllOptions;
+ }
const $elements = $scope.querySelectorAll(`[data-module="${Component.moduleName}"]`);
if (!isSupported()) {
- console.log(new SupportError());
+ if (onError) {
+ onError(new SupportError(), {
+ component: Component,
+ config
+ });
+ } else {
+ console.log(new SupportError());
+ }
return [];
}
return Array.from($elements).map($element => {
try {
return 'defaults' in Component && typeof config !== 'undefined' ? new Component($element, config) : new Component($element);
} catch (error) {
- console.log(error);
+ if (onError && error instanceof Error) {
+ onError(error, {
+ element: $element,
+ component: Component,
+ config
+ });
+ } else {
+ console.log(error);
+ }
return null;
}
}).filter(Boolean);
@@ -99,6 +128,25 @@ function createAll(Component, config, $scope = document) {
*
* @typedef {keyof Config} ConfigKey
*/
+/**
+ * @template {CompatibleClass} T
+ * @typedef {object} ErrorContext
+ * @property {Element} [element] - Element used for component module initialisation
+ * @property {T} component - Class of component
+ * @property {T["defaults"]} config - Config supplied to component
+ */
+/**
+ * @template {CompatibleClass} T
+ * @callback OnErrorCallback
+ * @param {Error} error - Thrown error
+ * @param {ErrorContext<T>} context - Object containing the element, component class and configuration
+ */
+/**
+ * @template {CompatibleClass} T
+ * @typedef {object} CreateAllOptions
+ * @property {Element | Document} [scope] - scope of the document to search within
+ * @property {OnErrorCallback<T>} [onError] - callback function if error throw by component on init
+ */
export { createAll, initAll };
//# sourceMappingURL=init.mjs.map
Action run for b771200 |
patrickpatrickpatrick
force-pushed
the
create-all-error
branch
from
a060a8c
to
e03e380
Compare
govuk-design-system-ci
temporarily deployed
to
govuk-frontend-pr-5252
| * @returns {Array<InstanceType<T>>} - array of instantiated components | ||
| */ | ||
| function createAll(Component, config, $scope = document) { | ||
| function createAll(Component, config, $scope = document, errorCallback) { |
There was a problem hiding this comment.
suggestion As discussed during our call, let's try to limit the number of arguments to 3 and regroup the scope and error callback in a param that can be either:
- an
Element|Documentrepresenting the scope - a
Functionrepresenting the error callback - an
{scope: Element|Document, errorCallback: Function}if both are needed.
There was a problem hiding this comment.
Let's also align with the on<NAME> format that's common in the DOM API.
patrickpatrickpatrick
force-pushed
the
create-all-error
branch
from
e03e380
to
2c166b1
Compare
govuk-design-system-ci
temporarily deployed
to
govuk-frontend-pr-5252
patrickpatrickpatrick
force-pushed
the
create-all-error
branch
from
2c166b1
to
c96a027
Compare
govuk-design-system-ci
temporarily deployed
to
govuk-frontend-pr-5252
patrickpatrickpatrick
force-pushed
the
create-all-error
branch
from
c96a027
to
200617d
Compare
govuk-design-system-ci
temporarily deployed
to
govuk-frontend-pr-5252
patrickpatrickpatrick
force-pushed
the
create-all-error
branch
from
200617d
to
15daa86
Compare
govuk-design-system-ci
temporarily deployed
to
govuk-frontend-pr-5252
Comment on lines
91
to
95
| $scope = | ||
| /** @type {createAllOptions<Component>} */ (createAllOptions).scope ?? | ||
| $scope | ||
| onError = /** @type {createAllOptions<Component>} */ (createAllOptions) | ||
| .onError |
There was a problem hiding this comment.
suggestion Would it work to do something like the following to put the casting in its own line and make things a little more readable by separating the assignments of the options from the casting?
Suggested change
| $scope = | |
| /** @type {createAllOptions<Component>} */ (createAllOptions).scope ?? | |
| $scope | |
| onError = /** @type {createAllOptions<Component>} */ (createAllOptions) | |
| .onError | |
| createAllOptions = /** @type {createAllOptions<Component>} */ createAllOptions | |
| $scope = createAllOptions.scope ?? $scope | |
| onError = createAllOptions.onError |
There was a problem hiding this comment.
That makes sense, I have disable eslint rule "no-self-assign" but it does look neater
Comment on lines
126
to
127
| if (onError) { | ||
| onError(/** @type {Error} */ (error), { |
There was a problem hiding this comment.
suggestion We've usually ran typeof/instanceof calls rather than casting to make typescript aware of types. Would the following work for that?
Suggested change
| if (onError) { | |
| onError(/** @type {Error} */ (error), { | |
| if (onError && error instanceof Error) { | |
| onError(error, { |
There was a problem hiding this comment.
Yeah, that seems to work here :)
Comment on lines
57
to
69
| /** | ||
| * @template {CompatibleClass} T | ||
| * @callback onErrorCallback | ||
| * @param {Error} error - Thrown error | ||
| * @param {ErrorContext<T>} context - Object containing the element, component class and configuration | ||
| */ | ||
|
|
||
| /** | ||
| * @template {CompatibleClass} T | ||
| * @typedef {object} createAllOptions | ||
| * @property {Element | Document} [scope] - scope of the document to search within | ||
| * @property {onErrorCallback<T>} [onError] - callback function if error throw by component on init | ||
| */ |
There was a problem hiding this comment.
question We've tended to put our types at the bottom of the files across the project, is there something forcing these to be early on? If not, would be good to align with the rest of the files for consistency.
suggestion We've generally been PascalCasing the types rather than camelCasing them.
There was a problem hiding this comment.
Mostly just found it easier to keep track of them being nearer where I was typing but I'll move them down again.
|
@patrickpatrickpatrick I've changed the PR base to What are your thoughts on whether the check should also trigger a call to |
patrickpatrickpatrick
force-pushed
the
create-all-error
branch
from
15daa86
to
9f7bc15
Compare
govuk-design-system-ci
temporarily deployed
to
govuk-frontend-pr-5252
patrickpatrickpatrick
force-pushed
the
create-all-error
branch
from
9f7bc15
to
2d321e0
Compare
govuk-design-system-ci
temporarily deployed
to
govuk-frontend-pr-5252
- Add support for `onError` callback in `createAll` which is called if error occurs on component initialisation. - New parameter for `createAll`, `createAllOptions` which allows user to specify a `scope`, `onError` or an object that contains both. - `isSupported` will execute `onError` callback if specified - New tests added for `onError` callback and `createAllOptions`.
patrickpatrickpatrick
force-pushed
the
create-all-error
branch
from
2d321e0
to
b771200
Compare
govuk-design-system-ci
temporarily deployed
to
govuk-frontend-pr-5252
That makes sense, have added this! |
romaricpascal
approved these changes
Aug 27, 2024
4 tasks
romaricpascal
changed the title
errorCallback to createAllonError to createAll
Merged
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
What
onErrorcallback increateAllwhich is called if error occurs on component initialisation.createAll,createAllOptionswhich allows user to specify ascope,onErroror an object that contains both.isSupportedwill executeonErrorcallback if specifiedonErrorcallback andcreateAllOptions.Why
Provide better support for defining custom components by allowing users to execute custom callback if the components fail to initialise.
Fixes #5212