underdash: Actions

[taion]

ref #337

From #337 (comment):

---

Actions/constants in Alt generally look very nice to work with due to the reduced boilerplate, but my main concern is that Actions classes implement idiosyncratic behavior to accomplish this that makes them not resemble normal ES6 classes, and behave in ways that may be surprising or unclear to users who aren't familiar with Alt patterns.

I'd like for actions to both offer a terse, low-boilerplate syntax, while still following the lines of more standard ES6 objects, with more intuitive behavior for this.

I propose to have 3 categories of properties on an Actions object;

1. Standard actions that just forward their payload
2. Actions that transform and forward their payload
3. Normal methods that aren't actions at all (but may serve as action creators that dispatch other actions)

I think the code should look like this:

class FooActions extends Actions {
  static actions = ['simpleAction', 'getFooStarting', 'getFooDone', 'getFooFailed'];
  // Or maybe this.generateActions(...) in constructor.

  @action
  actionWithTransform(value) {
    return this.getTransformed(value);
  }

  getTransformed(value) {
    return 2 * value;
  }

  getFoo(id) {
    this.getFooStarting({id});
    FooWebApiUtils.getFoo(id).then(
      result => this.getFooDone({id, result}),
      error => this.getFooFailed({id, error})
    );
  }
}

What you end up with is that FooActions.simpleAction is just a simple action that will dispatch its payload, and that FooActions.actionWithTransform is an action that will apply a simple transform to its payload before dispatching it.

However, FooActions.getTransformed isn't an action at all, and will just return the transformed value. Likewise, FooActions.getFoo is not an action - it's a standard Flux-y action creator that dispatches other actions (by calling their methods).

My concerns with this proposal are:

1. Is it sufficiently obvious that e.g. above, getFooDone is an action but getFoo is an action creator? Per my earlier proposal, maybe use CONSTANT_CASE for actions and camelCase for regular methods?
2. This is proposal is incompatible with some Alt patterns - if you have an action that looks like e.g. foo(value) { fooFuture().then(() => this.dispatch(value)); }, you would need to create a new e.g. fooDone action and dispatch that (via this.fooDone(value)), as fooDone, rather than as foo as it would be right now. This is the flip side of having a firm separation between actions and action creator or helper methods.

[taion]

Revisiting the above example, because I didn't totally like the API around the async example:

ES7:

@alt.register
class FooActions extends Actions {
  constructor() {
    super();

    this.generateActions('simpleAction');
    this.generateAsyncActions('updateFoo');
  }

  @action
  actionWithTransform(value) {
    return this.getTransformed(value);
  }

  getTransformed(value) {
    return 2 * value;
  }

  updateFoo(id) {
    this.updateFoo.starting({id});
    return FooWebApiUtils.updateFoo(id).then(
      result => this.updateFoo.done({id, result}),
      error => this.updateFoo.error({id, error})
    );
  }
}

The above desugars into:

ES6:

class FooActions extends Actions {
  constructor() {
    super();

    this.generateActions('simpleAction', 'actionWithTransform');
    this.generateAsyncActions('updateFoo');
  }

  actionWithTransform(value) {
    return this.getTransformed(value);
  }

  getTransformed(value) {
    return 2 * value;
  }

  updateFoo(id) {
    this.updateFoo.starting({id});
    return FooWebApiUtils.updateFoo(id).then(
      result => this.updateFoo.done({id, result}),
      error => this.updateFoo.error({id, error})
    );
  }
}

alt.register(FooActions);

With ES5:

alt.registerActions({
  actions: ['simpleAction', 'actionWithTransform'],
  asyncActions: ['updateFoo'],

  actionWithTransform: function (value) {
    return this.getTransformed(value);
  }

  getTransformed: function (value) {
    return 2 * value;
  }

  updateFoo: function (id) {
    // Use a Promise polyfill or something, I don't know...
    this.updateFoo.starting({id});
    return FooWebApiUtils.updateFoo(id).then(
      result => this.updateFoo.done({id, result}),
      error => this.updateFoo.error({id, error})
    );
  }
});

For the async piece, the idea is that we end up with updateFoo.starting, updateFoo.done, and updateFoo.error set up as actions (probably mapped to updateFooStarting, updateFooDone, and updateFooError).

Compared to Alt v0.17:

1. Actions that dispatch synchronously must use the return-to-dispatch pattern, for which there's a little more boilerplate (either decorate with @action or register as action ahead of time)
2. A little more boilerplate for async actions using the provided pattern, plus a little more opinionated in always generating loading/done/error actions
   • This could integrate nicely with underdash: Stores/Data Fetching #361, though, because it would allow doing e.g. asyncAction: FooActions.getFoo to wire up all 3 callbacks, e.g. you just generate the async actions, but you don't generate an action creator
3. Actions classes are plain old classes (or equivalent when not using ES6) - no hijacking of this, so things are more predictable for developers

Does this make sense to do? I'm not even totally sure at this point. I really like the idea of having (3) above as we move forward with this, especially if we're changing the API anyway.

I think this is easier to explain because we won't have edge cases like not auto-dispatching for actions that return promises, and we won't have to explain that this in the context of an action does not behave as would normally be expected in the context of a class.

[taion]

ref #383 which made me realize my async example in the OP was annoying and sucky.

[taion]

Thinking about this a little more, it's probably best not to change the syntax for POJSOs at all - it's only in the case of using ES6 classes that there would be any expectation of class-type behavior. Only exception here might be the sugar for async actions.

[taion]

Summarizing my thoughts from a Gitter discussion - basically, what's the point of any of this, rather than having actions be simple dispatchables?

Consider these examples:

• https://github.com/airbnb/airpal/blob/3786f94174d0d2683abd961a000486a2f632e8cd/src/main/resources/assets/javascripts/actions/QueryActions.js
• https://github.com/goatslacker/alt/blame/8c09de9a64cb46a95587c59357e3ef0550967e36/guides/getting-started/async.md#L60-L72

Compare:

• https://github.com/goatslacker/alt-search-docs/blob/853e82e367e6e72c31f454e80a3a767aafcc27fe/src/actions/SearchActions.js

The first and second examples look a lot like action creators from vanilla Flux, while the last example basically consists of simple dispatchable constants. The question here is whether we need to have actions of the former type, or whether we only need actions of the latter type.

One answer is that, for many use cases, you don't:

• For async data fetching, the data source pattern is generally better - it's cleaner, more declarative, and works really nicely with server-side rendering
• For many other use cases, you can create a separate utility/action creator function anyway

However, I think there are the following considerations that make it worthwhile to have "action creator" methods on actions objects that aren't just simple dispatchables:

• In vanilla Flux, usually you dispatch all actions via action creators, which insulates the caller of the action creators from implementation details. In a scheme with simple dispatchable actions and utilities, the user has two unattractive options:
  • Use utility methods for async operations but invoke actions directly for sync operations - this is unattractive because now I've leaked implementation details to the user of the actions, rather than just present action creators that can be used for whatever
  • Define utility methods for all actions - this is unattractive because it's just unnecessary boilerplate for actions that are synchronous and don't do anything fancy
• I'm not aware of any patterns like the data source/fetch pattern for dealing with async writes, so the only thing you have are something like action creators
• The return-for-dispatch-value pattern means that actions can apply arbitrary synchronous transforms, so being able to do other complex things seems like a natural extension functionality-wise

And I think that to the extent you have things like action creator methods on your actions that actually do something non-trivial, ES6 classes offer a nice way to extend/subclass behaviors, so it'd be nice to support them.

[jareware]

Thanks for a lot of thought-provoking discussion on this!

I think one of the guiding lights here should be minimal base API. We have methods for composing those into more convenient API's (decorators with ES7, extension with ES6, composition with ES5) as needed, and can continue the lovely trend in alt of keeping the more opinionated pieces in modules that play very nicely together with core, but are opt-in externals.

In that vein, I would propose the base API to only contain a single type of methods/actions, e.g.

class FooActions extends Actions {
  getFoo(newId) {
    asyncOp().then(this.receiveFoo); // no magic properties, just traditional this-access
    return newId; // this dispatches the action
  }
  receiveFoo(foo) {
    return foo;
  }
}

// or

var acts = alt.createActions({
  getFoo(newId) {
    asyncOp().then(acts.receiveFoo); // no magic this (also meaning no .bind() necessary!)
    return newId; // this dispatches the action
  }
});

On top of this, you can then layer the convenience as much as you want. Say, we offer the auto-generation of pass-through actions:

class FooActions extends Actions {
  constructor() {
    this.createSimpleActions('receiveFoo', 'failFoo');
  }
  // getFoo() as before
}

// or

alt.createActions( // multiple arguments are merged together as in _.extend()
  alt.createSimpleActions('receiveFoo', 'failFoo'),
  {
    getFoo(newId) {
      // as before
    }
  }
);

// or

alt.createActions( // multiple arguments allowed
  [ 'receiveFoo', 'failFoo' ], // array arguments mean "simple actions"
  {
    getFoo(newId) {
      // as before
    }
  }
);

What problems do you see with this? The idea would be trading a tiny bit of convenience in for a lot of reduced magic (which is frankly the thing I trip most with when working with alt).

[taion]

@jareware

I think that's pretty close to what I have. The main difference is that, by default, I want to make it be even less magical in that methods on ES6 classes should not all implicitly be actions, and that users should have to either decorate with @action or do something else to mark them as such.

This does add some boilerplate for things other than simple actions (though it's much less bad with decorators), but it reduces the magic by quite a lot. In your examples, to me it's quite magical and surprising that calling an innocent-looking function like receiveFoo will dispatch an action rather than just be a no-op.

Also, I think the case of async actions with start/success/error triplets are common enough that they should just be supported, though perhaps in utils rather than in core.

[goatslacker]

How I've been writing actions lately:

export default alt.createActions({
  displayName: 'FuckActions',  
  fucksGiven: x => x
});

import FuckActions from 'somewhere';
FuckActions.fucksGiven(2); // dispatches 2 with an action id of FuckActions.FUCKS_GIVEN

These are functions so you can prepare your payload if you need to in any way (action creator) but it also does the dispatching when something is returned. I don't think we need classes in actions muddying up the implementation.

[taion]

I'm mostly on board with that, but what about async write operations?

For a sync write operation, I can call FooActions.createFoo(foo), but if I want wiring around starting/success/error, I need something extra... either FooUtils.createFoo(foo) (which is annoyingly different), or I need to be able to do more than just attach a synchronous transform to the payload for FooActions.createFoo.

[taion]

Maybe:

function patchFoo({id, data, options}) {
  fetch( /* ... */ ).then(
    result => {id, data, options, result},
    error => throw {id, data, options, error}
  );
}

alt.createActions({
  @asyncAction
  patchFoo: (id, data, options) => patchFoo({id, data, options})
});

But I have no idea what the payload would be for the "starting" action.

[jareware]

@goatslacker hmm, that format looks nice. Two issues that come to mind:

1. still requires a magical this (e.g. for this.dispatch)
2. action methods and definition properties (e.g. displayName) are mixed together, but I suppose you can kinda tell which are which after working with alt a bit

@taion are you sure there actually needs to exist dedicated syntax for those triplets? I mean, this isn't too much typing and works quite nicely:

alt.createActions({
  fetchFoo(id) {
    this.dispatch(id);
    return axios.get("/")
      .then(this.actions.fetchFooSuccess)
      .catch(this.actions.fetchFooFailure)
  },
  fetchFooSuccess: x => x,
  fetchFooFailure: x => x
});

So especially if we'd agree that we don't really need/want to support Actions classes (good riddance!), it would be super simple to add whatever kind of utilities on top of that, to support e.g. these fetch* triplets.

[taion]

@jareware

It's a little magical, right? There's the this.dispatch, and there's how some actions automatically dispatch their return result, while other actions don't.

That's why I think there might be value in having explicit separation between "actions" and "action creators". I want to ship them all together to present a unified API to components that might dispatch actions, but I feel like it would be best if there were a clear separation between:

• "Actions" that just dispatch their return value, and
• "Action creators" that can dispatch actions, potentially asynchronously, and run other JS - i.e. just plain JS methods

And the this.dispatch, this.actions stuff is even more confusing if you have class FooActions extends Actions, because ES6 classes don't work that way normally.

[taion]

Here's a more structured pattern for async actions w/the full state triplet I just thought of:

@asyncAction({starting: (id, data) => {id, data}})
updateFoo(id, data) {
  return FooApiUtils.updateFoo(id, data).then(
    result => {id, data, result},
    error => throw {id, data, error}
  );
}

I think you still might want "Actions classes" with generic helper methods that can be called without dispatching actions, but this pattern smells like progress.

[taion]

Fleshed-out example with 3 actions of different types:

• simpleAction - just dispatches the payload
• complexAction - envelopes and dispatches the payload
• updateFoo - full async example, dispatches updateFoo.starting, updateFoo.success, updateFoo.error

1. ES6 classes with full-fledged Babel stage 0

@alt.register
class FooActions extends Actions {
  static actions = ['simpleAction'];

  @action
  complexAction(value) {
    return {value};
  }

  @asyncAction({starting: (id, data) => {id, data}})
  updateFoo(id, data) {
    return FooApiUtils.updateFoo(id, data).then(
      result => {id, data, result},
      error => throw {id, data, error}
    );
  }
}

2. ES6 classes with just ES6 features

@alt.register
class FooActions extends Actions {
  complexAction(value) {
    return {value};
  }

  updateFoo(id, data) {
    return FooApiUtils.updateFoo(id, data).then(
      result => {id, data, result},
      error => throw {id, data, error}
    );
  }
}

FooActions.actions = ['simpleAction', 'complexAction'];
FooActions.asyncActions = {
  updateFoo: {starting: (id, data) => {id, data}}
};

3. POJSOs with decorators

alt.createActions({
  actions: ['simpleAction'],

  complexAction: value => {value},

  @asyncAction({starting: (id, data) => {id, data}})
  updateFoo: (id, data) => FooApiUtils.updateFoo(id, data).then(
    result => {id, data, result},
    error => throw {id, data, error}
  )
});

4. ES5 with polyfilled promises

alt.createActions({
  actions: ['simpleAction'],
  asyncActions: {
    updateFoo: {
      starting: function (id, data) { return {id: id, data: data}; }
    }
  };

  complexAction: function (value) {
    return {value: value};
  }

  updateFoo: function (id, data) {
    return FooApiUtils.updateFoo(id, data).then(
      function (result) { return {id: id, data: data, result: result}; },
      function (error) { throw {id: id, data: data, error: error}; }
    );
  }
});

In almost all cases, (3) would be the preferred form - but (1) and (2) are available just in case people want to be able to extend and compose actions classes, and (4) would be available for luddites.

[goatslacker]

cc @lelandrichardson

[taion]

I think this is irrelevant now.