fix: auto init memory leak

[oliverhaas]

This PR fixes a memory leak in autoInit() of combobox, select, dropdown, overlay, tabs.

When using autoInit() repeatedly, e.g. when doing partial updates with ajax like in the docs https://preline.co/docs/preline-javascript.html, some components will repeatedly initialize global event listeners, even though the repeated ones are not needed.

This PR fixes that just by only registering the event listeners the first time an autoInit() is called.

[oliverhaas]

@langscot Pinging you since you commented on my earlier (messy) PR.

This PR does not address an issue with cleaning up old/destroyed components. My PR only reduces the amount of unnecessarily registered duplicate event listeners in autoInit().

I don't really understand what's the issue with the clean up of old components in autoInit like you're suggesting in #438.

[langscot]

Looked over the commits and looks really solid, nice work 👍

I seen "auto init memory leak" and my mind instantly jumped to the other cleanup issue I had going on, sorry about that.

[oliverhaas]

@olegpix Any updates on this? We've already hit production, and this is basically the easiest 10% of making preline have proper cleanup of components and event listeners.

[olegpix]

@oliverhaas Hi, Yes, this feature is in the priority for the next update (v2.6.0). I will add an additional destroy method for each plugin, to remove listeners, CSS classes, attributes, generated markup. I will also inform you here.

[himanshu-newstok]

Hi @oliverhaas ! I attempted to use your fork locally, but the solution didn’t resolve the issue. I also tested it in the demo link you provided, but unfortunately, it still didn’t work.

[oliverhaas]

(Deleted my previous comment, since I could follow the breadcrumbs a bit)

That demo wasn't by me.

I did not claim that this resolves any specific issue aside from registering too many event listeners, although I am aware that these event listeners are causing more problems than just wasting memory.

BUT: This PR should actually fix the issue you're actually interested in here https://github.com/htmlstreamofficial/preline/issues/463, but I don't have the time to investigate this.

This PR has been open for 3 months, and from my point of view it's a non-breaking simple fix for a bug which probably has multiple symptoms. But maybe I'm missing something.

[jahaganiev]

Just to update the status, this feature is fully ready and coming this month with v2.6.0 release including with all other open PRs and reported bug issues.

[mikescola]

@jahaganiev do you have a possible delivery date for this month?

[mikescola]

Just checking in on this, we never did get the release in November. Looking to see if there is any idea of a delivery date yet.

[jahaganiev]

Hey everyone, the update is live with destroy method for all plugins. Since, we've implemented in our way we will close this PR. Please check out the latest v2.6.0 release. Once again, thanks a lot for the input!

[oliverhaas]

Hey everyone, the update is live with destroy method for all plugins. Since, we've implemented in our way we will close this PR. Please check out the latest v2.6.0 release. Once again, thanks a lot for the input!

@jahaganiev Could you please quickly elaborate what changes you've made compared to this PR specifically? I can see that you changed a lot of other stuff, but as far is this PR goes, I can only see removed blank lines.

[olegpix]

@oliverhaas Hi, Let’s take a look at the updates to the HSComboBox plugin as an example. The main changes relevant to this ticket are:

1. Collection Initialization: In the updated code, global event listeners for click and keyboard actions are added immediately after initializing the collection. This ensures that listeners are registered only once, avoiding duplicate listeners when autoInit is called multiple times.

if (!window.$hsComboBoxCollection) {
    window.$hsComboBoxCollection = [];
    window.addEventListener('click', ...);
    document.addEventListener('keydown', ...);
}

2. Collection Cleanup: The code now filters out elements that are no longer present in the DOM using document.contains(element.el). This prevents stale elements from lingering in the collection, improving performance and maintaining accuracy.

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

3. destroy Method: The new destroy method allows for the complete removal of an HSComboBox instance. This includes unregistering event listeners, removing dynamically added classes and attributes, and cleaning up any generated markup. This enhancement improves memory management, ensures a clean DOM, and supports flexible reinitialization and efficient cleanup in dynamic environments.

[oliverhaas]

Thank you for the answer.

I was curious whether I made a mistake somewhere, but I as far as I understand I did not, and you did step 2 & 3 (and maybe more) on top of the same step 1 I did.

Just to clarify: I would have been open to contributing more (basically something like your step 2 & 3) after this PR, but since it never really got merged I did not really continue working on this.

[olegpix]

@oliverhaas We really appreciate your input, as well as @langscot — it was incredibly helpful in identifying the issue and pointing us in the right direction. Your work on step 1 laid a strong foundation, and we built upon that with steps 2 and 3.

We also understand that the delay in merging the PR might have made it harder to continue, but your willingness to contribute further is highly valued. If you ever decide to revisit or expand on this, your contributions would be more than welcome.

In the future, if you decide to contribute again, smaller PRs focused on specific plugins would be preferable. This makes it easier to merge changes, as smaller, more focused PRs are easier to review and integrate. We’d love to see more contributions, even if they’re in smaller chunks!