the-dr-lazy / deox

Functional Type-safe Flux Standard Utilities
https://deox.js.org
MIT License
206 stars 12 forks source link

Why "createReducer" return type isn't a deox/dist/types/Reducer? #64

Closed LouizFC closed 5 years ago

LouizFC commented 5 years ago

For those who received a incomplete issue in email, I am sorry, I have hit enter accidentally before finishing.

Now, to the issue:

I am trying to pass a Reducer as a parameter to a method, but I get a compile error about the types.

Please let me know if I am using the library wrong:

Error:(61, 27) TS2345: Argument of type '(state: DeepImmutableArray | undefined, action: { type: string; payload: Param; } | { type: string; payload: { index: number; param: Param; }; } | { type: string; payload: { fromIndex: number; toIndex: number; }; }) => Param[] | DeepImmutableArray<...>' is not assignable to parameter of type 'Reducer<DeepImmutableArray, { type: string; payload: Param; } | { type: string; payload: { index: number; param: Param; }; } | { type: string; payload: { fromIndex: number; toIndex: number; }; }>'. Types of parameters 'state' and 'prevState' are incompatible. Property '[Symbol.iterator]' is missing in type 'DeepImmutableObject<DeepImmutableArray>' but required in type 'DeepImmutableArray'.

export function createParamTableReducer(
  actions: ParamTableActionCreators
): Reducer<Param[]> {
  const deoxReducer = createReducer([] as Param[], (handle) => [
    handle(actions.addParam, (state, { payload }) =>
      produce(state, (draft: Draft<Param[]>) => {
        draft.push(payload);
      })
    ),
    handle(actions.removeParam, (state, { payload }) =>
      produce(state, (draft: Draft<Param[]>) => {
        draft.splice(payload.index, 1);
      })
    ),
    handle(actions.updateParam, (state, { payload }) =>
      produce(state, (draft: Draft<Param[]>) => {
        draft[payload.index] = payload.param;
      })
    ),
    handle(actions.reorderParams, (state, { payload }) =>
      produce(state, (draft: Draft<Param[]>) => {
        draft.splice(payload.fromIndex, 1);
        draft.splice(payload.toIndex, 0, state[payload.fromIndex]);
      })
    ),
  ]);

  return normalizeReducer(deoxReducer);
}
import { Reducer } from "deox/dist/types";
import { Action, Reducer as ReduxReducer } from "redux";

export function normalizeReducer<T, A extends Action>(
  deoxReducer: Reducer<T, A>
): ReduxReducer<T, A>

Why doesn't "createReducer" uses the existing Reducer type as a returnType?

the-dr-lazy commented 5 years ago

Sorry, I don't understand the topic. Can you explain more? or maybe reproduce it with codesandbox or whatever.

UPDATE: Whooops, the main comment was not loaded for me at that moment :|

LouizFC commented 5 years ago

@thebrodmann Sorry, I have pressed Enter by accident when typing the title

LouizFC commented 5 years ago

For completeness, here is the ParamTableActionCreators

export function createParamActionCreators(paramTableName: string) {
  return {
    addParam: createActionCreator(
      `[${paramTableName}] ADD_PARAM`,
      (resolve) => (param: Param) => resolve(param)
    ),
    removeParam: createActionCreator(
      `[${paramTableName}] REMOVE_PARAM`,
      (resolve) => (index: number, param: Param) => resolve({ index, param })
    ),
    updateParam: createActionCreator(
      `[${paramTableName}] UPDATE_PARAM`,
      (resolve) => (index: number, param: Param) => resolve({ index, param })
    ),
    reorderParams: createActionCreator(
      `[${paramTableName}] REORDER_PARAMS`,
      (resolve) => (fromIndex: number, toIndex: number) =>
        resolve({ fromIndex, toIndex })
    ),
  };
}

type ParamTableActionCreators = ReturnType<typeof createParamActionCreators>;
the-dr-lazy commented 5 years ago

Reducer<DeepImmutableArray | undefined, ...> is not assignable to Reducer<DeepImmutableArray, ...>

I think the problem is the difference in types between acceptable reducer of the target method and which you are giving to it. I don't think it is because of not using type helper in createReducer return type.

LouizFC commented 5 years ago

I understand. But what I mean is: The type given by deox/dist/types/Reducer is not compatible with createReducer return type.

I have made no further transformations as you can see, yet I am having a compile time error at return normalizeReducer(deoxReducer);.

I will make some tests to try to prove my argument, maybe I am overthinking something.

LouizFC commented 5 years ago

I think I found the cuprit:

// createReducer
export declare function createReducer<State, HM extends HandlerMap<State, any>>(
  defaultState: State,
  handlerMapsCreator: (handle: CreateHandlerMap<State>) => HM[]
): (
  state: DeepImmutable<State> | undefined,
  action: HM extends HandlerMap<State, infer T> ? T : never
) => State | DeepImmutable<State>;

//redux
export type Reducer<S = any, A extends Action = AnyAction> = (
  state: S | undefined,
  action: A
) => S
// deox/dist/types/Reducer
export declare type Reducer<State, Actions> = (
  prevState: DeepImmutable<State>, // < needs to be DeepImmutable<State> | undefined
 action: Actions
) => DeepImmutable<State> | State;
the-dr-lazy commented 5 years ago

You're right. Actually, Deox's Reducer type was used in handleAction which never call with an undefined state.

