YahooArchive / fluxible-router

MOVED TO FLUXIBLE REPO
104 stars 26 forks source link

Pass route action's arguments to navigateAction's done callback #101

Closed SiAdcock closed 8 years ago

SiAdcock commented 8 years ago

It would be useful to propagate the route action's arguments through to navigateAction's done callback.

============
routes.js
============

module.exports = {
  thing: {
    path: '/thing',
    method: 'post',
    action: function(context, payload, done) {
      var res = payload.getIn(['navigate', 'body', 'foo']); // res === 'bar'
      done(null, res);
    }
  }
};

============
postThing.js
============

var payload = {
  url: '/thing',
  method: 'POST',
  body: { 
    foo: 'bar' 
  }
};

context.executeAction(navigateAction, payload, function(err, res) {
  if (err) {
    // throw err
  } else {
    // do something with `res`
  }
});

Currently, res is undefined in the above snippet

mridgway commented 8 years ago

I wouldn't recommend using actions to send data back up the chain. Actions have callbacks only so that we know when they have completed, not to pass data around between them. The stores should be the source of truth for any data. In the parent, you could call context.getStore(RouteStore).getCurrentRoute() to get access to that information.

yahoocla commented 8 years ago

CLA is valid!

SiAdcock commented 8 years ago

The documentation for FluxibleContext states that the callback for an executeAction call may receive error and result parameters. If this is true of the FluxibleContext, is it not also true for the Action Context? Or have I misunderstood the intention?

http://fluxible.io/api/fluxible-context.html#executeactionaction-payload-callback

mridgway commented 8 years ago

You're right it does mention that. We've found that this pattern has caused inconsistency as far as what the source of truth is. We will probably remove that from the documentation, although we will not remove the capability to send data back.

With that said, I don't think we should add the pattern to fluxible-router to encourage people to use it.

SiAdcock commented 8 years ago

I see, thanks for clarifying, I agree now that Actions should be fire and forget.

We have a small number of generic Action Creators that make calls to an API using Fetchr. These are called from our route handlers. When calling a generic Action Creator, the route handler passes in extra functionality using the done callback, which does things with the response (e.g. dispatch Actions specific to the intention of the API call).

In the cold light of day, it is clear that this is an antipattern and, having coded ourselves into a corner, we were using callbacks as an escape hatch. We'll probably ditch the generic Action Creators for making Fetchr calls, and instead encapsulate the Fetchr call and response handling in the route handlers.

Cheers for your explanations.