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

Investigate components being initialised more than once #1127

Closed
Tracked by #5292 ...
joelanman opened this issue Jan 11, 2019 · 14 comments · Fixed by #5272
Closed
Tracked by #5292 ...

Investigate components being initialised more than once #1127

joelanman opened this issue Jan 11, 2019 · 14 comments · Fixed by #5272
Assignees
Labels
🕔 hours A well understood issue which we expect to take less than a day to resolve. javascript
Milestone

Comments

@joelanman
Copy link
Contributor

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
Copy link
Contributor

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
Copy link
Contributor

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
Copy link
Contributor

NickColley commented Mar 1, 2019

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,

@timpaul timpaul changed the title Should components warn if they're initialised more than once? [SPIKE ] Should components warn if they're initialised more than once? Mar 4, 2019
@timpaul timpaul added Priority: Low 🕔 hours A well understood issue which we expect to take less than a day to resolve. labels Mar 4, 2019
@kellylee-gds kellylee-gds added ⚠️ high priority Used by the team when triaging and removed Priority: Low labels Apr 3, 2019
@timpaul timpaul changed the title [SPIKE ] Should components warn if they're initialised more than once? [SPIKE ] Investigate components being initialised more than once Jun 26, 2019
@NickColley
Copy link
Contributor

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.

@NickColley NickColley changed the title [SPIKE ] Investigate components being initialised more than once Investigate components being initialised more than once Nov 22, 2019
@vanitabarrett vanitabarrett removed 🔍 investigation ⚠️ high priority Used by the team when triaging labels Jan 17, 2022
@vanitabarrett
Copy link
Contributor

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
Copy link
Member

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.

@querkmachine
Copy link
Member

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
Copy link
Contributor Author

happened again on support today

@querkmachine
Copy link
Member

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
Copy link
Contributor

@joelanman
Copy link
Contributor Author

was this fixed in recent work?

@romaricpascal
Copy link
Member

Issue should be fixed by #5272, but we'll wait till it's released to close this issue properly.

@romaricpascal
Copy link
Member

GOV.UK Frontend v5.7.0 now prevents components from being initialised twice on the same element, so this can now be closed.

@owenatgov owenatgov added this to the 5.7.0 milestone Jan 7, 2025
@domoscargin domoscargin moved this from Ready to release 🚀 to Done 🏁 in GOV.UK Design System cycle board Jan 9, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
🕔 hours A well understood issue which we expect to take less than a day to resolve. javascript
Projects
None yet