Introduce dispose method

[MarketingPip]

A dispose method would be nice for this using

clearAllEventListener()
then remove()

[hueitan]

Hi @MarketingPip I think I know what's your suggestion but could you please explain more about your idea?

[MarketingPip]

@hueitan - I am having to reinit this component to detect new links added to DOM. Rather than you guys using Intersection Observer API.

I suggest you add a dispose() method to properly remove / clean up the component and event listeners for memory purposes within apps.

I have peaked through the code - and suggest doing

clearAllEventListener()
element.remove()
// set any references to the element etc / things stored to null..

in events.

I could possibly make a PR - tho I will be honest I didn't have much hope I was going to get a reply on this repo. Seeing the bundle size issue still being open from years ago!

[hueitan]

Thanks for the feedback @MarketingPip

We previously worked on a dispose() like method in one of the refactor branches reef, but we decided to drop it due to issues with a third-party library.

We have a milestone to rewrite the library with a more structured solution instead of the current vanilla JavaScript implementation, but I don't have an update on the progress or deadline at the moment.

Pull Requests are welcome. As you may know, we use clearAllEventListener() in some places, but not everywhere.

[MarketingPip]

@hueitan - can you send me the pull request / the exact issue into the 3rd party library. For wikipedia etc - I would be willing to take some time to make a PR to nip this in the butt.

[hueitan]

This is the closed PR #191 and ticket https://phabricator.wikimedia.org/T363468

There isn't much you can look at for the event listener issues from these links, I would recommend you make the changes based on the current codebase.

[MarketingPip]

@hueitan - then I am confused... There should be no issue with removing the element / and all event listeners when not needed. By using this code.

clearAllEventListener()
element.remove()
// set any references to the element etc / things stored to null..

[hueitan]

@MarketingPip, I understand your point now about removing elements after calling clearAllEventListener().

In some scenarios, we don't completely remove the DOM elements because we reuse them to bind new events. For instance, when closing a preview and then opening a new one, the same element might be used again. Because of this, we can’t just remove the element outright within clearAllEventListener().

That’s also why we introduced the idea of refactoring—to unify how we handle event listeners across the board. This way, we can ensure a consistent approach without running into these kinds of challenges.

For now, I don’t see this as a major issue, since modern browsers are quite efficient at managing memory.

[MarketingPip]

@hueitan - I get this... But again.. For new links I append to the DOM. Links DO not get detected. I have to reinit the component - which adds MULTIPLE event listeners - causing a leakage in memory.....

So it would be ideal to COMPLETELY remove the component / event listeners WHEN I do not need this..............

As for your last comment. Browsers are NOT efficient for managing memory.

[hueitan]

https://phabricator.wikimedia.org/T380988

@MarketingPip Thanks for the explanation and I see this a problem now when we reinit() on the page

I have the several ideas for the fix

1. during the init(), we check if there is duplicate link
2. introduce a new method destroy(), you can decide destroying all or selected section

[MarketingPip]

@hueitan - better idea. Remove all event listeners by default on init - so if called again by accident. No extra / duplicate event handlers are added. Then add event listeners. Plus add a dispose method - as this should be a standard with any component (even tho I am guilty of not doing this).