the-dr-lazy / deox

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

prev state vs next state #114

Closed SantoJambit closed 5 years ago

SantoJambit commented 5 years ago

createReducer infers the next state from the actions instead of using the previous state, and I wonder why that is, because it creates weird types on the State interfaces.

For example, when I try this

For example, with the following sample code:

import { createReducer, createAction } from 'deox';

interface TestType {
    foo: string;
    bar: number;
}

interface TestState {
    [s: string]: TestType;
}

const initialState: TestState = {};

export const addTest = createAction('ADD_TEST', (resolve) => (id: string, type: TestType) => resolve({ id, type }));

const testReducer = createReducer(initialState, (handleAction) => [
    handleAction(addTest, (state, action) => ({
        ...state,
        [action.payload.id]: {
            ...state[action.payload.id],
            ...action.payload.type,
        },
    })),
]);

type CombinedState = ReturnType<typeof testReducer>;

I get this as CombinedState (when inspecting via vscode):

type CombinedState = {
    [x: string]: TestType | {
        foo: string;
        bar: number;
    };
}

And this gets worse with more reducers and actions.

Should the initialState not have the same type definition as anything that comes out of the reducer? Why the separation of both?

Obviously, I can create a wrapper function for this:

function createReducerClean<TPrevState, THandlerMap extends HandlerMap<TPrevState, any, any>>(defaultState: TPrevState, handlerMapsCreator: (handle: CreateHandlerMap<TPrevState>) => THandlerMap[]): (state: TPrevState | undefined, action: InferActionFromHandlerMap<THandlerMap>) => TPrevState {
    return createReducer(defaultState, handlerMapsCreator);
}

But aside from this being additional maintenance work, I wonder why this is done in the first place.

Btw, thanks for deox, it helps a lot.

the-dr-lazy commented 5 years ago

createReducer infers the next state from the actions instead of using the previous state

In fact, createReducer infer next state type from handlers map which in each handler the next state should be extended from the previous one. So if we make the relation simpler the next state type produces by createReducer should just extend the previous state in Deox. But why not just <State>(state: State, action) => State? Is there any difference between the previous state type and next state type?

The previous state that given to a reducer can be:

In the second case, you can see the next state type can be different from the previous state type. But there is an issue with the type signature in the second case. Imagine the following example:

type NamesState = DeepImmutableArray<string>

const defaultNamesState: NamesState = Object.freeze([])

const namesReducer = createReducer(defaultNamesState, handleAction => [
  handleAction(actionB, (state) => state) // DeepImmutableArray<string>
])

namesReducer(...) // DeepImmutableArray<string>

What if the reducer type in the above example was <State>(state: Immutable<State>, action) => State | Immutable<State>? The next state type instead of being DeepImmutableArray<string> will become string[] | DeepImmutableArray<string> and that was wrong. The TypeScript, in that case, will lie to the developer. So unlike other type signatures for reducer which have been defined in Redux or other projects, Deox considers the difference between the previous state and next state but they have relation, the next state should extend the previous one.

BTW, the whole history behind this can be found in #55 and #58.

SantoJambit commented 5 years ago

Thanks for the explanation.

I tried reproducing your example, but I keep getting DeepImutableArray<string> for CombinedType here:

import { createAction, DeepImmutableArray, Immutable } from 'deox';
import { HandlerMap, CreateHandlerMap, InferActionFromHandlerMap, createHandlerMap } from 'deox/dist/create-handler-map';
const merge = <T extends {}>(...objs: T[]): T =>
  Object.assign({}, ...objs)

export function createReducer<
  TPrevState,
  THandlerMap extends HandlerMap<TPrevState, any, any>
>(
  defaultState: Immutable<TPrevState>,
  handlerMapsCreator: (handle: CreateHandlerMap<TPrevState>) => THandlerMap[]
) {
  const handlerMap = merge(...handlerMapsCreator(createHandlerMap))

  return (
    state: Immutable<TPrevState> = defaultState,
    action: InferActionFromHandlerMap<THandlerMap>
  ): TPrevState | Immutable<TPrevState> => {
    const handler = handlerMap[action.type]

    return handler ? handler(<any>state, action) : state
  }
}
type TestState = DeepImmutableArray<string>

