Skip to content

Conversation

@dumbNickname
Copy link

Hey.

This is a brute force fix for leaking listeners and DOM elements. Currently scope parameter of link function is being passed via closure to listener function and to functions in data factory. As always in closures, this means that browser will keep whole context of the linking function, so here it will include element as well. It made proper garbage collection not possible (profiled on Chrome).

It can be fixed cleaner e.g. by using local variables in linking function and then passing those via closure or by restructuring the code a bit. Please excuse that I did not do it in the first place, but I do not know the project and had to quickly plug in this component.

I am pushing this PR just to provide a quick fix - in case anybody else would like to get rid of performance issues. If you decide to solve the problem in another way, please just ignore this PR.

Cheers!
Bartek

@dumbNickname
Copy link
Author

Hi again,

I noticed that nobody has merged this PR (probably due to the build failure, which is related only to decreasing code coverage). Do you want to write a test that checks proper resources unbinding on destroy? Or do you plan to rewrite the code in another manner? I am asking as I would like to keep a clear dependency to this component in our project, instead of some customized version.

Thank you in advance for any reply.

@dalelotts
Copy link
Owner

Sorry for the delay - you will need tests and also to submit the PR to the develop branch.

See https://github.com/dalelotts/angular-bootstrap-datetimepicker/blob/master/contributing.md for more details on submitting PR's

@dumbNickname
Copy link
Author

Forgot about this issue and somehow did not notice the notification about your reply ... sorry. I checked out of curiosity what is going on with the PR and then noticed that you replied. Is this issue still up to date?

@dalelotts
Copy link
Owner

If there are still leaking listeners, I'd love to get them fixed! If you want to submit to the develop branch with tests then I can merge the PR.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

2 participants