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

Use the public on method when listening #3615

Merged
merged 3 commits into from
Apr 9, 2017

Conversation

jridgewell
Copy link
Collaborator

Fixes #3611.

This uses a private listening var to share state between a Backbone
"listener" and "listenee", instead of using a private internalOn() to
share state. This allows #listenTo to use the public #on method and
keeps interop between Backbone and any other event library.

http://jsperf.com/internal-listening-public-on

@@ -149,10 +146,20 @@
}

// Bind callbacks on obj, and keep track of them on listening.
internalOn(obj, name, callback, this, listening);
var error = tryCatchOn(obj, name, callback, this);
listening = void 0;
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why is this necessary - won't it be gced anyway

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It's now a private global.

@jridgewell
Copy link
Collaborator Author

Hold off on this for today, it has an issue because the other library won't update listening.count, so we don't know when we can cleanup references to listenee from the listener.

@jashkenas
Copy link
Owner

"The other library" ?

Also, tryCatchOn isn't a good thing.

@jridgewell jridgewell force-pushed the private-listening-public-on branch 2 times, most recently from ecdaed6 to b076de8 Compare May 20, 2015 17:24
This uses a private `listening` var to share state between a Backbone
"listener" and "listenee", instead of using a private `internalOn()` to
share state. This allows `#listenTo` to use the public `#on` method and
keeps interop between Backbone and any other event library.
@jridgewell jridgewell force-pushed the private-listening-public-on branch from b076de8 to 4bd7ec8 Compare May 20, 2015 17:25
@jridgewell
Copy link
Collaborator Author

I've just revised the implementation to use a hybrid approach between #3455 and #3594.

If the listenee is using Backbone's #on, the listening will keep track of callbacks using a fast counter (#3594's approach). If not, then callbacks will be stored in an _events object on the listening object (#3455). This allows full interoperability while also properly cleaning up memory bindings when all callbacks have been removed.

JsPerf tends to like the implementation, except Chrome doesn't like #listenTo (running the same benchmark in node v0.12 shows a less than 0.2% slowdown).


Also, tryCatchOn isn't a good thing.

Can you explain?

A more descriptive name for what the property signifies.
@DomBlack
Copy link

DomBlack commented Jun 8, 2016

Is there any chance this PR will be merged?

@alexbaumgertner
Copy link

alexbaumgertner commented Jun 10, 2016

@DomBlack +1
I'm also interested in this issue. This PR could fix at least one bug in my project.
(Simple demo: http://jsbin.com/yubuhe/6/edit?html,output)

@jridgewell Do you need any help?

@alexbaumgertner
Copy link

@jridgewell I tried to rebase and fix merge errors in #4043 but a bunch of tests fails. Could you help me?

@jbboehr
Copy link
Contributor

jbboehr commented Apr 5, 2017

Any updates on this? I've merged master into it in #4131, if that helps at all.

@jbboehr jbboehr mentioned this pull request Apr 8, 2017
@megawac megawac merged commit b958517 into jashkenas:master Apr 9, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

listenTo change in behavior, 1.2.0?
6 participants