Extend createAction API to include additional action properties

[sjsyrek]

I would like to combine the simplicity of createAction with the flexibility of createStandardAction by extending the API of the former to include additional action properties. For example, it is a common use case to include an error property in Flux standard actions, but the current implementation of createAction does not support this. In order to support additional properties beyond just one (or just error), we can make the third parameter to the returned function an object with any number of properties that can be spread over the action.

Here is an example of what this might look like:

createAction(types.WITH_PAYLOAD_META_ERROR, action => {
  return (id: number, token: string, error: boolean) => action(id, token, { error });
});

Initially discussed in #108.

[piotrwitek]

One note I have discovered when I was rewriting all the tests to dts-jest.
Please use overload solution, conditional types don't work with optional parameters, so if you have optional param mapping to an action prop, the resulting prop will be a non-nullable type (it's erasing undefined) and I don't like it as this is not retaining type-soundness this library is all about.

action is based on overloads which I refactored and type-soundness is looking good.
createAction is based on conditional and is losing undefined from optional params - we need to refactor it to overloads solution matching action function. Fortunately, a new test suite is much more strict and will catch these types of errors now.

[sjsyrek]

@piotrwitek OK cool. I also see that the overload solution is arguably more readable (readability is one reason I created a bunch of extra types for the conditional solution for createAction in the original PR).

One question: I've been looking at both action and createAction and considering your idea to make it possible to add any number of additional properties. This is still fine, but action at least is documented as a flux standard action factory and according to the FSA docs, such an action MAY have an error property (likewise payload and meta) and MUST NOT include any other properties. So I just wanted to clarify your vision and intentions before proceeding further.

I may be confused by the difference between creationAction and createStandardAction with regard to FSAs and the intended (and documented) purpose of this library's API.

Sneak preview:

export function action<T extends StringType, E>(
  type: T,
  payload: undefined,
  meta: undefined,
  error: E
): { type: T; error: E };

export function action<T extends StringType, P, E>( // this one may not work
  type: T,
  payload: P,
  meta: undefined,
  error: E
): { type: T; payload: P; error: E };

export function action<T extends StringType, M, E>( // this one may not work
  type: T,
  payload: undefined,
  meta: M,
  error: E
): { type: T; meta: M; error: E };

export function action<T extends StringType, P, M, E>(
  type: T,
  payload: P,
  meta: M,
  error: E
): { type: T; payload: P; meta: M; error: E };

Obviously, this only adds error and not a generic container type. I wonder why that would be reserved for error and any others, whereas support for both payload and meta are built in, as per the FSA spec?

Could you clarify with a little more detail what you're looking for? For example, require support for type, payload, and meta but make any additional properties variadic and spread them over whatever the resulting action object would be? So, the above functions would all have some other: {} field and then action and/or createAction would spread those (all typed any?) over the returned action object?

[piotrwitek]

Hey @sjsyrek, I have a feeling that we should go with the fourth param as error property which would be compliant with FSA standard, and I'll update the tutorial to state that it's FSA standard compatible.

The reason for that is that now we have createCustomAction to handle all the remaining use-cases, so most basic API should cover most common use-cases.

I'd agree to go with what you suggested in the sneak preview. 👍

[sjsyrek]

@piotrwitek I spent some time today working on this. Results so far: https://github.com/sjsyrek/typesafe-actions/tree/add-error-property-to-action

I want to make sure I'm conforming to the spirit of this project, however. And also to the best representation of the FSA spec. It's a little unclear what to do about the relationship between error and payload, for example. If error is true, then payload SHOULD be an Error object. Of course, trying to account for this in typesafe-actions will introduce additional complexity, so I left it out for now, which means the user will have to make sure they are following conventions. This also means it's possible to have an action with an error property but no payload.

Also, you might have a look at the tests. Everything passes, but that doesn't mean I wrote them correctly.

I'm struggling a bit with the overload implementation of createAction. Since the part that varies is a callback, tslint keeps yelling at me to unify the overloads into a union, which is fine, but it still doesn't work. Have you experimented with this at all? Or do you have some sense of the best way to do this?

I tried something like this, but it's not working:

export function createAction<
  T extends StringType,
  AC extends ActionCreator<T> = () => { type: T }
>(
  type: T,
  createHandler: (
    actionCallback: <P, M>(
      payload: P,
      meta: M
    ) => { type: T; payload: P; meta: M }
  ) => AC
): AC;

export function createAction<
  T extends StringType,
  AC extends ActionCreator<T> = () => { type: T }
>(
  type: T,
  createHandler: (
    actionCallback: <P>(
      payload: P,
    ) => { type: T; payload: P }
  ) => AC
): AC;

export function createAction<
  T extends StringType,
  AC extends ActionCreator<T> = () => { type: T }
>(
  type: T,
  createHandler: (
    actionCallback: <M>(
      meta: M,
    ) => { type: T; meta: M }
  ) => AC
): AC {
  // validation is already done in action function

  const actionCreator: AC =
    createHandler == null
      ? ((() => action(type)) as AC)
      : createHandler(action.bind(null, type) as Parameters<
          typeof createHandler
        >[0]);

  return Object.assign(actionCreator, {
    getType: () => type,
    // redux-actions compatibility
    toString: () => type,
  });
}