const initialState: TestState = Object.freeze([])

export const actionA = createAction('ACTION_A', (resolve) => (foo: string) => resolve(foo));
export const actionB = createAction('ACTION_B');

const testReducer = createReducer(initialState, (handleAction) => [
  handleAction(actionA, (state, { payload }) => [...state, payload]), // string[]
  handleAction(actionB, (state) => state) // DeepImmutableArray<string>
]);

type CombinedState = ReturnType<typeof testReducer>;

The original createReducer does show DeepImmutableArray<string> | string[] instead, which is wrong according to your comment.. or did you mix up that sentence?

SantoJambit commented 5 years ago

Tried simplifying this even further by removing all referenced to Immutable* in createReducer and it still returns the same type:

import { createAction, DeepImmutableArray } from 'deox';
import { CreateHandlerMap, InferActionFromHandlerMap, createHandlerMap, HandlerMap } from 'deox/dist/create-handler-map';
import { Handler } from 'deox/dist/types';
const merge = <T extends {}>(...objs: T[]): T =>
  Object.assign({}, ...objs)

export function createReducer<
  TPrevState,
  THandlerMap extends HandlerMap<TPrevState, any>
>(
  defaultState: TPrevState,
  handlerMapsCreator: (handle: CreateHandlerMap<TPrevState>) => THandlerMap[]
) {
  const handlerMap = merge(...handlerMapsCreator(createHandlerMap))

  return (
    state = defaultState,
    action: InferActionFromHandlerMap<THandlerMap>
  ): TPrevState => {
    const handler = handlerMap[action.type]

    return handler ? handler(<any>state, action) : state
  }
}
type TestState = DeepImmutableArray<string>;

const initialState: TestState = Object.freeze([])

export const actionA = createAction('ACTION_A', (resolve) => (foo: string) => resolve(foo));
export const actionB = createAction('ACTION_B');

const testReducer = createReducer(initialState, (handleAction) => [
  handleAction(actionA, (state, { payload }) => [...state, payload]), // string[]
  handleAction(actionB, (state) => state) // DeepImmutableArray<string>
]);

type CombinedState = ReturnType<typeof testReducer>; // DeepImmutableArray<string>

Your other sample from #55 throws an error like expected with the above createReducer:

  handleAction(actionB, (state) => Object.freeze(state)) /* throws error
                                                            because of that input/prev state 
                                                            defined as a mutable array
                                                            but the output/next state is a `ReadonlyArray` */
//...
type CombinedState = ReturnType<typeof testReducer>; // string[]
the-dr-lazy commented 5 years ago

The next state type instead of being DeepImmutableArray will become string[] | DeepImmutableArray and that was wrong.

I meant here that the correct type is DeepImmutableArray<string>. So string[] | DeepImmutableArray<string> is wrong. Sorry if it is confusing.

Your 1st comment after mine:

I tried reproducing your example, but I keep getting DeepImutableArray for CombinedType here:

I was trying to demonstrate it to you. In fact, I think it's not possible to infer the input type of DeepImmutable. The correct implementation is something as follows:

type Primitive = undefined | null | boolean | string | number | Function

type Mutable<T> = T extends Primitive
  ? T
  : T extends ReadonlyArray<infer V>
  ? V[]
  : T extends ReadonlyMap<infer K, infer V>
  ? Map<K, V>
  : never // so on...

export function createReducer<
  TPrevState,
  THandlerMap extends HandlerMap<TPrevState, any, any>
>(
  defaultState: TPrevState,
  handlerMapsCreator: (
    handle: CreateHandlerMap<DeepImmutable<TPrevState>>
  ) => THandlerMap[]
) {
  const handlerMap = merge(...handlerMapsCreator(createHandlerMap))

  return (
    state = defaultState,
    action: InferActionFromHandlerMap<THandlerMap>
  ): TPrevState | Mutable<TPrevState> => {
    const handler = handlerMap[action.type]

    return handler ? handler(<any>state, action) : state
  }
}

