intercom/ember-href-to

Click handler leaks after app destruction

teddyzeenny opened this issue · 6 comments

The click event handler in the initializer is leaking after the app is destroyed.

While the next app created clears the previous listener, if an app is destroyed without being followed by another app's initialization there's no way to clear the body's event listener.

We were previously working around that by calling $('body').off('.href-to') after destroying our app, which stopped working when jQuery was replaced by a regular event listener. While automatically cleaning up the listeners would be great, exporting a function to remove the listener would suffice since it will allow us to manually remove the listeners after we destroy our app.

Something like this for example?

export function removeListeners() {
  document.body.removeEventListener('click', hrefToClickHandler);
}

Thanks @teddyzeenny. Just so I'm clear, in what case is negatively affecting you?

Hi @GavinJoyce! This is mostly affecting us when investigating memory leaks during tests. When we're working with one test, it's almost impossible to detect real memory leaks because the addon leaks the application instance and therefore the container which leaks all of the singletons.

While this specific leak is not harmful as it will not leak between tests, it produces too much noise in the heap snapshot after a single test which prevents us from detecting other harmful leaks.

Thanks, makes complete sense. 👍 on adding a removeListeners function.

More generally, I wonder if ember initializers should expose a tear down hook so that this can happen internally?

I wonder if ember initializers should expose a tear down hook so that this can happen internally?

Agreed.

@GavinJoyce do you think something like this would be better than exporting a removeListeners?

// app/instance-initializers/ember-href-to.js
// .... after setting up the event listener...
applicationInstance.reopen({
  // Cleanup
  willDestroy() {
    document.body.removeEventListener('click', hrefToClickHandler);
    hrefToClickHandler = undefined;
    return this._super(...arguments);
  }
})

@teddyzeenny, if that works that seems much better