the-dr-lazy commented 5 years ago

Have you any suggestion to handle this situation?

LouizFC commented 5 years ago

On the top of my mind, you could do this:

export declare type CreateHandlerMap<State> = <
  AC extends ActionCreator<string>,
  Actions extends AnyAction = AC extends (...args: any[]) => infer T ? T : never
>(
  actionCreators: AC | AC[],
  handler: HandlerReducer<State, Actions> // < change here
) => HandlerMap<State, Actions>;

And on deox/types:

export declare type HandlerReducer<State, Actions> = (
  prevState: DeepImmutable<State>,
 action: Actions
) => DeepImmutable<State> | State;
/* maintain the old Reducer export for backward compatibility maybe? 
 * if that is not a concern, DeoxReducer could be renamed to just Reducer */
export declare type Reducer<State, Actions> = HandlerReducer<State, Actions> 

export declare type DeoxReducer<State, Actions> = (
  prevState: DeepImmutable<State> | undefined,
 action: Actions
) => DeepImmutable<State> | State;
the-dr-lazy commented 5 years ago

I agree. Handler and Reducer types should be separate as Handler always gives non- undefined state but Reducer can give undefined state.

I don't think there is a need for backward compatibility, because Reducer type does not present in public API.

LouizFC commented 5 years ago

I have a question that is kinda offtopic but related to this.

If we were to make the return type of createReducer explicit using the DeoxReducer above, how would be it declared?

In DeoxReducer<State, Action> The State type is obvious, but I am having some trouble wrapping my head around the Action type, because it extracts the action of each of the HandlerMaps

How can I express this type in the return type?

the-dr-lazy commented 5 years ago

If we were to make the return type of createReducer explicit using the DeoxReducer above, how would be it declared?

It should be declared like how Action type was declared. By inferring the return type of HandlerMap(s).

type HandlerMap<TPrevState, TAction, TNextState extends TPrevState> = {
  [type in TAction['type']]: Handler<TPrevState, TAction, TNextState>
}

export type InferActionFromHandlerMap<
  THandlerMap extends HandlerMap<any, any, any>
> = THandlerMap extends HandlerMap<any, infer T, any> ? T : never

export type InferNextStateFromHandleMap<
  THandlerMap extends HandlerMap<any, any, any>
> = THandlerMap extends HandlerMap<any, any, infer T> ? T : never

function createReducer<
  TPrevState,
  THandlerMap extends HandlerMap<TPRevState, any, any>
>(
  defaultState: TPrevState,
  handlerMapsCreator: (handle: CreateHandlerMap<TPrevState>) => THandlerMap[]
) {
  // ...

 return (
    state = defaultState,
    action: InferActionFromHandlerMap<THandlerMap>
  ): InferNextStateFromHandleMap<THandlerMap> => {
    // ...
  }
}

I hope I understood your question correctly. If not, excuse me for my bad English and please ask your question with more details.

LouizFC commented 5 years ago

That is exactly what I was looking for. I am not a "typescript expert" so I have not yet understood the infer keyword completely.

Your example made it clear in my mind, Thank you very much

the-dr-lazy commented 5 years ago

@LouizFC Are you interested to work on this issue?

LouizFC commented 5 years ago

@thebrodmann I could work in this issue this weekend, but I am not sure exactly what should I do.

Renaming the current Reducer to HandlerReducer and adding a new Reducer with the undefined fix would be sufficient? I could also try to make the return type of createReducer explicit

What do you think should be done?

the-dr-lazy commented 5 years ago

Renaming the current Reducer to HandlerReducer and adding a new Reducer with the undefined fix would be sufficient?

Yes. I suggest the name Handler instead of HandlerReducer. Also, HandlerMap, CreateHandlerMap and createHandlerMap uses the current Reducer type (future Handler). So those should use Handler instead of Reducer.

I could also try to make the return type of createReducer explicit.

I will do it on #55.

Also, I suggest you fork a branch from the next branch and work on it due to the huge refactor in b350de45193a79c65b98fe0075f89a2f529ba23d.

So, I will assign this issue to you. Thank you.

LouizFC commented 5 years ago

@thebrodmann I am having a problem while pushing the changes.

Test Suites: 6 passed, 6 total
Tests:       202 passed, 202 total
Snapshots:   6 obsolete, 6 written, 95 passed, 101 total
Time:        27.952s
Ran all test suites.
npm ERR! code ELIFECYCLE
npm ERR! errno 1
npm ERR! deox@1.4.0 test-dts: `jest -c dts-jest.config.js "--bail"`
npm ERR! Exit status 1
npm ERR! 
npm ERR! Failed at the deox@1.4.0 test-dts script.
npm ERR! This is probably not a problem with npm. There is likely additional logging output above.
npm ERR! A complete log of this run can be found in:
npm ERR!     C:\Users\User\AppData\Roaming\npm-cache\_logs\2019-06-04T19_47_36_896Z-debug.log
husky > pre-push hook failed (add --no-verify to bypass)
error: failed to push some refs to 'https://github.com/LouizFC/deox.git'

Edit: I tried using the "--updateSnapshot" flag, but the problem persists

the-dr-lazy commented 5 years ago

Let's try git push with --no-verify flag to skip the hook.

the-dr-lazy commented 5 years ago

This issue has been solved. Thanks to @LouizFC.