reflux / refluxjs

A simple library for uni-directional dataflow application architecture with React extensions inspired by Flux
BSD 3-Clause "New" or "Revised" License
5.36k stars 330 forks source link

Asynchronous dataflow example #57

Closed simenbrekken closed 10 years ago

simenbrekken commented 10 years ago

It would be great with an example on how to deal with asynchronous data sources along with optimistic updates in Reflux. Remote data is something everyone works with and something that Flux documents very poorly.

I suppose one way would be creating several events for each action (Todo.createItem, Todo.createItemSuccess and Todo.createItemFailure) and using the first to do optimistic updates.

spoike commented 10 years ago

I agree. This will most likely be worked on for #47.

It's a bit difficult to describe how to do it because WebAPI's tend to look different from project to project. But as a matter of fact I just wrote an example on gist in reply to a blog post for how to handle errors from your WebAPI (which in my mind are seperate data flows).

What I don't mention yet is that you also can chain data stores (i.e. data stores may listen to other data stores because they have the same listen interface as actions) to reduce/fold the data further. Something like this for "read posts":

var readPostsStore = Reflux.createStore({
    init: function() {
        this.listenTo(postsStore, this.onPostLoad);
        /* handle "this._readPosts" ... */
    },
    onPostLoad: function(posts) { 
        /* handle posts and this._readPosts, etc ... */ 
        this.trigger(this._readPosts);
    }
});
simenbrekken commented 10 years ago

Not quite sure if I'm aboard on the store listening for the results of the PostsApi request. It feels cleaner to me if the PostActions.load creates the request via PostsApi and triggers new actions based on the outcome. As for who handles debounce/cancellation of duplicate requests, I'm not sure where that belongs.

When you talk about chaining data stores, is the outlined approach also applicable for scenarioes where stores are dependent on eachothers data?

E.g: ProjectsStore contains a reference to an entry in UserStore. So in order to do ProjectsStore.getOwner() it would essentially need to do UserStore.get(project.id) and wait for the action to complete.

Lastly I've been thinking about how one would do pre-fetching of store data on the server when everything is essentially asynchronous. I've put up a quick gist with an convoluted but working approach.

spoike commented 10 years ago

Not quite sure if I'm aboard on the store listening for the results of the PostsApi request. It feels cleaner to me if the PostActions.load creates the request via PostsApi and triggers new actions based on the outcome.

I can see that approach is beneficial for testing purposes to put this request creation in a seperate module.

Alternatively you could simplify this by adding code into the action's preEmit hook that creates the request...

PostActions.load.preEmit(function() {
    /*
    1. do an ajax load
    2. call another action based on the outcome
    */
});

... it is a bit hacky (for my taste) but that should be testable... just call the preEmit hook on that action. However on the issue of debounce/cancellation... well... actions can't listen to other actions that signal cancellation, unless you hack it... but then you could just as well use a store.

As for who handles debounce/cancellation of duplicate requests, I'm not sure where that belongs.

If you're doing it in the store (or in another module), that should handle the cancellation at least inside that. E.g. let the store listen to a cancelLoad action and cancel the transaction. The pattern would like this:

var ajaxyStore = Reflux.createStore({
    init: function() {
        this.listenTo(Actions.cancelLoad, cancel);
        this.listenTo(Actions.load, load);
    },
    load: function() {
        this._xhrLike = WebApi.loadSomething().done(onLoad).error(onError);
    },
    cancel: function() {
        if (this._xhrLike) {
            this._xhrLike.cancel();
        }
    },
    /* ... onError and onLoad callbacks here */
});

Since I use lodash in most of my projects debouncing/throttling wrappers are used on "hot" functions on the components themselves. I.e. in a component:

React.createClass({
    /* ... */
    handleMove: _.throttle(function(x, y) {
        // Note: _.throttle retains the context of `this` you, i.e.
        // this is the component
    }, 200);
    onMouseMove(evt) { // the event handler for mouse movement, very "spammy"
        this.handleMove(evt.clientX, evt.clientY);
    },
    /* ... */
});

If I didn't use lodash, I'd write the wrapping factory that does this because I find them to be very easy and composable enough for reuse.

I haven't ran in to a problem where I had to debounce in a store yet.

When you talk about chaining data stores, is the outlined approach also applicable for scenarioes where stores are dependent on eachothers data?

