Cleanup is not removing event listeners

[FingalP]

Note: for support questions, please use the Universal Slack Channcel or https://gitter.im/angular/universal

• I'm submitting a ...

• bug report
• feature request

• Which parts of preboot are affected by this issue?

• server side
• client side
• inline
• build process
• docs
• tests

• Do you want to request a feature or report a bug?

report a bug

• What is the current behavior?

Cleanup is not removing event listeners.

• If the current behavior is a bug, please provide the steps to reproduce and if possible a minimal demo of the problem by creating a github repo.

Open a prerendered page and use the Chrome inspector to see the event listeners on your root component.

• What is the expected behavior?

Cleanup should remove all of the event listeners.

• What is the motivation / use case for changing the behavior?

This has lead to issues which fail silently, are hard to debug and it's not clear that they are related to preboot. For example, a button of type submit inside a form which was prerendered only worked if I click on the text of the button, but not if I click on the rest of the button. I'm not sure exactly why that happened, but removing the event listeners fixed it.

• Please tell us about your environment:

• Browser: Chromium 68
• Language: TypeScript 2.9.2 with target es5
• OS: Linux

• Other information (e.g. detailed explanation, stacktraces, related issues, suggestions how to fix, links for us to have context, eg. stackoverflow, gitter, etc)

I fixed it by using this in my project:
document.addEventListener('PrebootComplete', () => {
prebootData.listeners.forEach(listener => {
listener.node.removeEventListener(listener.eventName, listener.handler, true)
})
})
It seems that the line linked below just needs to have an additional argument to removeEventListener, true, as they have useCapture=true.

preboot/src/lib/api/event.replayer.ts

Line 178 in 60a3ea8

listener.node.removeEventListener(listener.eventName, listener.handler);

The event listeners are added here:

preboot/src/lib/api/event.recorder.ts

Line 178 in 60a3ea8

serverRoot.addEventListener(eventName, handler, true);

[mgol]

Thanks for the report! I missed this in my Preboot refactor in 60a3ea8.

I don't think we should hardcode true for the last isCapture parameter to removeEventListener as it'd be good to keep prebootData.listeners generic. Rather than that, I'd change the type of prebootData.listeners so that its elements have an additional isCapture field, by default set to false. In the line where event handler data is pushed to prebootData.listeners we'd set isCapture to true and then read if during the removal.

Would you like to submit a PR? I can help if you have any doubts.

[FingalP]

Thanks for the reply, @mgol! I'm a little busy at the moment, but I'll see if I can get around to this sometime next week. It would be my first PR so I'm unsure how long it would take me.

[mgol]

No problem. :) Let me know if you get stuck on anything, I'd be happy to answer any questions you might have.

If it turns out you won't have time for it in the foreseeable future, that's OK, just please write a comment about that here so that we know we need to do it ourselves (or with help from another contributor). Take care!

[vdjurdjevic]

Any news for this? I can make pull request later today if this is still an issue.

[Splaktar]

@vladimirdjurdjevic AFAICT a PR for this is needed and would be welcome. Thank you!

[vdjurdjevic]

I just want to make sure that it will be merged before I spend time working on it. It looks like it's not actively maintained, that's why I asked first :) I will get my hands on it on Saturday, and submit PR as soon as possible.