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

Throw error when stores are chained in a circular loop #26

Closed
spoike opened this issue Aug 1, 2014 · 9 comments · Fixed by #34
Closed

Throw error when stores are chained in a circular loop #26

spoike opened this issue Aug 1, 2014 · 9 comments · Fixed by #34
Milestone

Comments

@spoike
Copy link
Member

spoike commented Aug 1, 2014

Story

As a developer using reflux
When I accidentally chain stores in a circular loop
Reflux.listenTo should throw an error notifying of the circular dependecy

Do some listener dependency tracking on actions and stores to make sure that developers don't accidentally do circular dependency and have an indication where it went wrong.

@dashed
Copy link
Contributor

dashed commented Aug 1, 2014

Maybe have set of recipients that received the emit request have been passed to? The first store that receives an emit will create and sign the recipient set with its own instance and then pass onto the next store. If a store finds its 'signature' (instance) on the recipient set, then there's an illegal cycle.

@dashed
Copy link
Contributor

dashed commented Aug 1, 2014

Instead of a store's instance, have an integer id. createStore would just assign and increment an integer id for every new store.

@spoike
Copy link
Member Author

spoike commented Aug 1, 2014

Sounds like you have a plan @dashed 👍 ;-)

@spoike
Copy link
Member Author

spoike commented Aug 1, 2014

The test is basically testing that you can't chain two stores in a circular loop and also that you can't chain the store with itself (which is the tiniest loop possible). Sample:

// throwingErrorDuringCircularLoop.spec.js or something
describe('when a store is created', function() {
    var store1;

    beforeEach(function() {
        store1 = Reflux.createStore({});
    });

    it('should fail when it listens to itself', function() {
        store1.listenTo(store1, _.noop);
        // assert that error is thrown, which I don't remember at the moment
        // how you do it in chai
    });

    describe('and another store is created', function() {
        var store2;

        beforeEach(function() {
            store2 = Reflux.createStore({});
        });

        it('should fail when both listen to each other', function() {
            store1.listenTo(store2, _.noop);
            store2.listenTo(store1, _.noop);
            // assert that error is thrown
        });

    });
});

@spoike spoike added this to the 0.1.6 milestone Aug 1, 2014
@spoike
Copy link
Member Author

spoike commented Aug 1, 2014

I'm adding this feature for 0.1.6 milestone, together with #27 and #28, so Reflux will be in feature parity with facebook/flux

@spoike
Copy link
Member Author

spoike commented Aug 1, 2014

Let me know if you want to take this one @dashed. If not I'll probably jump on it when I have the time in the weekend.

@dashed
Copy link
Contributor

dashed commented Aug 1, 2014

@spoike You can take this one. I won't have time to look at it till next week Tuesday.

@spoike
Copy link
Member Author

spoike commented Aug 4, 2014

Been too busy with IRL stuff this weekend so I haven't had the time to look into this. Anyone is free to take a gander at it... if nobody does then I'll notify if I start working on this.

@spoike
Copy link
Member Author

spoike commented Aug 5, 2014

I have a solution now. Need to integrate with Reflux.all in #28 and it'll be done.

@spoike spoike mentioned this issue Aug 5, 2014
@spoike spoike closed this as completed in #34 Aug 6, 2014
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants