Implement ActionCreatorFactory and ReducerFactory

[LouizFC]

The concept is very simple, making everything typesafe is when the headache comes.

I have no experience testing types, but I wrote some code to test the types:

const factory = createActionCreatorFactory({
  a: (type) =>
    createActionCreator(type, (resolve) => (index: number) =>
      resolve({ index })
    ),
  b: (type) =>
    createActionCreator(type, (resolve) => (name: string) => resolve({ name })),
});

const result = factory({ a: "A", b: "B" } as const);

const actionA = result.a(2); // produces { type: "A", payload: { index: number } } type
const actionAFail = result.a(); // should fail

const actionB = result.b("Bob"); // produces { type: "B", payload: { name: string } } type
const actionFail = result.b(); // should fail

const actionC = result.c(); // should fail

I will write the unit tests when I get some free time, for the moment I am opening this PR to show the concept that I was talking about at #106.

[the-dr-lazy]

There are some (potentially) boilerplate points that can be reduced. Please look at following example:

const pauci = resolve => () => resolve()

const createFetchActionCreators = createActionCreatorsFactory({
  next: pauci,
  cancel: pauci,
  error: resolve => (error: Error) => resolve(error),
  complete: resolve => (items: any[]) => resolve(items),
})

const fetch = createFetchActionCreators(<const>{
  next: '[Fetch] next',
  cancel: '[Fetch] cancel',
  error: '[Fetch] error',
  complete: '[Fetch] complete',
})

In the above example, the createActionCreatorsFactory accepts a map of executors which create payload and meta of an action by the given resolve callback (similar to executor parameter in createActionCreator). Also, there is a common executor if returning action of the action creator is type only (doesn't contain payload and/or meta) which can be done by resolve => () => resolve() and I named it pauci in the example.

[LouizFC]

Very interesting. I will try to work on it

[LouizFC]

I have modified the function to behave like you mentioned, but I am having trouble testing the types.

I found a testing tool called tsd, the output is not as elegant as dts-jest but at least I can test if the types behave like I want.

[the-dr-lazy]

What is the trouble? can you commit type tests?

[LouizFC]

I am sorry, it's just that I am not familiar of how dts-jest tests work.

How can I test types? for example, in tsd, I can do:

const factory = createActionCreatorFactory({
  a: (resolve) => (index: number) => resolve({ index })
});

const actionCreators = factory({ a: "A" } as const);

expectError(factory({ a: "A", b: "B", d: "D", e: "E" } as const));
expectError(factory({ a: "A", b: "B" } as const));

expectType<{ type: "A"; payload: { index: number } }>(actionCreators.a(1));
expectType<"A">(actionCreators.a.type); // Here it is testing the type, not the return value
expectType<"A">(actionCreators.a(1).type);
expectType<number>(actionCreators.a(1).payload.index);

expectError(actionCreators.a(1).fail);
expectError(actionCreators.a());
expectError(actionCreators.a("Test"));

With dts-jest I need to generate snapshots. The problem is that my implementation is not yet completed, so I really don't know how I can take a snapshot of something that don't exist yet.

[LouizFC]

Also, what you think about the following syntax?

const createFetchActionCreators = createActionCreatorsFactory(
  (
    resolve /** maybe an 'default' function here? in this case, ()=>resolve()*/
  ) => ({
    next: () => resolve(),
    cancel: () => resolve(),
    error: (error: Error) => resolve(error),
    complete: (items: any[]) => resolve(items),
  }) /** and maybe allow the user to customize the default function here? */
);

const fetch = createFetchActionCreators(<const>{
  next: "[Fetch] next",
  cancel: "[Fetch] cancel",
  error: "[Fetch] error",
  complete: "[Fetch] complete",
});

[the-dr-lazy]

The problem is that my implementation is not yet completed, so I really don't know how I can take a snapshot of something that don't exist yet.

Unfortunately, snapshot testing is not compatible with TDD. So write the snapshot tests after you are sure about API and then snapshots can play their role as guards for potential future unintentional changes in types. BTW, you can use other testing tools (e.g. tsd) which you are comfortable with it in your development environment.

const createFetchActionCreators = createActionCreatorsFactory(
 (
   resolve /** maybe an 'default' function here? in this case, ()=>resolve()*/
 ) => ({
   next: () => resolve(),
   cancel: () => resolve(),
   error: (error: Error) => resolve(error),
   complete: (items: any[]) => resolve(items),
 }) /** and maybe allow the user to customize the default function here? */
);

const fetch = createFetchActionCreators(<const>{
 next: "[Fetch] next",
 cancel: "[Fetch] cancel",
 error: "[Fetch] error",
 complete: "[Fetch] complete",
});

This is not possible because resolve actually is an action type bound version of createAction. In other words resolve for every key of executor map will bind to a different action type:

// resolve for next
function resolve(payload, meta) {
  return createAction('[Fetch] next', payload, meta)
}

// resolve for cancel
function resolve(payload, meta) {
  return createAction('[Fetch] cancel', payload, meta)
}

// resolve for error
function resolve(payload, meta) {
  return createAction('[Fetch] error', payload, meta)
}

// resolve for complete
function resolve(payload, meta) {
  return createAction('[Fetch] complete', payload, meta)
}

So with a single resolve, we cannot create different action with different action type. BTW, the executor function should be familiar for Deox users because it is the second argument of createActionCreator. So I think in this way, the API looks like more convenient and familiar.

[LouizFC]

Understood.

But a problem arises in the following cenario:

const createFetchActionCreators = createActionCreatorsFactory({
  next: resolve => () => resolve(),
  cancel: resolve => () => ({
    type: "Cancel", // the type property was given by the user
    payload: { status: "cancelled" },
    someOtherNonStandardProperty: true 
  }),
  error: resolve => (error: Error) => resolve(error),
  complete: resolve => (items: any[]) => resolve(items),
})

(lets call (resolve)=>()=>resolver() executor)

This is a non issue with createActionCreator because we know the type before creating the action inside the executor, so the compiler can enforce it.

We don't know the ActionType that cancel will have, so we cannot enforce it at compile time even if we want.

So, when passing the executor that we created at factory to the createActionCreator, we will need to overwrite the type property inside it before letting createActionCreator use it.

Something like this:

createActionCreator(types[key], (resolve) => (...args) => ({
    ...mapper[key](resolve)(...args), type: types[key]
}))

If we don't do that, we will produce an action with the Cancel type instead of the type given by the user.

Because we will overwrite the type property everytime, I think just one resolver should be enough

[the-dr-lazy]

This looks like a bad practice to me. As an example, you can run a promise in a reducer in Redux but that's not Redux's fault, that's your fault. So it's bad to explicitly return an object in an executor without calling resolve and we should discourage it. For preventing the user to return explicit objects without calling resolve maybe we can check that the resolve has been called or not then we can print a warning to console to note potentially strange behavior.

[LouizFC]

While I understand that this is a bad pratice, I think that giving a wrong type to the user is a even "badder" pratice.

I will rethink the whole approach to this problem in conjuction with the createReducerFactory that I talked about at #106

Edit: also, if you see the current implementation, there is a lot of type hacking, which by itself is very bad

[the-dr-lazy]

While I understand that this is a bad pratice, I think that giving a wrong type to the user is a even "badder" pratice.

Totally agree. I think the ideal implementation with executors should be as follow (please let me know your opinion):

const createTestActionCreators = createActionCreatorsFactory({
  test: resolve => () => ({ type: 'TEST' })
})

const { test } = createTestActionCreators({ test: 'NOT_TEST' })
test // () => { type: 'TEST' }

I'm not sure but I think there is a limitation about the inference of type property in the executor. I'll try to find a way to make the above example possible.

[LouizFC]

I really enjoy how clean the API looks with executors, but the only problem with executors is that the action type property that the resolve produces is unknown, so it gets widened to string. I will explore some ideas when I get some time

[LouizFC]

I think I finally got it right, the API is not so beautiful but I will try to simplify it as much as possible from the approach that I just tried.

The "ugliest" thing about the new API is that you need to declare your generics explicitly, like so:

const factory = createActionCreatorFactory((handle) => ({
  next: handle(<T extends string>(resolve: Resolver<T>) => (id: string) =>
    resolve({ id })
  ),
  cancel: handle(<T extends string>(resolve: Resolver<T>) => (value: boolean) =>
    resolve(undefined, value)
  ),
  error: handle(<T extends string>(resolve: Resolver<T>) => (error: Error) =>
    resolve(error)
  ),
  complete: handle(<T extends string>(resolve: Resolver<T>) => () => resolve()),
}));

const actionCreators = factory({
  next: (creator) => creator("A"),
  cancel: (creator) => creator("B"),
  error: (creator) => creator("C"),
  complete: (creator) => creator("D"),
});

I will try to simplify things now that I got it to work. I think I can probably get rid of handle

Edit: I forgot to mention: this new API solves the problem with the type property, if you try to declare the type property by yourself the compiler will scream something like "TEST" does not satisfy type constrait of T

[LouizFC]

@thebrodmann I tried some other approaches but I think this is how far we can go with typescript without relying on the previous "type hacking", the current approach produces the correct types, I just need to create a proper type for CreatorMap.

I will try to simplify the current implementation, but I want to know your opinion on the proposed API

[the-dr-lazy]

Ah, sorry for late reply @LouizFC. :(

I think the API you have suggested above looks too complicated to me even if we get rid of handle! But as I said I thought about type property issue.

So, when passing the executor that we created at factory to the createActionCreator, we will need to overwrite the type property inside it before letting createActionCreator use it.

From your above sentence, I come up with a solution. In #122 I propose a new API for createActionCreator function in which the resolve callback will be removed but, the related portion of that proposal to this PR is that the type property in return type of callable (a function which will create payload and meta of an action) will become obligated to be undefined and if user set it to something other than undefined TypeScript will throw an error for mismatched type.

The API will look like as follows:

const factory = createActionCreatorFactory({
  good: (payload: number) => ({ payload }),
  bad: (payload:number) => ({ payload, type: 'MISTAKE' }) // type mismatch error
})

const actionCreators = factory(<const>{
  good: 'GOOD',
  bad: 'BAD'
})

[LouizFC]

@thebrodmann As I commented on #122 , I am not really agains't it, but I think createActionCreator is fine the way it is.

In the other hand, what you proposed looks very similar to the idea I had here: https://github.com/thebrodmann/deox/pull/108#issuecomment-524030139 so I liked it very much (on the context of createActionCreatorFactory)

I will reiterate the problem again this weekend and see if I can simplify things up

[LouizFC]

@thebrodmann I made a quick draft here, please take a look

[the-dr-lazy]

There are two points about the above playground:

1. The return type of action creators are any

actionCreators.next // (id: string) => any

actionCreators.cancel // (value: boolean) => any

actionCreators.error // (error: Error) => any

actionCreators.complete // () => any

2. What resolve callback do? I think it should be something like (payload, meta) => ({ paylaod, meta }). Am I right?

[LouizFC]

@thebrodmann Can you please give more details? Previously MultiExecutor was named Resolver, so naming got a bit weird, I am sorry

The return type of action creators are any

I have put some "dummy" code on the playground to take a look at the types, you can see it here.

To me the types are correct:

What resolve callback do? I think it should be something like (payload, meta) => ({ paylaod, meta }). Am I right?

Yes. resolve works just like in createActionCreator, the only difference being that it creates typeless actions.

[the-dr-lazy]

Yes. resolve works just like in createActionCreator, the only difference being that it creates typeless actions.

So it doesn't carry any data or type and there is no need to pass it as a callback. We can have it as a separate function.

declare function createTypelessAction<
  TPayload,
  TMeta
>(payload?: TPayload, meta: TMeta): { payload: TPayload, meta: TMeta }

But there comes another question. Why someone should prefer the createTypelessAction function over simple object declaration syntax?

// [1]
declare function createActionCreatorFactory({
  next: (id: string) => createTypelessAction(id)
})

// [2]
declare function createActionCreatorFactory({
  next: (id: string) => ({ payload: id })
})

[LouizFC]

Sorry for the delayed response.

So it doesn't carry any data or type and there is no need to pass it as a callback. We can have it as a separate function.

You are right, maybe it is better to use it as a separate function, but it introduces some boilerplate as you mentioned.

But there comes another question. Why someone should prefer the createTypelessAction function over simple object declaration syntax?

Because it takes care of the error: true boilerplate, but I got your point, the user will not feel encouraged to use the function.

Let me ask you something: should we maintain extra properties (properties that are not error, payload or meta) or we should forbid it?

In my opinion these should be kept. Most of the projects I worked on people never use meta outside "error actions", most of them declares extra properties on the action instead, because most of times people avoid nested objects, mostly because it introduce boilerplate (null check, complex cloning, etc).

As a counter argument to myself, libraries should encourage good pratices.

[the-dr-lazy]

should we maintain extra properties (properties that are not error, payload or meta) or we should forbid it?

We forbid it, the only properties that can be returned are payload and meta then when an action creator has been called we create payload and meta by the provided function and pass them to createAction. As you can imagine the error property in this way will handle by createAction.

Most of the projects I worked on people never use meta outside "error actions", most of them declares extra properties on the action instead.

One of the Deox promises is FSA compliant. In fact, one of the conventions makes Deox possible is FSA. I saw some developers don't use payload or meta properties and send the payload with custom properties (e.g. name) in actions. How we can determine which properties are payload and which ones are meta? So we are sticking to the best practices by being FSA compliant.

[LouizFC]

@thebrodmann Understood. Thank you very much for your feedback. I will work on the next iteration with that in mind

[the-dr-lazy]

I'm going to close this PR due to inactivity. Feel free to open it if it's needed.