spoike/refluxjs — pull request #85 by krawaller (2014-09-25 21:06:28+00:00)
Title: fix circular check

I was pondering #80 and #82, trying to track down where I screwed up during the refactoring. Conclusion: I didn't! :)

The circular check was flawed from the beginning. Here's some code from `Store.prototype.listenTo` in `0.1.7`, which I moved to `ListenerMethods.validateListening` in `0.1.8`:

```javascript
if (this.hasListener(listenable)) {
throw Error("Store cannot listen to this listenable because of circular loop");
}
```

There are two problems in play here.

First, the name `hasListener`. What `hasListener` really tests is if we are already listening to the listenable. Thus a better name would be `isListeningTo`.

Given that, it's easier to spot that we're actually doing the wrong check with `this.hasListener(listenable)`! If I'm already listening to the listenable, that's not really a problem. At worst I'll be triggered twice, but it doesn't mean a circular dependency.

What we *should* be checking for is `listenable.hasListener(this)`, since that could mean a circular loop of emits. If the listenable is listening to me, it would be dangerous for me to listen to him.

We must also check if `listenable` actually has a `hasListener` method, since it need not be a listener (which corresponds to the filtering out of actions in your fix).

This PR fixes the check, and also the corresponding tests which are logically wrong in the same way. It also removes the `_isAction` flag as that is no longer needed.

The PR does NOT rename `hasListener`, or change the pirate version of `hasListener` in `Reflux.all`, as that would mean a merge conflict with #84 which reimplements `Reflux.all` as a store, which removes the faux `hasListener`.

I recommend merging this and #84, and then renaming `hasListener` to `isListeningTo` in a new PR.