Async actions

[ivan-kleshnin]

I'm not sure what does "Completely synchronous" on the title page means.

How about async actions?

async function fetch() {
   return await Axios.get("/some/api")...
}

P.S. Current API looks cute, I like it.

[malte-wessel]

Hey Ivan, thanks!

Completely synchronous means that the API does not have asynchronous methods. Actions can therefore not handle returned Promises. That was a design decision. You are maybe familiar with flummox where actions can return Promises. The Issue with that is that stores that handle the (async) actions need to take care of the whole flow and intermediate states. With flummox you would register an async action by defining a beginHandler, successHandler and a failureHandler. Once the action is invoked, the store itself is responsible for handling the request to your API. I think that this is beyond the scope of stores. Actions should take care of anything that goes into or leaves your application.

With minimal-flux you would do something like this:

class Messages extends Actions {

  fetch() {
    this.wait();
    Axios.get("/some/api").then(this.complete).catch(this.fail);
  }

  wait() {
    this.emit("wait");
  }

  complete(data) {
    this.emit("complete", data);
  }

  fail(err) {
    this.emit("fail", err);
  }
}

This is probably a bit more code than just returning a Promise, but you have more control over the flow. You maybe want to inform other parts of your application that you are waiting for a request. Inside the wait action you could invoke another action, for example a global loading indicator:

wait() {
  // Inside of actions you have access to all other actions that were registered.
  // Maybe there are multiple requests going on and you want show the user a progress bar.
  this.actions.loader.increment();
  this.emit('wait');
}
complete(data) {
  this.actions.loader.decrement();
  this.emit('complete', data);
}
fail(err) {
  this.actions.loader.decrement();
  // If something goes wrong, you maybe want to log it:
  this.actions.logger.add(err);
  this.emit('fail');
}

You can find another example here

Hope that helps :)

[ivan-kleshnin]

Thanks. Yes, I'm familiar with Flummox. I've been trying about all flux implementations out there :) What I don't like about Flummox is it's top-level API. let actionIds = flux.getActions("xxx") is too noisy for me. You've done a great job by replacing this with properties.

Also your flux == dispatcher. That's good, less concepts to confuse.

You're trying to keep visible and obvious the fact that actions are just event emitters. Can't say for sure, whether I like it or not. There are a lot of caused duplications, at least on the first sight. Can action function emit two or more events? Can those events be named differently than function name? I dunno. If not – where is the benefit?