[piotrwitek]

@sjsyrek thanks for the update 👍 Don't worry about the overloads, for now. Let's finish the first part and we can take a look at overloads later. I'll probably have to take a look at it myself and see what the compiler is telling me.

Please run npm run test-types:update in your repo to generate spec snapshots, I'll be able to see resulting types in changelog

[sjsyrek]

@piotrwitek Shall we close this one?

[piotrwitek]

Only when #119 is merged and documentation is updated.

[piotrwitek]

@sjsyrek inspiration for API documentation update example: https://github.com/piotrwitek/typesafe-actions/releases/tag/v3.2.0

[sjsyrek]

@piotrwitek For createAction itself, I am still struggling with how to do the overloads correctly. The linter complains about them. Here's what I have so far:

import { StringType, ActionCreator } from './type-helpers';
import { action } from './action';

export function createAction<
  T extends StringType,
  AC extends ActionCreator<T> = () => { type: T }
>(type: T): AC;

export function createAction<
  T extends StringType,
  AC extends ActionCreator<T> = () => { type: T }
>(
  type: T,
  // tslint:disable-next-line: unified-signatures
  createHandler: (
    actionCallback: <P>(payload: P) => { type: T; payload: P }
  ) => AC
): AC;

export function createAction<
  T extends StringType,
  AC extends ActionCreator<T> = () => { type: T }
>(
  type: T,
  // tslint:disable-next-line: unified-signatures
  createHandler: (actionCallback: <M>(meta: M) => { type: T; meta: M }) => AC
): AC;

export function createAction<
  T extends StringType,
  AC extends ActionCreator<T> = () => { type: T }
>(
  type: T,
  // tslint:disable-next-line: unified-signatures
  createHandler: (actionCallback: <E>(error: E) => { type: T; meta: E }) => AC
): AC;

export function createAction<
  T extends StringType,
  AC extends ActionCreator<T> = () => { type: T }
>(
  type: T,
  // tslint:disable-next-line: unified-signatures
  createHandler: (
    actionCallback: <P, M>(
      payload: P,
      meta: M
    ) => { type: T; payload: P; meta: M }
  ) => AC
): AC;

export function createAction<
  T extends StringType,
  AC extends ActionCreator<T> = () => { type: T }
>(
  type: T,
  // tslint:disable-next-line: unified-signatures
  createHandler: (
    actionCallback: <P, E>(
      payload: P,
      error: E
    ) => { type: T; payload: P; error: E }
  ) => AC
): AC;

export function createAction<
  T extends StringType,
  AC extends ActionCreator<T> = () => { type: T }
>(
  type: T,
  // tslint:disable-next-line: unified-signatures
  createHandler: (
    actionCallback: <M, E>(type: T, meta: M) => { type: T; meta: M; error: E }
  ) => AC
): AC;

export function createAction<
  T extends StringType,
  AC extends ActionCreator<T> = () => { type: T }
>(
  type: T,
  // tslint:disable-next-line: unified-signatures
  createHandler: (
    actionCallback: <P, M, E>(
      type: T,
      payload: P,
      meta: M
    ) => { type: T; payload: P; meta: M; error: E }
  ) => AC
): AC;

/**
 * @description typesafe action-creator factory
 */
export function createAction<
  T extends StringType,
  AC extends ActionCreator<T> = () => { type: T }
>(
  type: T,
  createHandler?: (
    actionCallback: <P = undefined, M = undefined, E = undefined>(
      payload?: P,
      meta?: M,
      error?: E
    ) => any
  ) => AC
): AC {
  // validation is already done in action function

  const actionCreator: AC =
    createHandler == null
      ? ((() => action(type)) as AC)
      : createHandler(action.bind(null, type) as Parameters<
          typeof createHandler
        >[0]);

  return Object.assign(actionCreator, {
    getType: () => type,
    // redux-actions compatibility
    toString: () => type,
  });
}

I haven't updated the tests yet, however, and two of them aren't passing.

[sjsyrek]

Also noticed this in the redux-actions repo:

If the payload is an instance of an Error object, redux-actions will automatically set action.error to true.

Is this something you think worth emulating?

[sjsyrek]

@piotrwitek I have some time in the next week or so to work on this if you have comments!

[piotrwitek]

@sjsyrek thanks for the heads-up, let me try to look at it tomorrow

[sjsyrek]

@piotrwitek ping :-)

[piotrwitek]

Sorry I was preparing for my vacations, I'll pick it up in 2,5 weeks when I'm back

[sjsyrek]

@piotrwitek Any update?

[piotrwitek]

Hey @sjsyrek, thanks for the reminder, I'll try to pick this up this week, I have a lot on my plate now 💻

[sjsyrek]

@piotrwitek No worries, me too. I just don't want to forget about this project. I like the library and want to help improve it if I can be useful.

[piotrwitek]

I'm blocking this issue for now as I'm working on new createAction API so this API might be deprecated. I'll get back to it when v5 design is complete.

[piotrwitek]

Closing because it is superseded by #207