f / delorean

An Agnostic, Complete Flux Architecture Framework
749 stars 40 forks source link

Fix shared state between stores #60

Closed dannymidnight closed 9 years ago

dannymidnight commented 9 years ago

Currently stores are sharing state which causes issues in the test environment and forces state to be manually reset between test cases. This probably isn't an issue in a production environment where you would generally have a single store.

This PR adds a spec to demonstrate the issue and the appropriate fix.

darcyadams commented 9 years ago

Thanks for the PR! We definitely need better test coverage, so I appreciate the help.

Why not just use lodash as a devDependency and _.clone test case store objects before feeding them to createStore? I'm not necessarily opposed to this approach, but it just strikes me as a problem we should solve in the tests rather than the framework. Stores are essentially singletons (except when testing), so I don't see much production benefit here, am I missing something?

dannymidnight commented 9 years ago

Passing the cloned object into createStore won't really fix the issue for me because they're already created before I can importing them in.

eg.

var MyStore = require('./stores/MyStore');
describe('X', function() {
    beforeEach(function() {
        var store = MyStore();
    });
    it('..');
    it('..');
});

Or maybe I'm missing something!? :smile:

lwc commented 9 years ago

Stores are essentially singletons

This feels like an unnecessary limitation - sure, there are likely only ever going to be single instances of stores, but enforcing that they are singletons adds a constraint that rules out several usage patterns.

Also the current behaviour essentially breaks the new keyword, looking at the example in the readme

var Store = Flux.createStore({
  data: null,
  setData: function (data) {
    this.data = data;
    this.emit('change');
  },
  actions: {
    'incoming-data': 'setData'
  }
});
var store1 = new Store();
var store2 = new Store();
// store1 === store2, new keyword broken :(

The dispatcher has reference to stores by instance anyway, so there is no advantage in this masked singleton approach.

darcyadams commented 9 years ago

alright, I'll buy that argument. Just want to make sure we are not misplacing the solution here.

thanks @dannymidnight & @lwc!

:+1:

dannymidnight commented 9 years ago

Awesome cheers @darcyadams :grin: