alphagov / govuk-frontend

GOV.UK Frontend contains the code you need to start building a user interface for government platforms and services.
https://frontend.design-system.service.gov.uk/
MIT License
1.16k stars 319 forks source link

Investigate components being initialised more than once #1127

Open joelanman opened 5 years ago

joelanman commented 5 years ago

We had a case of a user initialising the components twice by accident. This doesn't warn or error, but creates a broken state (the accordion for example opens then closes on click, and creates two 'Open all' buttons.

NickColley commented 5 years ago

We need to make sure that we allow for use cases where people inject new HTML into the DOM after the page has loaded and then running the initialise functions to enhance the new components.

Custom Elements seem to account for this, and the polyfills have code that could help us

https://github.com/webcomponents/custom-elements/blob/7263e628f3d13b6a66e23c61147b299f31ff0964/src/Patch/HTMLElement.js#L48

NickColley commented 5 years ago

I've put together an example of how you could store instances on the classes for each component.

We'd likely also want to have consistent destroy methods too, which I've demonstrated.

https://jsbin.com/mapoqof/edit?js,console,output

NickColley commented 5 years ago

My approach I've shared already puts the instance information on the components themselves, but we could also consider an approach similar to web component custom elements.

Where we have a single registry (could be an array), that we update and check:

if (!window.customElements.get('x-component')) {
  window.customElements.define('x-component', ComponentElement)
}

It's important to consider how downstream components might also copy this approach.

I think this approach only works well when you can be sure that initialising a component once will then work for any new components added to the page, for example loading markup in a modal.

Since this is not the case with GOV.UK Frontend, it may not be the best way to do things,

NickColley commented 4 years ago

Seen this again from a Prototype Kit user.

Their tabs component was skipping two tabs at once since they accidentally included their application.js bundle twice in the page which meant the component was initialised twice.

vanitabarrett commented 2 years ago

As discussed in the JavaScript effort/value session, this is happening because we don't have any guards against initialising a component more than once. This doesn't feel right, but it's also a bit of an unusual case which doesn't come up very often.

querkmachine commented 2 years ago

Having this functionality may come in useful with our current work relating to localisation and programatic APIs.

For example, a service developer may only want to pass configuration options to a specific instance of a component, but use the default options for all other components. In attempting to do so, they may write something like this:

new window.GOVUKFrontend.Accordion($element, { config }).init()
window.GOVUKFrontend.initAll()

However, unless the developer takes care to remove the data-module attribute from the custom instance (which isn't possible if using our Nunjucks macros), this will initialise the same Accordion twice—resulting in multiple buttons and event handlers being created.

The simplest way to do this is probably by recording which elements have already been initialised and checking against that record when components are initialised again. This could be done by adding a data-* attribute to the element, recording it in the element's dataset (this approach won't work in IE8–10), or maintaining a persistant array of initialised elements.

vanitabarrett commented 2 years ago

Discussed this today and decided it's not necessary for MVP internationalisation work, but could be a stretch goal. Otherwise, we might look at this as part of our later JavaScript work anyway.

querkmachine commented 2 years ago

In a call this week about upcoming changes to the Prototype Kit, it was asked whether it was possible to run Frontend's initAll function multiple times without side-effects, as they were investigating doing so as a way of implementing certain features (I think extensions?)

joelanman commented 1 year ago

happened again on support today

querkmachine commented 1 year ago

Another instance of this happening on support today, this time in an app using Turbolinks.

Turbolinks caches the DOM state of a page when navigating away, restoring that state if the page is navigated back to (in a similar, but not the same, way as bfcache). This was resulting in DOM manipulations made through our JavaScript happening multiple times if the page was navigated away from and back to again.

domoscargin commented 1 year ago

Another instance of this happening on support today, at: https://github.com/DFE-Digital/npq-prototype/blob/main/app/views/registration-status/accordion.html

joelanman commented 5 months ago

was this fixed in recent work?