Handling data flows in parallel is what Reflux.all is for (and Facebook's waitFor in their dispatcher), i.e. it joins emitters (actions and other stores). It is a bit limited now but hoping to fix this in #29 (it is already requested in #55).

Lastly I've been thinking about how one would do pre-fetching of store data on the server when everything is essentially asynchronous. (snip-snip... url to gist)

Sure, you can do that.

Personally I just use getDefaultData functionality (see #46) that initially before any load returns "empty data" for components listening to the stores. You may also send off promises.

I'm still investigating if this is viable for server-side rendering. Slowly getting there. ;-)

spoike commented 10 years ago

I just realized that your gist seems to be server-side code. Considering I'm still looking into it, you may disregard my answer above. :-)

simenbrekken commented 10 years ago

@spoike Thanks so much for the in-depth answer. I'm definitely going to have another look at your approach with Stores listening to load actions, since the API util can easily be mocked testing could actually be simpler than with my cascading approach.

simenbrekken commented 10 years ago

Worked bit more on ensuring stores and actions stay synchronous. The whole ProductActions.loadAll.preEmit feels very hacky. Would perhaps be better with Reflux.createAction(fn).

var ProductsApi = {
  loadAll: function() {
    return APIUtils.get('/products');
  }
};

var ProductActions = Reflux.createActions(['loadAll', 'loadAllComplete']);
ProductActions.loadAll.preEmit = function() {
  ProductsApi.loadAll().then(ProductActions.loadAllComplete);  
};

var ProductsStore = Reflux.createStore({
  init: function() {
    this._products = [];
    this.listenTo(loadAllComplete, this.onLoadAll);
  },

  onLoadAll: function(products) {
    this._products = products;
    this.trigger();
  }
});

var Products  = React.createClass({
  componentWillMount: function() {
    ProductsStore.listen(this.onProductsChange);
    ProductActions.loadAll();
  },

  onProductsChange: function(products) {
    this.setState({
      products: products
    });
  }
});
spoike commented 10 years ago

Cool.

Thinking about it you can also handle errors in the pre-emit if the APIUtils returns an error:

var ProductActions = Reflux.createActions(['loadAll', 'loadAllComplete', 'couldNotLoad']);
ProductActions.loadAll.preEmit = function() {
    ProductsApi.loadAll()
        .then(ProductActions.loadAllComplete)
        .fail(ProductActions.couldNotLoad);
};

If you could cancel/reuse the promise then that would be very cool.

The whole ProductActions.loadAll.preEmit feels very hacky. Would perhaps be better with Reflux.createAction(fn).

I was looking at preEmit in new light yesterday and I was puzzled why its action don't take any return value from the preEmit... meaning you can't use it as a map/reduce function.

I agree, that createAction should take callback. If we can find some kind of a nice interface for createActions as well, I'd be happy to have that implemented.

simenbrekken commented 10 years ago

What about:

Reflux.createActions({
  load: true,
  loadAll: function() {
    return new MagicalPromise()
  }
})
spoike commented 10 years ago

@sbrekken Thanks! I added a ticket for that: #58

(I feel a bit silly now for not coming up with something simple as using an options object)

simenbrekken commented 10 years ago

This is probably me messing things up but with actions being created on the fly you can't actually use them (without horrible hacks= in something like an API util because of the cycling dependency.

ProductActions.js

var ProductAPI = require('./ProductAPI')
var ProductActions = Reflux.createActions({
  'load',
  'loadComplete',
  'loadError'
})

ProductActions.load.preEmit = ProductAPI.load

ProductAPI.js

var ProductActions = require('./ProductActions')
var ProductAPI = {
  load: function() {
    httpPromiseDance
      .then(ProductActions.loadComplete) // undefined
      .catch(ProductActions.loadError) // undefined
  }  
}

Any ideas or is this kind of abstraction a Bad Idea™?

justin808 commented 10 years ago

Maybe there's a way to change the order or place of the "require" statements to fix the undefined. I've run into this sort of thing. The module loader uses an empty object when there's a circular dependency.

simenbrekken commented 10 years ago

@justin808 That won't work because ProductActions.loadComplete is evaluated at creation, the only way i've managed to get this working is putting the require statement in the actual load function, super ugly:

var ProductAPI = {
  load: function() {
    var ProductActions = require('./ProductActions')

    httpPromiseDance
      .then(ProductActions.loadComplete) // undefined
      .catch(ProductActions.loadError) // undefined
  }  
}
justin808 commented 10 years ago

@sbrekken Could you create a different Actions object? Or why do you need the load function to call actions at all. How about just other methods on the ProductAPI?

simenbrekken commented 10 years ago

@justin808 Good suggestion on the separate actions, that works. Though I'd like to avoid having twice the amount of action creators just because of this.

simenbrekken commented 10 years ago

@spoike I keep running into "chicken or the egg" problems with actions being functors, that is you can't require A from B and B from A without using requires inside the actual actions. This is pretty much the only place I can see the use for named action constants over functors.

nishp1 commented 10 years ago

@sbrekken: I keep APIUtils only webapi specific. See below:

ProductActions.js

var ProductAPI = require('./ProductAPI')
var ProductActions = Reflux.createActions({
  'load',
  'loadComplete',
  'loadError'
})

ProductActions.load.preEmit = function () {
     ProductAPI.load()
          .then(ProductActions.loadComplete)
          .catch(ProductActions.loadError)
}

ProductAPI.js

var ProductAPI = {
  load: function() {
    return httpPromiseDance()
  }  
}
simenbrekken commented 10 years ago

@nishp1 Good suggestion!

spoike commented 10 years ago

I was honestly a bit puzzled why there was a circular dependency inside the API parts of the code when they normally shouldn't have many dependencies (other than xhr wrappers and possibly promise libraries if that's your thing).

So yes. I'm with @nishp1 on keeping the actual API implementation separate from flux actions. I.e. they only should be concerned with Web-API specific stuff and just let actions/stores handle the responses separately as data flows.

PS. A side note: I'm under the impression that whenever you have a circular dependency happening in your code it is indicative of a brain fart and needs to be rethought, i.e. you need to get "back to the drawing board" and straighten that dependency out. I mean, I've been subject to this many times in my experience so I know the feeling. :-)

