Support Promises in actions

[mridgway]

Support handling promises that are returned from actions instead of using callbacks.

[cesarandreu]

I'm pretty interested. Currently I'm using a plugin that I wrote to overwrite context.executeAction. I think it's mostly backwards compatible; I think the only case where it's not backwards compatible is when you're using the arguments object.

I've been abusing co and generators, so feel free to ignore that part. From my actions I could just export co.wrap(action) instead to achieve the same goal.

action-plugin.js

var co = require('co'),
  debug = require('debug'),
  log = debug('ActionPlugin'),
  isPromise = require('is-promise'),
  isGeneratorFunction = require('is-generator-function')

module.exports = function ActionPlugin () {
  return {
    name: 'ActionPlugin',
    plugContext
  }
}

function plugContext (opts, context) {
  context.executeAction = executeAction.bind(context)

  return {
    plugComponentContext,
    plugActionContext
  }

  function plugActionContext (actionContext) {
    actionContext.executeAction = context.executeAction
  }

  function plugComponentContext (componentContext) {
    componentContext.executeAction = componentContextAction
  }

  function componentContextAction (action, payload) {
    context.executeAction(action, payload).catch(err => {
      context.executeAction(context._app._componentActionHandler, {err}, () => {})
    })
  }
}

function executeAction (action, payload={}, done) {
  log(`Executing action ${action.displayName || action.name} with payload ${payload}`)

  var promise
  if (isGeneratorFunction(action)) {
    promise = co(action(this.getActionContext(), payload))
  } else {
    promise = new Promise((resolve, reject) => {
      var result = action(this.getActionContext(), payload, (e, r) => e ? reject(e) : resolve(r))
      if (isPromise(result)) {
        result.then(resolve, reject)
      } else if (action.length < 3) {
        resolve(result)
      }
    })
  }

  if (done)
    promise.then(result => done(null, result)).catch(err => done(err))

  return promise
}

If it's a generator function, call the action with action context and payload, and pass it to co.
Otherwise call the action with action context, payload, and a callback inside of a promise. If a promise is returned then wait for that. If it has less than three params assume its sync, and resolve with the returned value. If the the action has 3 params then it will have received the callback, which will also resolve the promise. Finally, if the action has less than 3 params and it throws, I think the promise will catch it and it'll get rejected.

If execute action got called with a third param, it'll also call done. One of the nice perk of using promises is that you don't have to worry about releasing zalgo.

Would you be interested in a PR or did you have a different approach in mind?

[mridgway]

Yes, we'll absolutely take a PR.

I'm not a big fan of adding co as a dependency though, so maybe we leave that part out for now.

[cesarandreu]

Oh, definitely. co is too opinionated for something like this, I think just supporting promises is fine.

Should it use the global Promise and assume that anyone that's using this will either polyfill it or replace it with their own preferred implementation?

[mridgway]

Good question. For now let's use Bluebird. Once we start using babel to compile Fluxible for ES6, we will be able to drop that dependency.

[mridgway]

Hi @cesarandreu we've migrated the docs over to this repo so if you want to add some documentation we can get this merged.

[quicksnap]

👍 👍 👍 👍 =)

[cesarandreu]

I'll try to write up the docs tonight, thanks! :D

[igorbt]

IMHO, having callbacks and/or promises in actions is breaking Flux unidirectional data flow. According to a Facebook developer post:

1. Actions don't have callbacks, as they are by design fire-and-forget.

Why Fluxible did this "correction"?

[jedireza]

@igorbt your observation is correct. In order to render server-side, we need to know when the page is ready to render. And we do that with callbacks. However, we do keep the flux flow with actions fired from the componentContext (from your React views)... those are still fire-and-forget (no callbacks).

[mridgway]

@igorbt There is a description of what @jedireza mentioned in the Components documentation.

[igorbt]

Thanks, @jedireza , @mridgway ! I understand now. It would be nice to see this explanation in the Actions documentation also.
Btw, is it legit to pass parameters in the action callback function then? I was tempted to do so, but if the only purpose of the callback is to know when to render, then it probably would be wrong.

[cesarandreu]

Haven't had a chance to update the docs yet.

@igorbt:
In my PR, the action promise will be resolved with the callback value e.g. done(null, value), the returned promise's resolve value e.g. return Promise.resolve(value) , or if the action function has length below 3, it'll resolve the action promise with the returned value, e.g. return value

I think triggering actions inside of actions is fine as long as you make sure to wait for them in your parent action.

[mridgway]

@igorbt Good call out. I added more information to the actions doc.

[igorbt]

I'm just starting to explore Flux and Fluxible and the thing with calling another action from an action just doesn't feel right for me. I re-read the original Facebook Flux description but didn't find anything for or against this practice. I didn't find anywhere else some strong opinion about that.

If Action1 calls Action2 and then dispatch Payload1, and in the same time Action2 dispatches Payload2, if feels like cascading updates. But Flux is trying to minimize that!
The thing seems to me even worst when Action2 returns Payload2 as value (in callback or promise) and Action1 then uses Payload2 to construct Payload1. Then Payload1 is derived data. It seems more like a Store responsibility.

[quicksnap]

While the actions may be asynchronous, there's nothing wrong with nearly-simultaneous dispatches--the stores should handle events and their payloads discretely, only being concerned with potentially waiting for other stores to handle an event first (using waitFor).

This isn't cascading, only asynchronous. It would be cascading if the stores were to trigger an action, or a component doing so without any user interaction.

Does that clarify at all? (Disclaimer: I do not work on Fluxible)

[quicksnap]

Furthermore, IMO it's very helpful for actions to set up other actions on intervals, for situations like polling.

[cesarandreu]

Updated my PR with docs!

[mridgway]

Awesome! I'll check it out soon.