[SPIKE] Use TypeScript for the Accordion's defaults immutability #5493
+33
−7
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
`Object.freeze` only does a shallow freeze of the object, meaning we have nothing checking our code against something setting one of the keys of `i18n`. The `ReadonlyDeep` type from `type-fest` allows TypeScript to check that even deep properties cannot be updated by our code.
govuk-design-system-ci
had a problem deploying
to
govuk-frontend-pr-5493
📋 StatsFile sizes
Modules
View stats and visualisations on the review app Action run for 3042948 |
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 64b3fda7d..a4a31eec1 100644
--- a/packages/govuk-frontend/dist/govuk/govuk-frontend.min.js
+++ b/packages/govuk-frontend/dist/govuk/govuk-frontend.min.js
@@ -224,6 +224,7 @@ class I18n {
}
}
}
+var t;
I18n.pluralRulesMap = {
arabic: ["ar"],
chinese: ["my", "zh", "id", "ja", "jv", "ko", "ms", "th", "vi"],
@@ -377,7 +378,7 @@ class Accordion extends GOVUKFrontendComponentConfigurable {
return t.classList.add("govuk-visually-hidden", this.sectionHeadingDividerClass), t.textContent = ", ", t
}
}
-Accordion.moduleName = "govuk-accordion", Accordion.defaults = Object.freeze({
+Accordion.moduleName = "govuk-accordion", Accordion.defaults = {
i18n: {
hideAllSections: "Hide all sections",
hideSection: "Hide",
@@ -387,7 +388,7 @@ Accordion.moduleName = "govuk-accordion", Accordion.defaults = Object.freeze({
showSectionAriaLabel: "Show this section"
},
rememberExpanded: !0
-}), Accordion.schema = Object.freeze({
+}, Accordion.schema = Object.freeze({
properties: {
i18n: {
type: "object"
@@ -396,7 +397,7 @@ Accordion.moduleName = "govuk-accordion", Accordion.defaults = Object.freeze({
type: "boolean"
}
}
-});
+}), Accordion.defaults.rememberExpanded = !1, null != (t = Accordion.defaults.i18n) && t.hideSection && (Accordion.defaults.i18n.hideSection = "Some new string");
class Button extends GOVUKFrontendComponentConfigurable {
constructor(t, e = {}) {
super(t, e), this.debounceFormSubmitTimer = null, this.$root.addEventListener("keydown", (t => this.handleKeyDown(t))), this.$root.addEventListener("click", (t => this.debounce(t)))
Action run for 3042948 |
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 956e54478..224d68eae 100644
--- a/packages/govuk-frontend/dist/govuk/all.bundle.js
+++ b/packages/govuk-frontend/dist/govuk/all.bundle.js
@@ -530,6 +530,8 @@
}
};
+ var _Accordion$defaults$i;
+
/**
* Accordion component
*
@@ -810,6 +812,32 @@
return $punctuationEl;
}
}
+ Accordion.moduleName = 'govuk-accordion';
+ Accordion.defaults = {
+ i18n: {
+ hideAllSections: 'Hide all sections',
+ hideSection: 'Hide',
+ hideSectionAriaLabel: 'Hide this section',
+ showAllSections: 'Show all sections',
+ showSection: 'Show',
+ showSectionAriaLabel: 'Show this section'
+ },
+ rememberExpanded: true
+ };
+ Accordion.schema = Object.freeze({
+ properties: {
+ i18n: {
+ type: 'object'
+ },
+ rememberExpanded: {
+ type: 'boolean'
+ }
+ }
+ });
+ Accordion.defaults.rememberExpanded = false;
+ if ((_Accordion$defaults$i = Accordion.defaults.i18n) != null && _Accordion$defaults$i.hideSection) {
+ Accordion.defaults.i18n.hideSection = 'Some new string';
+ }
/**
* Accordion config
@@ -847,28 +875,11 @@
/**
* @typedef {import('../../common/configuration.mjs').Schema} Schema
*/
- Accordion.moduleName = 'govuk-accordion';
- Accordion.defaults = Object.freeze({
- i18n: {
- hideAllSections: 'Hide all sections',
- hideSection: 'Hide',
- hideSectionAriaLabel: 'Hide this section',
- showAllSections: 'Show all sections',
- showSection: 'Show',
- showSectionAriaLabel: 'Show this section'
- },
- rememberExpanded: true
- });
- Accordion.schema = Object.freeze({
- properties: {
- i18n: {
- type: 'object'
- },
- rememberExpanded: {
- type: 'boolean'
- }
- }
- });
+
+ /**
+ * @template T
+ * @typedef {import('type-fest').ReadonlyDeep<T>} ReadonlyDeep<T>
+ */
const DEBOUNCE_TIMEOUT_IN_SECONDS = 1;
diff --git a/packages/govuk-frontend/dist/govuk/all.bundle.mjs b/packages/govuk-frontend/dist/govuk/all.bundle.mjs
index b56f12f83..94fe8a0d6 100644
--- a/packages/govuk-frontend/dist/govuk/all.bundle.mjs
+++ b/packages/govuk-frontend/dist/govuk/all.bundle.mjs
@@ -524,6 +524,8 @@ I18n.pluralRules = {
}
};
+var _Accordion$defaults$i;
+
/**
* Accordion component
*
@@ -804,6 +806,32 @@ class Accordion extends GOVUKFrontendComponentConfigurable {
return $punctuationEl;
}
}
+Accordion.moduleName = 'govuk-accordion';
+Accordion.defaults = {
+ i18n: {
+ hideAllSections: 'Hide all sections',
+ hideSection: 'Hide',
+ hideSectionAriaLabel: 'Hide this section',
+ showAllSections: 'Show all sections',
+ showSection: 'Show',
+ showSectionAriaLabel: 'Show this section'
+ },
+ rememberExpanded: true
+};
+Accordion.schema = Object.freeze({
+ properties: {
+ i18n: {
+ type: 'object'
+ },
+ rememberExpanded: {
+ type: 'boolean'
+ }
+ }
+});
+Accordion.defaults.rememberExpanded = false;
+if ((_Accordion$defaults$i = Accordion.defaults.i18n) != null && _Accordion$defaults$i.hideSection) {
+ Accordion.defaults.i18n.hideSection = 'Some new string';
+}
/**
* Accordion config
@@ -841,28 +869,11 @@ class Accordion extends GOVUKFrontendComponentConfigurable {
/**
* @typedef {import('../../common/configuration.mjs').Schema} Schema
*/
-Accordion.moduleName = 'govuk-accordion';
-Accordion.defaults = Object.freeze({
- i18n: {
- hideAllSections: 'Hide all sections',
- hideSection: 'Hide',
- hideSectionAriaLabel: 'Hide this section',
- showAllSections: 'Show all sections',
- showSection: 'Show',
- showSectionAriaLabel: 'Show this section'
- },
- rememberExpanded: true
-});
-Accordion.schema = Object.freeze({
- properties: {
- i18n: {
- type: 'object'
- },
- rememberExpanded: {
- type: 'boolean'
- }
- }
-});
+
+/**
+ * @template T
+ * @typedef {import('type-fest').ReadonlyDeep<T>} ReadonlyDeep<T>
+ */
const DEBOUNCE_TIMEOUT_IN_SECONDS = 1;
diff --git a/packages/govuk-frontend/dist/govuk/components/accordion/accordion.bundle.js b/packages/govuk-frontend/dist/govuk/components/accordion/accordion.bundle.js
index 7eb946c6d..e2f49db2c 100644
--- a/packages/govuk-frontend/dist/govuk/components/accordion/accordion.bundle.js
+++ b/packages/govuk-frontend/dist/govuk/components/accordion/accordion.bundle.js
@@ -470,6 +470,8 @@
}
};
+ var _Accordion$defaults$i;
+
/**
* Accordion component
*
@@ -750,6 +752,32 @@
return $punctuationEl;
}
}
+ Accordion.moduleName = 'govuk-accordion';
+ Accordion.defaults = {
+ i18n: {
+ hideAllSections: 'Hide all sections',
+ hideSection: 'Hide',
+ hideSectionAriaLabel: 'Hide this section',
+ showAllSections: 'Show all sections',
+ showSection: 'Show',
+ showSectionAriaLabel: 'Show this section'
+ },
+ rememberExpanded: true
+ };
+ Accordion.schema = Object.freeze({
+ properties: {
+ i18n: {
+ type: 'object'
+ },
+ rememberExpanded: {
+ type: 'boolean'
+ }
+ }
+ });
+ Accordion.defaults.rememberExpanded = false;
+ if ((_Accordion$defaults$i = Accordion.defaults.i18n) != null && _Accordion$defaults$i.hideSection) {
+ Accordion.defaults.i18n.hideSection = 'Some new string';
+ }
/**
* Accordion config
@@ -787,28 +815,11 @@
/**
* @typedef {import('../../common/configuration.mjs').Schema} Schema
*/
- Accordion.moduleName = 'govuk-accordion';
- Accordion.defaults = Object.freeze({
- i18n: {
- hideAllSections: 'Hide all sections',
- hideSection: 'Hide',
- hideSectionAriaLabel: 'Hide this section',
- showAllSections: 'Show all sections',
- showSection: 'Show',
- showSectionAriaLabel: 'Show this section'
- },
- rememberExpanded: true
- });
- Accordion.schema = Object.freeze({
- properties: {
- i18n: {
- type: 'object'
- },
- rememberExpanded: {
- type: 'boolean'
- }
- }
- });
+
+ /**
+ * @template T
+ * @typedef {import('type-fest').ReadonlyDeep<T>} ReadonlyDeep<T>
+ */
exports.Accordion = Accordion;
diff --git a/packages/govuk-frontend/dist/govuk/components/accordion/accordion.bundle.mjs b/packages/govuk-frontend/dist/govuk/components/accordion/accordion.bundle.mjs
index dbb20e68a..486d050a3 100644
--- a/packages/govuk-frontend/dist/govuk/components/accordion/accordion.bundle.mjs
+++ b/packages/govuk-frontend/dist/govuk/components/accordion/accordion.bundle.mjs
@@ -464,6 +464,8 @@ I18n.pluralRules = {
}
};
+var _Accordion$defaults$i;
+
/**
* Accordion component
*
@@ -744,6 +746,32 @@ class Accordion extends GOVUKFrontendComponentConfigurable {
return $punctuationEl;
}
}
+Accordion.moduleName = 'govuk-accordion';
+Accordion.defaults = {
+ i18n: {
+ hideAllSections: 'Hide all sections',
+ hideSection: 'Hide',
+ hideSectionAriaLabel: 'Hide this section',
+ showAllSections: 'Show all sections',
+ showSection: 'Show',
+ showSectionAriaLabel: 'Show this section'
+ },
+ rememberExpanded: true
+};
+Accordion.schema = Object.freeze({
+ properties: {
+ i18n: {
+ type: 'object'
+ },
+ rememberExpanded: {
+ type: 'boolean'
+ }
+ }
+});
+Accordion.defaults.rememberExpanded = false;
+if ((_Accordion$defaults$i = Accordion.defaults.i18n) != null && _Accordion$defaults$i.hideSection) {
+ Accordion.defaults.i18n.hideSection = 'Some new string';
+}
/**
* Accordion config
@@ -781,28 +809,11 @@ class Accordion extends GOVUKFrontendComponentConfigurable {
/**
* @typedef {import('../../common/configuration.mjs').Schema} Schema
*/
-Accordion.moduleName = 'govuk-accordion';
-Accordion.defaults = Object.freeze({
- i18n: {
- hideAllSections: 'Hide all sections',
- hideSection: 'Hide',
- hideSectionAriaLabel: 'Hide this section',
- showAllSections: 'Show all sections',
- showSection: 'Show',
- showSectionAriaLabel: 'Show this section'
- },
- rememberExpanded: true
-});
-Accordion.schema = Object.freeze({
- properties: {
- i18n: {
- type: 'object'
- },
- rememberExpanded: {
- type: 'boolean'
- }
- }
-});
+
+/**
+ * @template T
+ * @typedef {import('type-fest').ReadonlyDeep<T>} ReadonlyDeep<T>
+ */
export { Accordion };
//# sourceMappingURL=accordion.bundle.mjs.map
diff --git a/packages/govuk-frontend/dist/govuk/components/accordion/accordion.mjs b/packages/govuk-frontend/dist/govuk/components/accordion/accordion.mjs
index 4cd678923..9c66c463d 100644
--- a/packages/govuk-frontend/dist/govuk/components/accordion/accordion.mjs
+++ b/packages/govuk-frontend/dist/govuk/components/accordion/accordion.mjs
@@ -2,6 +2,8 @@ import { GOVUKFrontendComponentConfigurable } from '../../common/configuration.m
import { ElementError } from '../../errors/index.mjs';
import { I18n } from '../../i18n.mjs';
+var _Accordion$defaults$i;
+
/**
* Accordion component
*
@@ -282,6 +284,32 @@ class Accordion extends GOVUKFrontendComponentConfigurable {
return $punctuationEl;
}
}
+Accordion.moduleName = 'govuk-accordion';
+Accordion.defaults = {
+ i18n: {
+ hideAllSections: 'Hide all sections',
+ hideSection: 'Hide',
+ hideSectionAriaLabel: 'Hide this section',
+ showAllSections: 'Show all sections',
+ showSection: 'Show',
+ showSectionAriaLabel: 'Show this section'
+ },
+ rememberExpanded: true
+};
+Accordion.schema = Object.freeze({
+ properties: {
+ i18n: {
+ type: 'object'
+ },
+ rememberExpanded: {
+ type: 'boolean'
+ }
+ }
+});
+Accordion.defaults.rememberExpanded = false;
+if ((_Accordion$defaults$i = Accordion.defaults.i18n) != null && _Accordion$defaults$i.hideSection) {
+ Accordion.defaults.i18n.hideSection = 'Some new string';
+}
/**
* Accordion config
@@ -319,28 +347,11 @@ class Accordion extends GOVUKFrontendComponentConfigurable {
/**
* @typedef {import('../../common/configuration.mjs').Schema} Schema
*/
-Accordion.moduleName = 'govuk-accordion';
-Accordion.defaults = Object.freeze({
- i18n: {
- hideAllSections: 'Hide all sections',
- hideSection: 'Hide',
- hideSectionAriaLabel: 'Hide this section',
- showAllSections: 'Show all sections',
- showSection: 'Show',
- showSectionAriaLabel: 'Show this section'
- },
- rememberExpanded: true
-});
-Accordion.schema = Object.freeze({
- properties: {
- i18n: {
- type: 'object'
- },
- rememberExpanded: {
- type: 'boolean'
- }
- }
-});
+
+/**
+ * @template T
+ * @typedef {import('type-fest').ReadonlyDeep<T>} ReadonlyDeep<T>
+ */
export { Accordion };
//# sourceMappingURL=accordion.mjs.map
Action run for 3042948 |
|
This feels like a sensible approach - I think we definitely want to make sure we're testing for read-onliness internally. I'm less worried about the impact on |
I know your comment now dates from a while back, but do you remember what you meant by this? Was this about having checks when our code is built or having checks happening when code runs in the browser? |
|
For the former, TypeScript is taking care of checking that 🥳 For the later, we could add some defensiveness (for example, wrapping the default config in a |
|
Pretty sure I was just reacting positively to this spike! I think we had some discussions about whether, if we dropped the Object.freeze, we needed to do anything else to enforce read-onliness, so this was just me saying doing it with typechecking is fine! |
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.
Object.freezeonly does a shallow freeze of the object, meaning we have nothing checking our code against something setting one of the keys ofi18n. TheReadonlyDeep<T>type fromtype-festallows TypeScript to check that even deep properties cannot be updated by our code.This spike illustrates how we would use the
ReadonlyDeep<t>type, using theAccordioncomponent and building on the fact that we define the types for each component's configuration as<COMPONENT_NAME>Config.Important
Tests are expected to fail on this spike to demonstrate that TypeScript does indeed pick up on nested properties of the
Accordion'sdefaultsgetting modified when they shouldn'tThoughts
Removing
Object.freezeBecause
Object.freezeonly does a shallow freeze, it provide a false sense of safety that deep keys are immutable if you're not aware of its shallowness. Implementing a function to deep freeze an object recursively would mean extra code shipped to the browser for the sake of preventing users' code from modifying a component's defaults.Leaving these defaults unprotected by removing
Object.freezedoesn't seem a massive risk. If anything it allows an undocumented way to set defaults for all the components of a given class, while allowing multiple calls tocreateAllorinitAllto override those defaults with specific configurations for given pages or parts of pages. And it shaves a little bit of code which won't be sent to the browser.Keeping things readonly in our code
This doesn't mean we cannot check that our code doesn't inadvertently try to update one of the properties in
defaults. TypeScript'sReadonly<T>type unfortunately works shallowly, just likeObject.freeze. However, thetype-festpackage provides aReadonlyDeep<T>type that ensures nested objects/Maps/Sets/Arrays are also readonly (and is heavily tested).The
type-festpackage doesn't add to our own dependencies as type checking happens at build time, so it only needs to be adevDependency. However, it'll have impact on the@types/govuk-frontendpackage, for which it's become a dependency (unless there's a system for vendoring it).