The original createReducer does show DeepImmutableArray | string[] instead, which is wrong according to your comment.. or did you mix up that sentence?

The correct type in the tested case is string[] or DeepImmutableArray<string> in handlers. The issue is with implementation. I used <State>(state: Immutable<State>, action) => State | Immutable<State> type signature for demonstration, it is not correct.

Your 2nd comment after mine:

Tried simplifying this even further by removing all reference to Immutable* in createReducer and it still returns the same type:

When the default reducer state is mutable there is no problem. But if the default reducer state you give to the reduce is immutable you become obligated to return immutable data structure in handlers. To clarify, you can test the following example:

type NamesState = DeepImmutableArray<string>

const defaultNamesState: NamesState = Object.freeze([])

const namesReducer = createReducer(defaultNamesState, handleAction => [
  handleAction(actionB, (state) => [...state]) /* throws error
                                                  because the expected return type of handler
                                                  is `DeepImmutableArray<string>`
                                                  but it's returning `string[]` */
])

namesReducer(...) // DeepImmutableArray<string>
SantoJambit commented 5 years ago

I meant here that the correct type is DeepImmutableArray<string>. So string[] | DeepImmutableArray<string> is wrong. Sorry if it is confusing.

OK, so I read it correctly the first time.

This is a correct type in the tested case because you are really producing string[] or DeepImmutableArray<string> in handlers. But even your handlers only return DeepImmutableArray<string> the reducer's return type still become string[] | DeepImmutableArray<string> which is wrong.

Actually, no.. my latest sample does have a return type of DeepImmutableArray<string>. That's what confused me in the first place, as the latest version of deox (2.1.0) returns string[] | DeepImmutableArray<string>, which you just said is wrong. Here you can see that the simplified reducer returns the correct type:

Screenshot from 2019-08-28 16-20-51

When the default reducer state is mutable there is no problem. But if the default reducer state you give to the reduce is immutable you become obligated to return immutable data structure in handlers. To clarify, you can test the following example:

That sample throws no error for me:

Screenshot from 2019-08-28 16-24-48

the-dr-lazy commented 5 years ago

@SantoJambit I updated the comment.

the-dr-lazy commented 5 years ago

as the latest version of deox (2.1.0) returns string[] | DeepImmutableArray, which you just said is wrong.

If one of your handlers is returning string[] and another one is returning DeepImmutableArray<string> so the reducer return type should be string[] | DeepImmutableArray<string>. If all of your handlers only returning string[] so the reducer return type should be string[] and if all of your handlers are returning DeepImmutableArray<string> so the reducer return type should be DeepImmutableArray<string>. Are you trying to say that Deox@2.1.0 is behaving rather than this?

That sample throws no error for me:

This is because you are using the new version of Handler type: https://github.com/thebrodmann/deox/blob/65e4c1bc11c098fac499eca5b6e2d4a1d868d172/src/types.ts#L38-L42

As you can see in type signature the NextState is extending from PrevState and it is not explicitly PrevState. You can checkout to https://github.com/thebrodmann/deox/commit/0ce4fa134d7a1fe26063c941efc77bf3f450076d commit and test the example and it should throw the error 😁.

the-dr-lazy commented 5 years ago

I provided a codesandbox for testing the return type of reducer.

SantoJambit commented 5 years ago

Thanks for the code sandbox, that helped to make it click. I get the following types with 2.1.0:

type MixedReducerReturnType = ReturnType<typeof mixedReducer>; // string[] | DeepImmutableArray<string>
type MutableReducerReturnType = ReturnType<typeof mutableReducer>; // string[]
type ImmutableReducerReturnType = ReturnType<typeof immutableReducer>; // DeepImmutableArray<string>

Could not find a solution that creates better return types and still works with the above :\