Change variable name in adapter.observe to avoid argument capture into closure #546
Conversation
|
After some investigation, i found that the two remaining detached DOM nodes in TodoMVC example does not increase over the time and is removed as soon as the view get's out of scope. It is a implementation detail from each binder and does no harm. |
|
Merging this is beyond my understanding of coffeescript, hopefully someone else can come in on this. |
|
OK. An strategy is to look at the diff between compiled code before and after, but i'll understand if you still not feels confident in merging. |
| @@ -77,7 +77,7 @@ Rivets.public.adapters['.'] = | |||
| callbacks = map.callbacks | |||
|
|
|||
| if callbacks[keypath] | |||
| callback() for callback in callbacks[keypath].slice() when callback in callbacks[keypath] | |||
There was a problem hiding this comment.
@blikblum I understand this now. Why do you think the original author was cloning the array and checking with when? Is there a reason you haven't followed this?
There was a problem hiding this comment.
I think it was an oversight or remaining of a refactoring
We already know that the callback is in the array, since we just looped over it. There's no need to check again if is in the array. Looking at the JS generated code helps to understand.
|
Many thanks. |
|
Yep, I've tried unsuccessfully to tweet him, you could email him? |
|
I dont have his contact |
|
His Gmail is here https://github.com/mikeric |
|
I sent him a message. Lets wait |
Report PR #546 on ES6 branch
Don't use view.els.forEach to avoid exception when jquery is used Report PR mikeric#546 on ES6 branch
In the default adapter
observemethod the callback argument is accidentally captured into the closure. This can lead to memory leaks in situations as simple as http://codepen.io/blikblum/pen/BjYPgoThe PR also removes an redundant check in the loop
With this and mikeric/sightglass#13 most of the leaks found in the TodoMVC example ( tastejs/todomvc#1295 ) are resolved.
Some leaks still remains, which i'm looking at
@mikeric @Leeds-eBooks