-
Notifications
You must be signed in to change notification settings - Fork 334
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
[SPIKE][DO NOT MERGE] Create IE-compatible configuration merging function #2810
Conversation
src/govuk/common.mjs
Outdated
for (var i = 0; i < arguments.length; i++) { | ||
var obj = flattenObject(arguments[i]) | ||
for (var key in obj) { | ||
if (Object.prototype.hasOwnProperty.call(obj, key)) { | ||
configObject[key] = obj[key] | ||
} | ||
} | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I like the flexibility of getModuleConfig
being able to take any number of objects, but it does mean that each component is 'responsible' for the order of precedence when it comes to merging configs.
I'm wondering if it would make sense to 'codify' the order by giving getModuleConfig
named arguments e.g.
export function getModuleConfig (defaultOptions, initOptions, datasetOptions) {
I appreciate that the calls to the function from each component would look exactly the same as JavaScript doesn't have named arguments, but it makes it explicit and e.g. any sort of IDE autocomplete functionality will hint at the order that things are meant to be passed in.
Thoughts?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think this would also mean we could avoid unnecessarily flattening the dataset object?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
To my mind, there isn't much practical difference between allowing an arbitrary number of objects and codifying specific objects, as either way it depends on the component JS passing them in the correct order to obtain the expected result.
We'd then need to combine them into a loopable array anyway. Albeit, not much complexity, probably something like...
var configArray = [flattenObject(defaultOptions), flattenObject(initOptions), datasetOptions]
for (var i = 0; i < configArray.length; i++) {
var obj = configArray[i]
// …
}
The main downside I see of this approach is that, if config objects are passed in the wrong order, this'll could make a mess—but I think we're probably okay with that given it's an internal API.
Good points on IDE suggestion and flattening dataset though. I'm not against codifying if we think that makes it easier to understand.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Picking up a conversation we've been having on Slack, we've talked about a potential move to having a 'base component' that would take care of setting up config etc that each component can extend as part of the JS work in 5.0.
On the assumption that we are going to go down that route in the future, and therefore we have a future state where the merging of the different configs happens in a single place, I think we can go ahead with the current approach and just consider this as a sort of tech debt.
import '../../vendor/polyfills/Function/prototype/bind.mjs' | ||
import '../../vendor/polyfills/Element/prototype/classList.mjs' | ||
|
||
function Accordion ($module) { | ||
function Accordion ($module, options) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
- We use the terms
options
andconfig
interchangeably – do we intend them as two different things, or can we standardise on one or the other? - Are we accepting config when the object is created or when the
init
function is called? I mainly ask because I've realised I'd been assuming the latter but re-reading the proposal we had it being passed on object creation… Happy to go with the proposal but wondering if it is more consistent with the wayinitAll
is called to do it oninit
? 🤔
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah, we should probably standardise on config
—it feels a bit more specific.
On object vs. init, they feel like different cases to me. There isn't any other way of passing config into initAll
, so we're kind of stuck with what we have there.
For components, doing it on object creation makes more sense to me. Our existing use of init
functions seems like more of a legacy layover from not being able to use native constructors. (I think we've even briefly discussed refactoring out the init
s after we drop IE JS support.)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Makes sense to me 👍🏻
Co-authored-by: Oliver Byford <oliver.byford@digital.cabinet-office.gov.uk>
The team seems to be happy with the state of this. Will close this PR for now. This branch could serve as a starting point for #2699 when we get to that. |
This function and the modified Element.prototype.dataset polyfill are taken from an earlier spike (govuk-frontend PR #2810)
This function and the modified Element.prototype.dataset polyfill are taken from an earlier spike (govuk-frontend PR #2810) Add new helper function to extract config keys by namespace This is used to pull out the i18n-related configuration keys, so that they can be passed into the I18n function. Add data attribute localisation support to Accordion Write unit tests for mergeConfigs and extractConfigByNamespace Amend extractConfigByNamespace to pass unit tests Add Nunjucks parameter documentation Add integration tests for localisation of the accordion Change test -> it Add whitespace control to Yaml values Tweaking whitespace Add data attribute template tests
This matches what we're doing within components after a discussion when we spiked the config merging [1]. [1]: #2810 (comment)
This function and the modified Element.prototype.dataset polyfill are taken from an earlier spike (govuk-frontend PR #2810) Add new helper function to extract config keys by namespace This is used to pull out the i18n-related configuration keys, so that they can be passed into the I18n function. Add data attribute localisation support to Accordion Write unit tests for mergeConfigs and extractConfigByNamespace Amend extractConfigByNamespace to pass unit tests Add Nunjucks parameter documentation Add integration tests for localisation of the accordion Change test -> it Add whitespace control to Yaml values Tweaking whitespace Add data attribute template tests
This matches what we're doing within components after a discussion when we spiked the config merging [1]. [1]: #2810 (comment)
Spike to see if I could make a version of the configuration merging function that works back to IE8.
This exists as a helper function (
getModuleConfig
) that takes an unlimited number of configuration objects, flattens them into a non-nested list of key-value pairs, and merges them into one object. Objects are merged from left to right, in the order they're passed into the function (aka, the last object has the highest priority).Also includes a polyfill for
Element.prototype.dataset
. This is derived from the one formerly served by polyfill.io, with some modifications:Object.prototype.__defineGetter__
andObject.prototype.__defineSetter__
has been added, as these are required by the polyfill but unavailable in IE8, and the existingObject.defineProperty
polyfill doesn't cover these. Instead, a (very hacky) fallback is used if these aren't detected.Seeing it in action
The Accordion component has been modified to accept i18n options and to use the helper function. Currently, no visual changes take place and the resulting configuration object is logged in the console instead (in a way that IE8 won't fall over on!)
A new example has been added to the review app, 'Accordion with localisations', which overrides the
i18n.showSection
andi18n.showAllSections
strings, exhibiting how the default configuration and data-* attribute configuration options get merged and flattened.If all is working as expected, you should get this logged to the console on every browser: