Implementing proper cleanup of components when destroyed

[langscot]

A lot of JavaScript frameworks (React, Vue, SvelteKit, etc.) rely on the handy HSStaticMethods.autoInit()method. This is often ran whenever the DOM conditionally re-renders (page change, user logged in, tab navigation, etc.). Whilst this is a good fix, it results in the respective collections array for each plugin growing in size for the duration of the session (without page refresh).

To get around this, we can look to the HSStaticMethods.cleanCollection() method, which flushes out the instances from each collection array. So, we could call this clean method before we call the auto intialize method. However, the clean method does not "cleanup" the event listeners or other elements added by the components init() methods. This results in things breaking. An example is an overlay toggle, which may have previously "destroyed" overlay instances' onClick event listeners attached, resulting in multiple backdrops being shown.

See my fork, where I have added an abstract method destroy() on the HSBasePlugin class, and have made the HSStaticMethods.cleanCollection() method call this destroy() for each instance of HSBasePlugin in the respective collection arrays.

I have only really implemented a proper destroy() method for the HSOverlay class, which removes all event listeners from any DOM elements. This has fixed my issue where multiple backdrops would appear when clicking an open modal button.

Would the maintainers be open to implementing such a mechanism for cleanup, if so, I would be happy to put in the effort to create an issue, discuss a proper design solution, and implement/contribute to a PR. If not, no worries, I will continue to use my fork.

[oliverhaas]

Do you know why cleanCollection(), i.e. resetting the array of the elements, is not enough to clean up old instances? Like, where is the reference that keeps an instance alive?

I could not observe problems like you're describing, but as you've seen I have other issues with ajax + preline, and well, some memory leaks...

[oliverhaas]

Ok I get it now. In the end cleanCollection() and autoInit() are not really properly implemented, especially when using both.

As of now I think preline should probably implement the autoInit() function such that it basically always goes through the whole document, checks for new preline components and removes stale ones. Convenient but (slightly?) less efficient than my previous ideas.

[langscot]

In my project so far, using my fork of Preline, I have been having a pretty great experience using it with React. I have a list of cards which open an overlay when clicked. When pagination occurs, via a useEffect, I can re-run cleanCollection() and then autoInit() to conveniently instantiate them all. My fork implements the start of a proper cleanCollection() and destroy() lifecycle for HSOverlay so I don't need to worry about stale components/event listeners/etc.

[oliverhaas]

But your cleanCollection() destroys all the components every time, right?

[langscot]

Yeah - so currently cleanCollection just sets the collection arrays to [] but does no proper "cleanup" of the components. In my fork I have implemented a destroy() method in every component class, but I've only properly implemented it for HSOverlay. The destroy() function basically removes any event listeners on DOM elements, etc. The actual design implementation of this should be discussed as in my fork I have "hacked" it in as quick as I could to keep momentum on my project.

I do like your idea of scoping the autoInit() and cleanCollection() functions by passing in a DOM element. There are parts in my project where I am calling cleanCollection() then autoInit() but I am needlessly doing that on the whole DOM rather than just the part of the DOM that is being refreshed.

[olegpix]

Hi, guys!
It seems to me that @oliverhaas is suggesting that during the full cleanup of the collection, some elements that shouldn't be deleted are being removed.

I had an idea to implement a check directly within the autoInit method, without using the collection cleanup methods. However, it might be necessary to add some kind of destroy method to remove listeners.

The idea is to check whether a specific element still exists on the page after it has been removed or unmounted in some way.

The code is very rough:

if (window.$hsAccordionCollection)
		window.$hsAccordionCollection = window.$hsAccordionCollection.filter(
			({ element }) => document.contains(element.el),
		);

static autoInit() {
	if (!window.$hsAccordionCollection) window.$hsAccordionCollection = [];

	if (window.$hsAccordionCollection)
		window.$hsAccordionCollection = window.$hsAccordionCollection.filter(
			({ element }) => document.contains(element.el),
		);

	document
		.querySelectorAll('.hs-accordion:not(.--prevent-on-load-init)')
		.forEach((el: HTMLElement) => {
			if (
				!window.$hsAccordionCollection.find(
					(elC) => (elC?.element?.el as HTMLElement) === el,
				)
			)
				new HSAccordion(el);
		});

	console.log(window.$hsAccordionCollection);
}

