-
Notifications
You must be signed in to change notification settings - Fork 320
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
fix: auto init memory leak #441
fix: auto init memory leak #441
Conversation
@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. |
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. |
@olegpix |
@oliverhaas Hi, |
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. |
(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 #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. |
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. |
@jahaganiev do you have a possible delivery date for this month? |
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. |
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. |
@oliverhaas Hi,
|
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. |
@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! |
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.