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

Prevent JavaScript component config being flattened #4230

Closed
colinrotherham opened this issue Sep 19, 2023 · 1 comment · Fixed by #4792
Closed

Prevent JavaScript component config being flattened #4230

colinrotherham opened this issue Sep 19, 2023 · 1 comment · Fixed by #4792
Labels
accordion 🐛 bug Something isn't working the way it should (including incorrect wording in documentation) button character count error summary exit this page javascript notification banner
Milestone

Comments

@colinrotherham
Copy link
Contributor

colinrotherham commented Sep 19, 2023

Description of the issue

Our JavaScript component configs are being flattened in mergeConfigs() and don't match JSDoc output

this.config = mergeConfigs(
  Accordion.defaults,
  config || {},
  normaliseDataset($module.dataset)
)

Steps to reproduce the issue

Instantiate a component (e.g. Accordion) using the JavaScript API

Before

Console output for Accordion.defaults shows an unflattened config

{
  i18n: {
    hideAllSections: 'Hide all sections',
    hideSection: 'Hide',
    hideSectionAriaLabel: 'Hide this section',
    showAllSections: 'Show all sections',
    showSection: 'Show',
    showSectionAriaLabel: 'Show this section'
  },
  rememberExpanded: true
}

After

Console output for new Accordion().config shows an unexpectedly flattened config

{
  'i18n.hideAllSections': 'Hide all sections',
  'i18n.hideSection': 'Hide',
  'i18n.hideSectionAriaLabel': 'Hide this section',
  'i18n.showAllSections': 'Show all sections',
  'i18n.showSection': 'Show',
  'i18n.showSectionAriaLabel': 'Show this section'
  module: 'govuk-accordion',
  rememberExpanded: true
}

Both are accidentally typed with AccordionConfig even though they're different

Actual vs expected behaviour

Whilst we've picked "flattening" to our method to normalise i18n text between data-* attributes and the JavaScript API, the final merged new Accordion().config is expected to match the JavaScript API format in JSDoc output.

We hold our JavaScript component configs in two places:

  1. Using static fields for defaults, e.g. Accordion.defaults
  2. Using class fields once merged, e.g. new Accordion().config

We merge component config data-* attributes over JavaScript config, using the defaults to fill the gaps

@colinrotherham colinrotherham added 🐛 bug Something isn't working the way it should (including incorrect wording in documentation) button javascript character count accordion error summary notification banner exit this page labels Sep 19, 2023
@colinrotherham colinrotherham added this to the v5.0 milestone Sep 19, 2023
@colinrotherham
Copy link
Contributor Author

To avoid confusion, we should distinguish mergeConfigs() from a new flattenConfigs() helper

Then we may want to clearly flatten and extract only i18n key/values as shown:

this.config = mergeConfigs(Accordion.defaults, config)

// Component data attributes
const dataset = normaliseDataset($module.dataset)

// Flatten i18n config property only
this.config.i18n = extractConfigByNamespace(
  flattenConfigs(this.config, dataset),
  'i18n'
)

this.i18n = new I18n(this.config.i18n)

Additional dataset properties

Notice the untyped config property module above from data-module="govuk-accordion"

We may want to assign only known dataset values rather than merging them all

const dataset = normaliseDataset($module.dataset)

// Add known values only
this.config.AAA = dataset.AAA
this.config.BBB = dataset.BBB
this.config.CCC = dataset.CCC

@36degrees 36degrees modified the milestones: v5.0, v5.x-candidate Sep 26, 2023
colinrotherham added a commit that referenced this issue Feb 23, 2024
This prevents unnecessary data attributes (e.g. `data-module="govuk-accordion"’) from being merged into the config

See: #4230
colinrotherham added a commit that referenced this issue Feb 23, 2024
This prevents unnecessary data attributes (e.g. `data-module="govuk-accordion"’) from being merged into the config

See: #4230
colinrotherham added a commit that referenced this issue Feb 23, 2024
This prevents unnecessary data attributes (e.g. `data-module="govuk-accordion"’) from being merged into the config

See: #4230
colinrotherham added a commit that referenced this issue Feb 27, 2024
This prevents unnecessary data attributes (e.g. `data-module="govuk-accordion"’) from being merged into the config

See: #4230
colinrotherham added a commit that referenced this issue Mar 7, 2024
This prevents unnecessary data attributes (e.g. `data-module="govuk-accordion"’) from being merged into the config

See: #4230
colinrotherham added a commit that referenced this issue Mar 8, 2024
This prevents unnecessary data attributes (e.g. `data-module="govuk-accordion"’) from being merged into the config

See: #4230
colinrotherham added a commit that referenced this issue Mar 8, 2024
Since we only pass in `DOMStringMap` values, this removes all tests for `normaliseDataset()` that don’t pass in strings

We also prevent unnecessary data attributes (e.g. `data-module="govuk-accordion"’) from being merged into the config

See: #4230
colinrotherham added a commit that referenced this issue Mar 8, 2024
Since we only pass in `DOMStringMap` values, this removes all tests for `normaliseDataset()` that don’t pass in strings

We also prevent unnecessary data attributes (e.g. `data-module="govuk-accordion"’) from being merged into the config

See: #4230
@36degrees 36degrees modified the milestones: v5.x-candidate, [next] Mar 21, 2024
owenatgov pushed a commit that referenced this issue Apr 4, 2024
Since we only pass in `DOMStringMap` values, this removes all tests for `normaliseDataset()` that don’t pass in strings

We also prevent unnecessary data attributes (e.g. `data-module="govuk-accordion"’) from being merged into the config

See: #4230
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
accordion 🐛 bug Something isn't working the way it should (including incorrect wording in documentation) button character count error summary exit this page javascript notification banner
Projects
Development

Successfully merging a pull request may close this issue.

2 participants