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

[SPIKE][DO NOT MERGE] Create IE-compatible configuration merging function #2810

Closed
wants to merge 5 commits into from

Conversation

querkmachine
Copy link
Member

@querkmachine querkmachine commented Aug 25, 2022

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:

  • The regex used to detect data-* attributes has been altered so that it catches attributes containing full stops (.), as we're using these to namespace i18n attributes.
  • A feature detection for Object.prototype.__defineGetter__ and Object.prototype.__defineSetter__ has been added, as these are required by the polyfill but unavailable in IE8, and the existing Object.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 and i18n.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:

Accordion configuration:
    i18n.hideAllSections: Hide all sections
    i18n.hideSection: Hide <span class="govuk-visually-hidden">this section</span>
    i18n.showAllSections: Show everything
    i18n.showSection: Show<span class="govuk-visually-hidden"> something</span>
    module: govuk-accordion

@govuk-design-system-ci govuk-design-system-ci temporarily deployed to govuk-frontend-pr-2810 August 25, 2022 14:13 Inactive
Comment on lines 83 to 90
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]
}
}
}
Copy link
Contributor

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?

Copy link
Contributor

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?

Copy link
Member Author

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.

Copy link
Contributor

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.

src/govuk/common.mjs Outdated Show resolved Hide resolved
src/govuk/common.mjs Outdated Show resolved Hide resolved
src/govuk/common.mjs Outdated Show resolved Hide resolved
import '../../vendor/polyfills/Function/prototype/bind.mjs'
import '../../vendor/polyfills/Element/prototype/classList.mjs'

function Accordion ($module) {
function Accordion ($module, options) {
Copy link
Contributor

Choose a reason for hiding this comment

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

  1. We use the terms options and config interchangeably – do we intend them as two different things, or can we standardise on one or the other?
  2. 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 way initAll is called to do it on init? 🤔

Copy link
Member Author

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 inits after we drop IE JS support.)

Copy link
Contributor

Choose a reason for hiding this comment

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

Makes sense to me 👍🏻

src/govuk/components/accordion/accordion.mjs Outdated Show resolved Hide resolved
src/govuk/components/accordion/template.njk Outdated Show resolved Hide resolved
src/govuk/common.mjs Outdated Show resolved Hide resolved
Co-authored-by: Oliver Byford <oliver.byford@digital.cabinet-office.gov.uk>
@govuk-design-system-ci govuk-design-system-ci temporarily deployed to govuk-frontend-pr-2810 August 26, 2022 09:18 Inactive
@govuk-design-system-ci govuk-design-system-ci temporarily deployed to govuk-frontend-pr-2810 August 26, 2022 10:05 Inactive
@querkmachine
Copy link
Member Author

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.

querkmachine added a commit that referenced this pull request Aug 30, 2022
This function and the modified Element.prototype.dataset polyfill are taken from an earlier spike (govuk-frontend PR #2810)
querkmachine added a commit that referenced this pull request Sep 1, 2022
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
36degrees added a commit that referenced this pull request Sep 14, 2022
This matches what we're doing within components after a discussion when we spiked the config merging [1].

[1]: #2810 (comment)
querkmachine added a commit that referenced this pull request Oct 17, 2022
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
querkmachine pushed a commit that referenced this pull request Oct 17, 2022
This matches what we're doing within components after a discussion when we spiked the config merging [1].

[1]: #2810 (comment)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants