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

Already on GitHub? Sign in to your account

Y.Node._instances breaks GC #1637

Open
wants to merge 7 commits into
from

Conversation

Projects
None yet
2 participants
Contributor

jafl commented Feb 12, 2014

This picks up where the old Ticket #2532270 left off.

Breaking the GC model drags us back to manual memory management. IE6 and IE7 force us to do this anyway, but YUI should not force this upon developers who choose to not target these older browsers.

Another example: If I dereference a Node instance and pass it to a 3rd party widget that operates on raw DOM elements, then even if that widget cleans up correctly, the DOM node won't get cleaned up as long as the Node instance still exists.

By storing the Y.Node reference on the DOM element instead of in Y.Node._instances, then none of this is a problem.

Contributor

jafl commented Feb 13, 2014

The failed build is due to Charts. I think that must also be in dev-master, not related to my changes.

Member

juandopazo commented Feb 13, 2014

Sorry for rehasing. If the goal is to only fix this in modern browsers, can we just use a WeakMap and slowly get the new behavior as browsers implement it? It's a lot simpler than keeping a flag everywhere and it's already there in Firefox and IE11.

Contributor

jafl commented Feb 13, 2014

My understanding of WeakMap is that (1) the keys are objects and (2) only the keys are subject to garbage collection. Since the keys in Y.Node._instances are strings, WeakMap doesn't sound applicable.

Member

juandopazo commented Feb 13, 2014

But the idea is to map DOM Nodes to YUI Nodes. When using the WeakMap you'd just use the DOM node as the key.

Contributor

jafl commented Feb 13, 2014

Doh. You're right. However, I'm not sure the code will be any simpler, because you'll still have to test whether or not WeakMap is available. And it looks like it may still be a while before it's available in enough browsers to be reliable. If it's only available in some browsers, it's no help, because you still have to keep doing manual memory management. My patch fixes it for everything except IE6 and IE7, which are already basically dead.

Member

juandopazo commented Feb 13, 2014

I think your implementation is fine (storing the YUI node in the DOM node), but I still think we should be using a WeakMap as the correct future-proof solution.

Contributor

jafl commented Feb 13, 2014

I agree that WeakMap is the ideal long-term solution.

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