simenbrekken commented 10 years ago

@spoike Do you have any thoughts on inter-store depencies and event-chains? I recently ran into the "cannot dispatch while in the middle of a dispatch" invariant in FB's Dispatcher.

The issue is basically StoreA firing a change callback after receiving data making ComponentB mount and invoke componentDidMount that fires an action to fetch the data it needs.

I've detailed the whole issue here, but frankly the limitation strikes me as a bit odd in the real world outside todo lists and synchronous chat app examples.

spoike commented 10 years ago

That is the situation (and others similar) what getDefaultData callback in stores and the third callback parameter in listenTo is for.

https://github.com/spoike/refluxjs#sending-default-data-with-the-listento-function

I admit that it isn't well documented, but here is an example:

var StoreA = Reflux.createStore({
    init: function() {
        this._data; // cached data
    },
    getDefaultData: function() {
        return this._data;
    },
    /* 
       whenever you trigger the store's change event 
       you replace ._data with what was sent 
    */
});

var ComponentB = React.createClass({
    componentDidMount: function() {
        this.listenTo(StoreA, 
            this.onStoreBChange, 
            this.onStoreBChange); // note third param
    },
    onStoreBChange: function(data) {
        // will be called once the component did mount
        // and subsequent change events in StoreB
    }
    /* snip snip */
});
Lenne231 commented 10 years ago

What do you think about something like this? I don't think that putting things like xhr into preEmit is a good idea.

var actions = Reflux.createActions([
    'loadAll',
    'loadAllCompleted',
    'loadAllFailed'
]);

var handler = Reflux.createHandler({
    init: function() {
        this.listenTo(actions.loadAll, loadAll);
    },
    loadAll: function() {
        http.get('/api/data')
            .then(actions.loadAllCompleted)
            .catch(actions.loadAllFailed);
    }
});

var store = Reflux.createStore({
    init: function() {
        this.listenTo(actions.loadAllCompleted, loadAllCompleted);
        this.listenTo(actions.loadAllFailed, loadAllFailed);
    },
    loadAllCompleted: function(data) {
        // ...
        this.trigger();
    },
    loadAllFailed: function(error) {
        // ...
        this.trigger();
    }
});
spoike commented 10 years ago

@Lenne231 if you believe that preEmit is a bad idea then you could use the action's listen function instead with a lot less boilerplate, like this:

actions.loadAll.listen(function() {
    http.get('/api/data')
        .then(actions.loadAllCompleted)
        .catch(actions.loadAllFailed);
});

No need to implement a Reflux.createHandler. But if you really, really, REALLY wanna do that, you could do this in your initial bootstrap code (since the interface is pretty much the same even though you won't need the trigger function):

    Reflux.createHandler = Reflux.createStore; // alias
spoike commented 10 years ago

Closing this ticket for now.