Allow for booleans and numbers to be passed via data attributes

[36degrees]

What

Ensure that config passed by data attributes is typed sensibly for the way we want to use it.

Why

Attributes passed via data attributes will always be strings, but in some cases we want to be able to treat them as booleans or numbers.

For example:

• data-prevent-double-click on buttons which is a boolean
• data-maxlength on the character count which is a number

Who needs to work on this

Developers

Who needs to review this

Developers

Done when

• [ ] Attributes that we expect to be booleans or numbers are typed as such when used as config in a component
• [ ] We have good test coverage of any function(s) required to implement this

[36degrees]

I can think of a couple of different ways we could approach this…

Option 1: If it looks like a duck, and quacks like a duck…

Minic what Bootstrap does by testing for specific cases, e.g. the string 'true' or 'false', or a number that can be parsed into something that's equivalent to its string value.

This is the simpler option but means we're always normalising based on what the user has passed to us, rather than what we actually expect / want.

Option 2: Convert based on the types of the default options

This feels like a 'stricter' option, but is possibly at odds with the way merge configs currently works?

This might also get tricky if we want 'nullable' options – for example the character count currently has maxlength and maxwords, so we'd have to use '0' as the default for both so that the type is correct.

This is roughly how I think this could work:

// purely for example
function convertToType(value, type) {
    switch (type) {
        case 'string':
            return value

        // could potentially cater for 'boolean' data attributes e.g `<button data-prevent-double-click>` with this approach
        case 'boolean':
            if (value === 'true') {
                return true
            } else if (value === 'false') {
                return false
            } else {
                return Boolean(value)
            }

        case 'number':
            return Number(value)

        default:
            return value
    }
}

function normaliseDataset(dataset, defaultConfig) {
    for (var key in dataset) {
        dataset[key] = convertToType(
            dataset[key],
            typeof defaultConfig[key]
        )
    }

    return dataset
}

function MyComponent($module, config) {
    var defaultConfig = {
        points: 100,
        isAwesome: true
    }

    this.config = mergeConfigs(
        defaultConfig,
        config,
        normaliseDataset($module.dataset, defaultConfig)
    )
}

[romaricpascal]

I was thinking of a different approach for this where the component would explicitely declare how it needs parsing for some attributes.

This has the advantage of:

• making explicit what'll get transformed and what'll remain as is
• only run transformations on what needs to be transformed (though there'll likely be a transformation to turn specific values to null in any case)
• allow arbitrary transformations if another design system needs to build a component with something we didn't think of for parsing

// The `as...` functions would likely be in their own modules, maybe alongside the `normaliseData` one
function asInt(stringValue) {
  if (!stringValue) {
    return null;
  }
  return Number(stringValue);
}

function asBoolean(stringValue) {
  if (!stringValue) {
    return null;
  }
    if (stringValue === 'true') {
      return true
  } else if (value === 'false') {
      return false
  } else {
      return Boolean(value)
  }
}

function normaliseData(config, normalizers) {
  for(var key in normalizers) {
        // We don't want to be adding keys that were not there
        if (config[key]) {
            config[key] = normalizers[key](config[key]);
        }
  }
  return config;
}

function MyComponent($module, config) {
  var defaultConfig = {
    points: 100,
    isAwesome: true
  }

  this.config = mergeConfigs(
    defaultConfig,
    config,
    normaliseData($module.dataset, {points: asInt, isAwesome: asBoolean})
  );
}

[romaricpascal]

Another question, is whether we normalise only the element's dataset or the merged config. Normalising only the dataset means we expect people passing values in JavaScript to provide the right type. The main concern here is more with Boolean than Numbers, as 'false' would give the opposite of false when evaluated as a Boolean.

[36degrees]

I think if we opted for that sort of approach it'd be useful to keep the 'config config' (!) in one place, rather than having some of it in class-level variables and some of it passed to normaliseData as options.

Something like:

function MyComponent($module, config) {
  var defaultConfig = {
    points: {
      type: 'number',
      default: 100
    },
    isAwesome: {
      type: 'boolean',
      default: true
    }
  }
}

This would be particularly useful if we do end up moving to components inheriting from a base component that takes care of the config merging for us, as we'll want a single place to determine how the config should work.

This does however feel like it's starting to become quite complicated considering currently parts of it will need to be duplicated across every component that takes config…

Maybe we should optimise for the simplest option to allow us to handle the config in the components we have now, and revisit in 5.0? I guess that'd be option 1? 🦆

[36degrees]

Alternatively, we can work around this for now by handling it at the component level, for example in the button component we'd need to do something like:

if (config.preventDoubleClick === 'true' || config.preventDoubleClick === true) {

Which is a little messy but might unblock #2808 and we can revisit later.

[romaricpascal]

I thought of grouping defining the type in the same hash as the default, like you describe. Indeed, it has quite a different impact on what needs to be updated, so left it out (should probably have mentioned it as "a route we should probably not take for now"). I think it'd be a great pattern to have if we move to a base component, though.

The method I was proposing felt like a step that would need to happen before that and we could take now. I can understand the clunkiness of having defaults in one place, parsing in another though.

Regarding that last solution, both the Button and the CharacterCount each have a very specific place where they access the attribute, and only one so it feels like a good idea to get us out for now.

I guess it depends on how far ahead we see the update of components to using a base class vs. having new components come in that'd need some parsing. If we foreses a lot of parsing comin ahead, we'd likely want a specialized method. On that topic, it's worth noting that using defaults to get the types wouldn't work for the CharacterCount as it doesn't have any default maxlength (that I could find, but maybe I missed something). So we may be tied to Bootstrap-like duck-typing for parsing the values. I don't think it's such a bad thing for the moment, though.

A couple of thoughts while I read through:

1. The button reads the data-prevent-double-click attribute during the execution of the click listener. This means that another piece of JS may toggle that attribute to control whether to prevent double clicks or not at runtime. I'm not sure if anyone does it, nor can't think of a reason why you'd want to sometimes prevent, sometimes not. Moving to normalizing inside the component's init() call would result in changing that, as we'd store the value at that point in time. Maybe that's more a discussion for #2808, though.
2. The parsing should likely be extended to data-maxwords as well, as it's a number just like data-maxlength

[colinrotherham]

For options/config did the https://github.com/alphagov/govuk-frontend/issues/2736 JSON approach get ruled out?

Input: JSON string from data attribute

<div id="my-component" data-config='{ "points": 100, "default": true }'>
  <!-- Example -->
</div>

Output: JavaScript object with types

const component = document.getElementById('module')
const config = JSON.parse(component.dataset.config)

console.log(config) // { points: 100, default: true }

[36degrees]

We touched on it briefly in https://github.com/alphagov/govuk-frontend/issues/2736#issuecomment-1208303510 but I think the biggest downsides were:

• It means that you need a way to JSON encode things from the 'view' layer (so that we can convert the Nunjucks options for i18n into the JSON object). We have this in Nunjucks via the dump filter but we weren't confident that port maintainers would have this available in every templating language
• It felt messier in terms of dealing with quotes and escaping

As flagged in the linked issue, Bootstrap now support this via data-bs-config but that's in addition to the individual data attributes.