plainblack / Lacuna-Web-Client

The pure javascript client for The Lacuna Expanse.
http://www.lacunaexpanse.com
Other
31 stars 37 forks source link

Stateful stores #198

Closed nataliethistime closed 8 years ago

nataliethistime commented 8 years ago

This PR implements a store mixin that allows stores to maintain their own state. We've been doing this (in various different forms) but I've now organized it all into a reusable system.

Here's an example of what it looks like:

var Reflux        = require('reflux');
var StatefulStore = require('js/stores/mixins/stateful');

var clone         = require('js/util').clone;

var MyStore = Reflux.createStore({
    mixins : [
        StatefulStore
    ],

    getDefaultData : function() {
        return {
            foo    : 'bar',
            spam   : 'eggs',
            points : 10,
            users  : [
                'Dave',
                'Bob',
                'Harry'
            ]
        }
    },

    onAddUser : function(name) {
        // Clone the store's state. See above for reasoning.
        var state = clone(this.state);

        // Modify the copied object however we deem necessary.
        state.users.push(name);

        // Send it out to whoever is listening.
        this.emit(name);
    },

    clear : function() {
        // This is common pattern for reseting a store back to its initial data.
        this.emit(this.getDefaultData());
    }
});

// Elsewhere...

function somethingThatOnlyNeedsPointsOnce() {
    // Note: only do this if you need something from the store once. Otherwise, you should
    // listen to it and handle when the data changes.
    var points = MyStore.getData().points;
}
icydee commented 8 years ago

What problem are you trying to solve here? It seems as though the only benefit is that the store does not issue a trigger if the store data has not changed? Is this a case of premature optimisation, in that the React components using store will not render unless their input state has changed? Why not just let React handle this issue? Please help me to understand why this is needed.

On a separate point, I see you re-introduced rpc/body/index.js rather than rpc/body.js. The main issue I have with this is that it is not so obvious what the filename is. Imagine, you start off with an rpc/body.js (because there are no sub-components) and you get used to that, and then you decide there are some more components so you create a sub-directory (and here is the crucial point) you move the original component into the sub-directory as index.js there is no need to do this as your code evolves. Keep it where it was. (probably this is influenced by the way that Perl does it, but I still see no advantage to the index.js files). I introduced this change in the straw-man branch for discussion, let's agree on a sensible approach.

nataliethistime commented 8 years ago

Yeah. It's premature optimization. I'll fix that.

I like the index.js thing because it keeps all "body stores" in the "body stores" directory.

The benefits I have in mind for this PR are:

Also, when a new component is mounted (let's say the essentia window opened). The new component calls getInitialState on the stores it's listening to. Before this PR, each store had to check if it had data in that getInitialState method. If it did, it should return it so that the new component gets up-to-date data. Otherwise, it should return it's initial state, as normal. StatefulStore handles this.

Finally, though not related (and could be fixed without this new mixin), all stores are now immutable. Previously, shouldComponentUpdate would get confused because both the "previous state" and "new state" were the same value every time. The clone calls around the place are fixing that.

All that said, I simply wanted to make the store stuff look prettier. There was some repeated code which I've now fixed.

icydee commented 8 years ago

It's good to factor out repeated code, and thanks for explaining the other stuff.

One other thing occurs to be, is the 'clone' function a deep clone or shallow? It should be deep.

Hmm, I still don't like that index.js thing, can we compromise? You can have all the other stuff if you undo the index.js approach? (As I said, it means as we add sub-components we have to move body.js into body/index.js and actually, body is not a component of itself, it's a parent of the sub components so should not be in the same directory) It's probably just how we look at this differently.

nataliethistime commented 8 years ago

Clone function is deep.

(That's way deep, maan!)