fix(focus-monitor): implement OnDestroy logic

[crisbeto]

Similarly to most of the other DOM-related services, these changes implement ngOnDestroy for the FocusMonitor which removes the monitoring from all of the currently-monitored elements.

[crisbeto]

Note: I would prefer to keep this as a WeakMap, but we need a Map in order to be able to iterate over the elements. Alternatively, if we make an assumption that all the monitored elements will be removed on destroy, we could remove only the global listeners.

[jelbourn]

This is a tricky trade-off. We don't have any data to know for sure, but I expect most apps will end up with FocusMonitor provided at the root, meaning that using Map could end up making elements remain in memory after they'd otherwise be removed.

Do you think it would be common that monitored elements would remain while the module with the FocusMonitor would be unloaded?

[crisbeto]

I believe the most common case would be that the elements are removed while the monitor remains, in which case the responsibility for stopping the monitoring is on the consumer.

Regarding using the Map, I have a hunch (haven't verified it) that the current setup would retain references to the elements as well. While we use the element as a key, we also have a reference to it in the unlisten function which is inside the value. I think that this would retain the reference until the item is removed from the map.

[jelbourn]

You're right, the unlisten would preserve the element reference already, so switching to a Map won't change anything

[mmalerba]

Does ngOnDestroy get automatically called for Injectables? I was under the impression it didn't and that the lifecycle methods only applied to Components and Directives.

[jelbourn]

@mmalerba it gets called if the module is unloaded, which very large apps (like GCP) do.

[jelbourn]

LGTM

[angular-automatic-lock-bot]

This issue has been automatically locked due to inactivity.
Please file a new issue if you are encountering a similar or related problem.

Read more about our automatic conversation locking policy.

_{This action has been performed automatically by a bot.}