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

don't rely on Events? #763

Closed
pixelzoom opened this issue Apr 5, 2018 · 4 comments
Closed

don't rely on Events? #763

pixelzoom opened this issue Apr 5, 2018 · 4 comments
Assignees

Comments

@pixelzoom
Copy link
Contributor

I'm working on an issue related to dispose, and I ran across this comment in Node.dispose:

4892 Events.prototype.dispose.call( this ); // TODO: don't rely on Events

What does "don't rely on Events" mean? Is there a problem with Events.dispose?....

@jonathanolson
Copy link
Contributor

It's basically referring to #490, where things like Emitter would be strictly better for performance and usability.

@pixelzoom
Copy link
Contributor Author

Events.dispose currently does nothing. Should it remove all of its listeners, similar to Property.dispose? That would eliminate the need for tricky/buggy logic like I encountered for Dialog while fixing phetsims/vegas#61.

@pixelzoom pixelzoom assigned jonathanolson and unassigned pixelzoom Apr 5, 2018
@jonathanolson
Copy link
Contributor

I have no strong preferences on it. If it's helpful to wipe the listeners, go ahead.

@pixelzoom
Copy link
Contributor Author

If I wipe listeners, I suspect that's going to cause problems throughout PhET code. I typically check hasListener before calling off, but that doesn't seem to be general practice. So I'm not going to make that change. But something to consider if we ever get around to replacing Events with Emitter.

Closing.

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

2 participants