search

angular-ui / ui-tinymce

AngularUI wrapper for TinyMCE
MIT License
488 stars 371 forks source link

fix(Memory leak): binding tinymce events with angularjs in configurat… #194

Closed arjunasuresh3 closed 8 years ago

arjunasuresh3 commented 8 years ago

…ion using pub-sub pattern to detach circular references between tinymce object and angularjs objects.

Attaching profile screen shot: tinymce_fix

Fixes #191

arjunasuresh3 commented 8 years ago

@deeg plunker with the fix.

http://plnkr.co/edit/ykjMMuYyj4WaMHntSVFT?p=preview

deeg commented 8 years ago

Thanks for posting this PR with the screenshot plunker and to the correct branch.

I will try and review tonight and we can merge in barring any changes needed.

One thing I want to get people in the habit of is writing tests for their changes, so we can start to get full coverage. If you wouldn't mind updating the PR with some tests that would be great.

In this case since it is an important fix, might fix some other issues, and the state of the tests is not up to par right now, I am okay letting it slide this time. I plan to add/update the tests in the near future to try and get full coverage when I have some more time.

arjunasuresh3 commented 8 years ago

@deeg I will add test cases in my free time. Held up at important tasks.

deeg commented 8 years ago

@arjunasuresh3, sorry I have not had time to look at this. I will get to it ASAP

deeg commented 8 years ago

@arjunasuresh3, I reviewed the difference in heap snapshot and timeline both with & without your pull request.

They seem to be the same to me. Seems like after the garbage collection runs (when the values in the timeline dip) the values go back to very similar numbers.

Would you mind describing to me the difference in the timelines which shows a memory leak that was fixed?

deeg commented 8 years ago

Closing this due to inactivity. I'm happy to have more discussion on #191.