Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Store bound update method in constructor and use it as callback #13

Merged
merged 1 commit into from Feb 1, 2016

Conversation

blikblum
Copy link
Contributor

Currently each time update method is used as callback, it pass a new / different bound instance
The callback is not removed when unobserve is called and each time the same object is bound to a rivets view the callback count increases.

Another side effect is the memory leak, independent of the object be rebound to a view, because observer instance cannot be collected so the binding and the HTMLElement.

The issue can be see in examples as simple as http://s.codepen.io/blikblum/debug/BjYPgo
Just break at rivets.adapter['.'].unobserve, click in bind / unbind and see 'person' callbacks increase

@blikblum
Copy link
Contributor Author

Just see #12 now. Basically the same, just with a different name

@stalniy
Copy link

stalniy commented Jan 27, 2016

@mikeric I believe a lot people would like to see this being merged!

@ydaniv
Copy link

ydaniv commented Jan 28, 2016

Sorry for the noise, but a big +1!

@mikeric
Copy link
Owner

mikeric commented Feb 1, 2016

Hey guys, merging this in. Sorry for my lack of involvement over the past year. Glad to see there are many people interested in furthering the development of these projects.

@Duder-onomy, @Leeds-eBooks I've added you guys as contributors to this project as well, since their releases often need to be coordinated.

mikeric added a commit that referenced this pull request Feb 1, 2016
Store bound update method in constructor and use it as callback
@mikeric mikeric merged commit a9a11fc into mikeric:master Feb 1, 2016
@blikblum blikblum deleted the use-same-callback-reference branch February 1, 2016 08:56
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants