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

Protocol for cleaning up after Velocity (to avoid memory leaks) #769

Closed
estaylorco opened this issue Apr 2, 2017 · 9 comments
Closed

Protocol for cleaning up after Velocity (to avoid memory leaks) #769

estaylorco opened this issue Apr 2, 2017 · 9 comments

Comments

@estaylorco
Copy link

I'm using Velocity ("V") in an Aurelia project using the aurelia-animator-velocity plug-in, which, as you may know, is a very, very thin wrapper over V.

Everything works great, except that V doesn't clean up the DOM nodes involved in animation. I ruled out the wrapper by removing it from my project and loading V in the usual way: a script tag in index.html. I then replaced all calls to this.animator.animate(...) with Velocity(...). Again, everything works perfectly.

But V is still leaking. When I navigate away from a view, Aurelia invokes detached(), and all DOM nodes that can be removed are removed. The V nodes remain.

With the plugin, I'm invoking V this way:

this.animator.animate(document.getElementById('recoveryFieldSet'), { height: '80px' }, { duration: 100 });

In other words, I'm not using jQuery. What do I need to do the prepare V nodes for removal from the DOM?

Thank you!

Eric

@Rycochet
Copy link
Collaborator

Rycochet commented Apr 3, 2017

Unfortunately it's something that's on the todo list - currently you'd just have to call Velocity([el, ...], "stop", true); on them when removing (and even then I'm not sure if it frees resources properly).

@estaylorco
Copy link
Author

estaylorco commented Apr 3, 2017

@Rycochet Thanks for suggestion.

Unfortunately, resources are not freed properly. But it did have an effect. Apparently, stop with true does remove the node, but something is still hanging around. I can comment and uncomment the animation calls and, at will, create/eliminate the memory leak.

Surely you guys would know how to release the very resources you're allocating? Please give this top priority, as time permits, of course.

Thank you.

@Rycochet
Copy link
Collaborator

Rycochet commented Apr 4, 2017

@estaylorco I'm the only coder maintaining Velocity, and I still don't know exactly how it does everything - if it was in Typescript it'd be simple to work out, but Javascript doesn't have interfaces or type tracking so it's non trivial trying to follow things in there - it's also one monolithic file that needs to be split up. I do what I can when I have time, but some months are busier than others ;-)

@estaylorco
Copy link
Author

estaylorco commented Apr 4, 2017

@Rycochet Oh, no problem. That's why I indicated "...as time permits..." Even if it's months and months from now, fixing this problem will be most welcome 👍 . I'll check back come the end of August, if that's O.K. with you.

I did switch to Aurelia's CSS-based animator for now, but will be most happy to switch back to Velocity.

@zewa666
Copy link

zewa666 commented Apr 6, 2017

@Rycochet thanks for your efforts, it is much appreciated 👍

@Rycochet
Copy link
Collaborator

In Velocity 2 the per-Element data is stored in a WeakMap, so it will be freed when the Element is removed from the DOM and released. THe list of animations is also no longer a list so frees each one as they finish.

@Rycochet Rycochet self-assigned this Oct 24, 2017
@Rycochet
Copy link
Collaborator

Changed to storing the data against the element itself, without any internal references (was slightly faster and made it easier to manage).

@zewa666
Copy link

zewa666 commented Nov 23, 2017

@estaylorco would be great If you could give this a Test drive and see If your issues are gone

@Rycochet
Copy link
Collaborator

@zewa666 @estaylorco Just remember that this is related to the develop branch - which is unlikely to be fully released until late December at the earliest.

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

No branches or pull requests

3 participants