I tested it quite crudely (here: http://localhost:3000/docs/accordion.html), but at first glance, the solution seems to work, although it likely requires significant improvements:

<script>
    window.addEventListener('load', () => {
      (function() {
        const collapse = document.querySelector('#hs-basic-heading-one');

        collapse.remove();

        const newCollapse = document.createElement('div');
        newCollapse.innerHTML = `
          <div class="hs-accordion active" id="hs-basic-heading-one">
            <button class="hs-accordion-toggle hs-accordion-active:text-blue-600 py-3 inline-flex items-center gap-x-3 w-full font-semibold text-start text-gray-800 hover:text-gray-500 focus:outline-none focus:text-gray-500 rounded-lg disabled:opacity-50 disabled:pointer-events-none dark:hs-accordion-active:text-blue-500 dark:text-neutral-200 dark:hover:text-neutral-400 dark:focus:text-neutral-400" aria-expanded="true" aria-controls="hs-basic-collapse-one">
              <svg class="hs-accordion-active:hidden block size-3.5" xmlns="http://www.w3.org/2000/svg" width="24" height="24" viewBox="0 0 24 24" fill="none" stroke="currentColor" stroke-width="2" stroke-linecap="round" stroke-linejoin="round"><path d="M5 12h14"/><path d="M12 5v14"/></svg>
              <svg class="hs-accordion-active:block hidden size-3.5" xmlns="http://www.w3.org/2000/svg" width="24" height="24" viewBox="0 0 24 24" fill="none" stroke="currentColor" stroke-width="2" stroke-linecap="round" stroke-linejoin="round"><path d="M5 12h14"/></svg>
              Accordion #1
            </button>
            <div id="hs-basic-collapse-one" class="hs-accordion-content w-full overflow-hidden transition-[height] duration-300" role="region" aria-labelledby="hs-basic-heading-one">
              <p class="text-gray-800 dark:text-neutral-200">
                <em>This is the third item's accordion body.</em> It is hidden by default, until the collapse plugin adds the appropriate classes that we use to style each element. These classes control the overall appearance, as well as the showing and hiding via CSS transitions.
              </p>
            </div>
          </div>
        `;

        document.body.appendChild(newCollapse);

        HSStaticMethods.autoInit();
      })();
    });
  </script>

If you comment out the line

collapse.remove();

you'll see in the console that autoInit recreates the element, increasing the number in the collection to 28. However, if you uncomment it, the number of elements remains 27. This means the code checks whether a specific element from the collection still exists on the page, and if not, it removes it.

Of course, more testing and improvements are needed for this solution, but this is the general direction of my thinking at this stage.

Thank you for suggesting your solutions and for improving our codebase. Your insights really shed light on certain issues, which is incredibly helpful, and I truly appreciate it! I hope that together we can find the optimal solution.

[oliverhaas]

I think I understand the issue now.

I don't like your approach with cleanCollection() for the specific use-case, since destroying and initializing basically the whole page on every partial update sounds bad to me, because that would usually mean the state of all preline components is lost even if just a tiny part of the html got replaced via ajax.

But in general there has to be a way to "destroy" preline instances (destroy() as a function name already exists in some cases though, e.g. HSSelect). I think currently using autoInit() with ajax naively (like in the docs) will lead to global event listeners piling up, preline instances piling up, and even swapped-out html elements being kept alive by the preline instances....

What preline needs to properly work with ajax/partials is two things

1. autoInit() should allow an html element to be given, such that it the method only works on that element instead of document. This would allow to only initialize html fragments which are inserted into the DOM.
2. A new autoClean() method, which also takes an html element and cleans/destorys all instances in that part of the DOM. This would allow to clean up html fragments which are removed from the DOM.

This would be actually quite easy to implement, and I could provide a PR if someone more official gives the go-ahead.

And in my case with HTMX, I would only need event listeners which call autoInit() and autoClean() on swapping in and swapping out partial html fragments.

[langscot]

I really like your idea of having autoInit take in an HTMLElement to act as the entry point.

I appreciate running cleanCollection() for the entire DOM multiple times could be bad (and certainly feels wrong), but this is a design decision left to the developer. For example, if a developer has a SPA with a few pages and only a handful of components, this would likely be their easiest approach.

For a bigger application, you definitely don't want to be running cleanCollection() on every page change for the reasons you mentioned. However, paired with your suggestion of adding a HTMLElement argument, this could turn into a really nice solution.

[oliverhaas]

A little bit spammy on my part, but I just had a go at implementing my approach
https://github.com/oliverhaas/preline/tree/feat/autoClean

Seems to work well as a proof of concept. As I described earlier I now just clean up and initialize when partial updates happen

document.addEventListener("htmx:afterSwap", function (event) {
    // event.detail.target is the node that was swapped out by HTMX, we clean up all preline in there
    window.HSStaticMethods.autoClean(undefined, event.detail.target);
    // event.detail.elt is the node that was swapped in by HTMX, we init up all preline in there
    window.HSStaticMethods.autoInit(undefined, event.detail.elt);
});

One could even do this clean up completely automagically by checking inside autoInit whether an element is still connected to the DOM with element.isConnected, and clean up if it's not connected... But then again, this maybe goes too far and there might be use cases where this causes problems.

EDIT: The code above is wrong. One would probably need to use both beforeSwap and afterSwap...

[jahaganiev]

@langscot @oliverhaas

First of all huge thanks for helping others with the replies - appreciate it much!

Our team will look into the above issue in details as soonest as possible.

Cheers!

[olegpix]

@langscot @oliverhaas Thank you, guys, for your discussion and for your great interest in our library. I really appreciate your contributions to improving it!

@oliverhaas I've noticed that the previous PR doesn't include the changes present in your fork. If it's not too much trouble, could you please create a PR from your repository to ours?

Thank you in advance!

[oliverhaas]

The PR are fairly obvious and safe changes, and from my point of view that one can be merged without much issues.

The changes in my fork are both somewhat significant and just a proof of concept, and I would need quite some time to make a clean PR out of it. I'm open to giving it a try, but only if the maintainers are seriously interested in such features (meaning more granular autoInit() and/or an autoClean() function). And then I probably need some feedback on the details too...