Child (async) actions and promise handling

[willembult]

Async actions

There has been some discussion on asynchronous actions (#57, #103) and where to best handle them.

The consensus seems to be that triggered execution is best kept outside of the stores, and I personally agree. In my opinion, the store shouldn't worry about how actions are run, focusing merely on their results. It also makes testing easier, as a few folks have noted.

The pattern proposed in the update on v0.1.8 links the execution of asynchronous code to the preEmit hook of an action, after which it triggers events on "Completed" and "Failed" actions, like so:

var ProductAPI = require('./ProductAPI');  
// contains `load` method that returns a promise

var ProductActions = Reflux.createActions({  
    'load',         // initiates the async load
    'loadComplete', // when the load is complete
    'loadError'     // when the load has failed
});

// Hook up the API call in the load action's preEmit
ProductActions.load.preEmit = function () {  
     // Perform the API call, get a promise
     ProductAPI.load()
          .then(ProductActions.loadComplete)
          .catch(ProductActions.loadError);
};

Issues with preEmit

While this is definitely the cleanest way I've seen, some things still bothered me about it:

• There is no semantic link between the "load" and "loadComplete" / "loadError" events. Nothing in the interface reflects the fact that they are related.
• Putting an API call, or some other asynchronous code, in the preEmit hook seems hacky to me. Others have expressed similar sentiments (Asynchronous dataflow example #57).
• I think it's overly verbose. There's still some unnecessary boilerplate here, and for every async event (of which there can be loads) we have to now manually define 3 separate actions. If at some point we want to add a "Canceled" action as well for all our calls, we'll be doing busy-work for a while.

Proposal: Child actions

It appeared to me that if we could define child actions (attached to a parent action), we could design a much nicer interface. This commit is an implementation of that idea. The general interface looks as such:

// ProductActions.js

var ProductAPI = require('./ProductAPI');  
// contains `load` method that returns a promise

// Define action with child actions
var ProductActions = Reflux.createActions({  
    'load': {children: ['completed','failed']}
});

// And here is how you would hook it up to a promise
ProductActions.load.listen( function() {
    // Bind listener context to action by default
    ProductAPI.load()
        .then(this.completed)
        .catch(this.failed);
});

// ProductStore.js

var ProductActions = require('./ProductActions');

// This is how you would listen for a completed load
ProductActions.load.completed.listen(function(result) {
    // do something with result
});

// listenToMany (and ListenerMixin) will still work intuitively
var Store = Reflux.createStore({
    init: function() {
        this.listenToMany(ProductActions);
    },
    onLoad: function(){
        // product load started
    },
    onLoadCompleted: function(product){
        // product load completed
    }
});

Common-case helpers

I figured it would make sense as well to provide some helper methods and options to make the 80% case easier. This works as such:

// Define actions with async option to auto-create
// 'completed' and 'failed' child events
var ProductActions = Reflux.createActions({  
    'load': {async: true}
});

// Hook up standard async child functors
// to a promise easily:
ProductActions.load.listen( function() {
    // Have the action attach callbacks to the promise
    // and pass on data to "complete" and "failed" actions
    ProductActions.load.promise(ProductAPI.load());
});

// Or even easier for the 80% case, a method that
// hooks ups callbacks to a returned promise directly
ProductActions.load.listenAndPromise( function() {
    return ProductAPI.load();
});

Alternative uses

For custom cases, you could create alternative children events:

var ImageAPI = require('./ImageAPI');

// If we need custom child actions, we could do so:
var ImageActions = Reflux.createActions({
    'pictureUpload': {children: ['progressed','completed']}
});

// Hook things up to a non-promise-based API
ImageActions.pictureUpload.listen(function(){
    ImageAPI.upload({
        onProgress: this.progressed,
        onSuccess: this.completed
    });
});

Notes

Note, while createActions now operates on an associative Object, the interface remains backwards-compatible and accepts Arrays as well.

This has cleaned up my action definitions quite a bit. Curious to hear your critical thoughts / feedback! Thanks!

[WRidder]

I like this approach. It's essentially exactly the same as I have been doing already, see:
https://github.com/WRidder/react-spa/blob/master/src/actions/resourceActions.js
However, now you have a semantic link between the actions.

[spoike]

👍 nice idea

To be honest array as parameter in createActions was a mistake. Been thinking about using the arguments array instead, makes more sense with parameter object now as actions become increasingly complex.

[willembult]

@spoike interesting, what would the interface look like using arguments?

[spoike]

@willembult This is a side note and not in scope for this PR. However what I mean is this is currently:

Reflux.createActions(['action1', 'action2']);

How it really should've be done, without using array, just have a "varlist" instead:

Reflux.createActions('action1', 'action2');

Implementation inside Reflux wouldn't change a lot, just use the whole arguments array instead of the first argument as array.

[willembult]

Right, that makes sense. I guess what I meant to ask was, what would that look like if we want to pass options to actions (like in this PR)?

[spoike]

I think having an options object is fine as this PR has it, since each key represents an action.

@WRiddler is the semantics between child and parent actions any way bad idea? I think it's an okay concession for ajax-y actions and completely optional.

The only thing missing in this PR is an update on README file.

I'll review the code changes more closely when I get back home. What I've seen so far looks OK, as long as the promise helper avoids to pull in any particular 3rd party library. Should we add a test with alternative libraries such as bluebird?

[WRidder]

@spoike No, it's not a bad idea at all imo. Prevents some boilerplate. I'm in favor of the common-case helpers as well.

[willembult]

This was confusing, but legit:

Reflux.createActions({  
    'load': {async: true, sync: true}
});

So I renamed the option to asyncResult instead.

[spoike]

(Disclaimer: I just couldn't resist the opportunity for a meme...)

[andrew-d]

This seems like a really cool idea for how to handle server communication and async code in general. +1 from me - looking forward to being able to use this!

[VinSpee]

+1 on these. Sugar like this will keep this project on top! Thanks for the hard work!

[jsdir]

👍

[spoike]

@willembult will you write something about it in the README or would you like someone else to do it?

[willembult]

Sure, I'll do it. Pretty slammed right now, but I'll look at it this weekend.

—
Sent from Mailbox

On Fri, Dec 5, 2014 at 12:09 AM, Mikael Brassman notifications@github.com
wrote:

@willembult will you write something about it in the README or would you like someone else to do it?

Reply to this email directly or view it on GitHub:
#140 (comment)

[dashed]

Probably better to check completed and failed are last two elements in this.children.

[willembult]

What about this then?

createActions({
    myAction: {'children': ['completed','failed','progressed']}
});

[dashed]

Wouldn't that be concatenated with ['completed','failed'] here?

https://github.com/spoike/refluxjs/pull/140/files#diff-52ace21827ea0130a413a22cd1697dc8R38

[willembult]

Only if you use the "asyncResult" option. So you could use this:

createActions({
    myAction: {'children': ['progressed'], asyncResult: true}
});

and it would append ['completed','failed'] because of asyncResult.

But you could also do this:

createActions({
    myAction: {'children': ['completed','failed','progressed']}
});

in which case it can still handle promises, but since we've explicitly defined the child actions, they don't appear last in the list.

[dashed]

Oh I see. I misunderstood.

[dashed]

Would a shorthand be a possibility?

var ImageActions = Reflux.createActions({
    'pictureUpload': {children: ['progressed','completed']}
});
// above and below are equivalent
var ImageActions = Reflux.createActions({
    'pictureUpload': ['progressed','completed']
});

[andreypopp]

I don't understand

• how this reduces boilerplate — the code before and after has identical LoC
• how such "semantic link" between actions helps in application design

This increases API surface quite a lot providing no benefits.

[dashed]

I don't like this this.completed and this.failed convention. Can we just stick to node.js callbacks?

Also, I think listenAndPromise is completely redundant. Just stick to listen.

Then have:

// callback as param
ProductActions.load.listen(function(cb) {
    ProductAPI.load()
        .then(cb.bind(this, null))
        .catch(cb); 
});
// or just return a promise
ProductActions.load.listen(function() {
    return ProductAPI.load();
});

gulp does this already for quite a while:
https://github.com/gulpjs/gulp/blob/master/docs/API.md#async-task-support

[dashed]

To elaborate, I'm thinking of this as an alternative:

var ProductActions = Reflux.createActions({  
    'load': {asyncResult: true, callback: 'completed', children: [...]}
});

ProductActions.load.completed.listen(function(result) {
    // do something with result
});

// listenToMany (and ListenerMixin) will still work intuitively
var Store = Reflux.createStore({
    init: function() {
        this.listenToMany(ProductActions);
    },
    onLoad: function(){
    },
    onLoadCompleted: function(err, result){
    }
});

callback option (or call it callbackName?) defines the name of the "callback child action".

[willembult]

@dashed

Also, I think listenAndPromise is completely redundant. Just stick to listen.

Sure, you could still use listen. listenAndPromise is a completely optional helper method that reduces a small amount of boilerplate.

To elaborate, I'm thinking of this as an alternative

Can you elaborate on why you'd prefer this? It's slightly more conventional and less explicit, and seems to me like it wouldn't reduce any LoC, because now we'll need to do a conditional inside the onLoadCompleted.

In my mind, the data flowing from a completed load and a failed load are inherently different, so we should treat them as separate data flows and thus have separate handlers.

[willembult]

@andreypopp

I don't understand

how this reduces boilerplate — the code before and after has identical LoC
how such "semantic link" between actions helps in application design

Fair enough, I guess if you're not using any of the helper functions, then you'll have similar LoC. However, when you do use those, this:

var ProductActions = Reflux.createActions([
    'load',
    'loadCompleted',
    'loadFailed'
]);
ProductActions.load.preEmit = function () {  
     // Perform the API call, get a promise
     ProductAPI.load()
          .then(ProductActions.loadComplete)
          .catch(ProductActions.loadFailed);
};

gets reduced to this

var ProductActions = Reflux.createActions({
   'load': {asyncResult: true}
});
ProductActions.load.listenAndPromise( ProductAPI.load );

The "semantic link", for lack of a better term, allows us precisely to provide those helper methods / options. Without having child actions, those helper methods would depend on other actions outside of their scope, which would be super messy.

Async actions are super commonplace, so I think it's important to have easy facilities for them. Some even incorrectly reach the conclusion that Reflux doesn't cater well to async functionality, because we currently lack such explicit patterns.

[scott-coates]

👍 Really excited for this to get deployed in the v0.2.2 release!

[jsdir]

Is there any way for the child actions to access the arguments that the parent action was called with?

[ericclemmons]

Congrats to everyone, particularly @willembult & @spoike, for this update!

I'm modifying a large codebase with this support now. Can someone post a concise example on how this affects stores?

Currently, I've been using this helper:

Reflux.StoreMethods.promise = function(action) {
  var args = Array.prototype.slice.call(arguments, 1);

  return new Promise(function(resolve) {
    this.listen(resolve);

    action(args);
  }.bind(this));
};

// MyActions.js
Reflux.createActions(['fetch', 'fetchSuccess', 'fetchError']);

// MySore.js
Reflux.createStore({
  onFetch: function(query) {
    MyApi.search(query, function(response) {
      return response.ok ? MyActions.fetchSuccess(response.body) : MyActions.fetchError(response.error);
    });
  }
});

And consume it via:

MyStore.promise(MyActions.fetch, { id: 123 }).then(function(results) {
  console.log('Promised results:', results);
});

So, naturally, I just got the error Error: Cannot override API method promise in Reflux.StoreMethods, but I'm still wrapping my head around the changes.

Thanks!

[damassi]

Felt the need to comment here if anyone else runs across this as I've spent the last hour debugging a non-issue with this new Reflux feature: When using the 6to5 compiler with ES6, children callbacks will never get called if using the => function form:

ResponseActions.load.listen(() => {
  Api.fetch()
    .done( this.completed )
    .error( this.failed );
});

Though the former form works perfectly:

ResponseActions.load.listen(function() {
  Api.fetch()
    .done( this.completed )
    .error( this.failed );
});

Feel free to delete this comment if totally out of place!

[jsdir]

@damassi This is only due to the lexical binding in ES6.

[damassi]

Thanks @jsdir -- I wasn't aware that this was going to be implemented, thinking instead that it was a bug in the compiler and that => was equivalent to CoffeeScript's ->.