Add destroy() method to avoid memory leaks in SPAs

[kswedberg]

I love the functionality of your script, but I realized that if I used it in a site that uses pushState for page loading, it would leak memory as the event handlers are never removed. In order to avoid this problem, I changed rovingIndex to a constructor function — while still allowing it to be called without new — and added a destroy() method to it.

I put together a codepen so you can see how this would work at https://codepen.io/kswedberg/pen/VwbrgNy

Let me know if you have any questions or would like me to change anything

[argyleink]

nice, makes sense that the events can stack up because they're never removed 👍🏻

what about using DOMNodeRemoved on the rover element? we could clean things up without adding a remove() for authors, we'd watch for the element to be removed and clean it up in the lib?

[kswedberg]

Mutation events are deprecated, so I'd be hesitant to use it (https://developer.mozilla.org/en-US/docs/Web/API/MutationEvent). Even if you used a Mutation Observer instead, I don't think it would work because you'd have to attach it to an element that is not being removed for it to be triggered. I could be wrong, but I'm not seeing how it would be possible.

[argyleink]

here's where my heads at currently:

• are we sure it's a memory leak or just guessing? how many roving-ux instances need created to see the effect of the leak?
• i understand your hesitation to use DOMNodeRemoved, but i don't think it's risky.

while those are on my mind, i do think it could be ok to return a destroy() method on the return object from calling rovingIndex(). it's a normal enough pattern, and as long as we document it, it's cool. but, if you're down to refactor to DOMNodeRemoved, less of the original src code would need changed and no documentation updates required.

if calling destroy is def the way you think this should go, here's the remaining items I'd appreciate seeing:

• update the readme to mention the teardown function on the returned object. aka, document the api
• remove the instance of check and the use of .prototype in favor of the simpler this.destroy = () => { ... }, less to juggle and can achieve the same scoped effect

thoughts?! 🙂

[kswedberg]

Thanks for the reply. Here are a couple of my thoughts in response:

• I'm sure it's a memory leak. Not sure how many instances would be needed to "see the effect," but in a dynamic environment in which I can't predict a user's behavior, I also don't know how many instances would actually be created, so better safe than sorry. One use case I have for this is a row of buttons for a wysiwyg editor. Any number of editor instances could be initialized on a given page, so it could definitely become a problem.
• I'm curious about your feeling that it isn't risky to use DOMNodeRemoved. MDN cautions: "Be aware that this feature may cease to work at any time." I guess in general browser vendors are reluctant to remove features, but it still makes me nervous. Do you have inside knowledge about this one?

I don't know of a way to use this.destroy = () => { ... } inside the rovingIndex function without making it a constructor function, as this will be undefined. That said, we could do rovingIndex.destroy = () => { ... } and then return rovingIndex at the end of the rovingIndex function, which seems kind of weird to me, but I think it would work.

When I have a bit more time I'll try to put together a couple other options if you're interested.

[argyleink]

I'm curious about your feeling that it isn't risky to use DOMNodeRemoved.

It's not a security threat or anything high priority, from what i understand it's just slower and hard to optimize for. Computers are faster every year though and it's not slow enough to be worried about. I dont see any intents to remove it, there'll be deprecation warnings first. And if I need to, I'll research the best ways at that time to keep the lib working.

When I have a bit more time I'll try to put together a couple other options if you're interested.

I'm def interested in seeing your ideas. Let's find the most minimal way to save the memory.

[kswedberg]

Closing in favor of #7