It feels cleaner in flummox to just return value or promise instead of emitting them. But they have problems with invoking subsequent actions during the handle of original one (https://github.com/acdlite/flummox/issues/82). Maybe their approach is the straight source of the issue.

Anyway, those string constants (emitted event names) are very easy to put wrong and this will require debugging (cause no syntax error). That's a pain point.

Reflux made the same choice of explicit trigger but with Stores. And this caused a lot of problems. Code is more imperative, easy to forget this line and so on. Your implementation of setState is ok, why not apply the same reasoning to actions.

Once the action is invoked, the store itself is responsible for handling the request to your API. I think that this is beyond the scope of stores.

Here are my store handlers in flummox:

  loadManyCompleted(models) {
    this.setState({
      models: this.state.models.merge(models),
      loaded: true,
    });
  }

  loadManyFailed(response) {
    this.setState({
      loaded: true,
    })
  }

  loadOneCompleted(model) {
    this.setState({
      models: this.state.models.set(model.get("id"), model),
    });
  }

There is no request handling, just response handling. That looks ok, are you disagree?

And here are my actions. Very clean and readable:

  async loadMany() {
    let response = await Axios.get(`/api/robots/`);
    return Map([for (model of response.data) [model.id, Map(model)]]);
  }

  async loadOne(id) {
    let response = await Axios.get(`/api/robots/${id}`);
    return Map(response.data);
  }

  async remove(id) {
    let response = await Axios.delete(`/api/robotz/${id}`, {id});
    return id;
  }

[malte-wessel]

Yes you're right. The emitting part can cause confusion. I'm currently adding some warnings when actions emit events that are not registered as actions. But that shouldn't be the solution. I had another idea for solving this problem:

complete(data) {
  // Every action could be bound to an own object that provides 
  // a convenient `dispatch` method. Then you could write:
  this.dispatch(data);
}

Under the hood this would call this.emit('complete', data);. But one could argue that this is too much magic. I will think about it.

Thanks for your feedback!

[ivan-kleshnin]

I've updated my prev comment with a note about Reflux. I just think that Store and Action should have about the same depth of code layer in this stuff. Now stores use setState, but actions use naked emit. That's what I find not exactly right.

Magic is always about benefits and drawbacks. If it frees us from thinking and errors – that's good. React is a big magic blob on itself, but as it frees us from template mess we had before – we use it and we love it. If it's encapsulated and never fail – I'm after it.

[malte-wessel]

Yes, agree, the code is super clean. But as you pointed out in https://github.com/acdlite/flummox/issues/82 you cannot invoke subsequent actions. Besides that I think there are some problems with invoking actions from stores. This will break the unidirectional data flow, which makes it difficult to reason about changes in state:

Component -> Action -> Store -> Action -> Store -> Component

What do you mean by that?

Your implementation of setState is ok, why not apply the same reasoning to actions.?

[ivan-kleshnin]

What do you mean by that? (Your implementation of setState is ok, why not apply the same reasoning to actions)

Same as that (quoting myself):

I just think that Store and Action should have about the same depth of code layer in this stuff. Now stores use setState, but actions use naked emit. That's what I find not exactly right.

[ivan-kleshnin]

Besides that I think there are some problems with invoking actions from stores. This will break the unidirectional data flow, which makes it difficult to reason about changes in state.

Well I don't really think we have unidirectional data-flow in Flux. We have loop. Component > Action > Store > [dependent Stores] > Component. You may think of a state as a starting point but I believe it's an artefact of React implementation. My "proposal" will add subloop without Component in the chain, but doesn't change data-flow directions.

I may easily be wrong, but how to add alert on failed load without subsequent action? I surely can set loadFailed flag and visualize that in some Component but what if I want to add explicit Alert object in corresponding AlertStore? (alerts should be removable, maybe even sortable, e.g. be models as well). As I said in that thread, I don't want to make AlertStore dependent on every other stuff in the system.

But as you pointed out in acdlite/flummox#82 you cannot invoke subsequent actions.

Yeah, but they somehow messed it up. For example, in Reflux we can use them for sure.

[malte-wessel]

Ah, ok got it :) Yes, maybe the dispatch method for actions is the right way to go. I will check that!

[malte-wessel]

Yes, right, kind of a unidirectional loop :) With minimal-flux you could do that by having your alert store listening to any kind of fail action.

class Messages extends Actions {
  fetch() {
    Axios.get("/some/api").then(this.complete).catch(this.fail);
  }
  fail(err) {
    this.emit("fail", err);
  }
  // ...
}

class AlertsStore extends Store {
  constructor() {
    this.handleAction('messages.fail', this.addAlert);
  }
  // ...
}

That's why I think returning from actions is not the right way.

[ivan-kleshnin]

And that makes AlertStore dependent of every other alert emitters, as I mentioned...

class AlertsStore extends Store {
  constructor() {
    this.handleAction('users.loadManyFailed', this.add);
    this.handleAction('articles.loadManyFailed', this.add);
    this.handleAction('whatever.loadManyFailed', this.add);
    ...
  }
  // ...
}

Well, maybe the right way is just to accept it and move along. I'm not sure.

[ivan-kleshnin]

Well it turns out more interesting than it seems from the first sight.

We can approach this in two ways.

Interactive

AlertActions < UserStore [calls AlertActions.add()]
AlertActions < DocumentStore [calls AlertActions.add()]
AlertActions < SubscriptionStore [calls AlertActions.add()]

Reactive

UserActions < AlertStore [listens to UserActions.loadManyFailed]
DocumentActions < AlertStore [listens to DocumentActions.loadManyFailed]
SubscriptionActions < AlertStore [listens to SubscriptionActions.loadManyFailed]

Replacing with letters for brevity and removing "Store" & "Actions" suffixes that don't matter...:

Interactive

A < U
A < D
A < S 

• A can be library, U, S, D - can't
• Dependencies from A are scattered
• Changes in A may break U, D, C

Reactive

U < A
D < A
S < A

• U, D, S can be library, A - can't
• A module contains all it's dependencies
• Changes in U, D, C may break A

I bet there's more info I can't extract right now...

React on itself is not 100% reactive, but if we believe the more reactivity - the better, I think I should follow your advice and restructure my dependencies to be reactive.