the-dr-lazy / deox

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

Make "DeepImmutable" optional. #58

Closed LouizFC closed 5 years ago

LouizFC commented 5 years ago

Greetings.

I am having some trouble passing the state to other functions:

export interface HttpUriStore {
  rawUrl: string;
  query: Param[];
  method: string;
}

export function updateUrl(
  state: HttpUriStore,
  index: number,
  newParam: Param
): string

export const HttpUriReducer = createReducer(uriDefaultState, (handle) => [
  handle(updateRawUrl, (state, { payload }) => {
/*   TS2345: Argument of type 'DeepImmutableObject<HttpUriStore>' is not assignable to parameter of type 'HttpUriStore'.
  Types of property 'query' are incompatible.
    Type 'DeepImmutableArray<Param>' is missing the following properties from type 'Param[]': pop, push, reverse, shift, and 6 more.
                v*/
    updateUrl(state, 0, {} as Param);
    return produce(state, (draft) => {
      draft.rawUrl = payload;
    });
  }),
])

Is there a way to "disable" The DeepImmutable type wrapper? I understand that it can prevent acidental mutations, but I think it should be optional

I tried the following:

import { DeepImmutable } from "deox/dist/types";

function recalculateParams(
  state: DeepImmutable<HttpUriStore>,
  index: number,
  newParam: Param
) {

The import gives the following warn: unable to resolve path to module, but that is a @typescript-eslint bug I think, because it compiles.

Also as a minor incovenience: I am using immer, and because of the immutable wrapper I need to specify the draft type( (draft: Draft<State>)=> ) when I using splice or other commands inside the draft function.

the-dr-lazy commented 5 years ago

Hi @LouizFC, There is also another open issue on #55 that I think share the same cause.

@rlesniak @Haaxor1689 It would be great to have you guys on this to make a good decision.

I'm confused about DeepImmutable, looks like its trouble is over the benefits. I think it is a good idea to make DeepImmutable optional like follows:

import { createActionCreator, createReducer, DeepImmutable } from 'deox'

type RootState = DeepImmutable<{ counter: number }>

const increment = createActionCreator('INCREMENT')

const handleIncrement = (state: RootState) => ({ ...state, count: state.count + 1 })

const defaultState: RootState = { counter: 0 }

const rootReducer = createReducer(defaultState, handleAction => [
  handleAction(increment, handleIncrement)
])
Haaxor1689 commented 5 years ago

Your updateUrl function should take the state as DeepImmutable<HttpUriStore> and return new DeepImmutable<HttpUriStore>. These functions should always be pure. That's why this library automatically wraps the state inside the DeepImmutable type. Also your state should have all it's members readonly like this:

export interface HttpUriStore {
  readonly rawUrl: string;
  readonly query: Param[];
  readonly method: string;
}

And Param interface should have all it's members readonly as well etc.

Making the DeepImmutable optional is going against the philosophy of redux in my opinion.

LouizFC commented 5 years ago

@Haaxor1689 all my functions are pure, I just forgot to make the return type explicit.

While I understand that DeepImmutable gives you extra safety and enforces redux philosophy, I do not want to "spread" all these types on my projects just so I can use the utility that deox provides

I really enjoy using deox, but I think that "managing immutability" should be an optional part of this lib, not something that should be forced to the user.

For example, in this project I am using immer to manage my immutable state, so I don't need the extra layer that deox provides.

the-dr-lazy commented 5 years ago

In #45 I have tested Deox with immer. @LouizFC do you confirm that draft type is unwrapped from DeepImmutable?

LouizFC commented 5 years ago

@LouizFC I confirm, with the exception of arrays.

I need to type the draft type explicitly when using array specific methods.

// no need to specify type
 handle(actions.updateParam, (state, { payload }) =>
      produce(state, (draft) => {
        draft[payload.index] = payload.param;
      })
    ),
// need to specify type, otherwise compile error
    handle(actions.reorderParams, (state, { payload }) =>
      produce(state, (draft: Draft<Param[]>) => {
        draft.splice(payload.fromIndex, 1);
        draft.splice(payload.toIndex, 0, state[payload.fromIndex]);
      })
    ),

But that is a minor inconvenience, I don't think it matter much.

Edit: Looking at another angle, specifying the draft type makes the code alot easier for those who are not familiar with Immer, so I think it is more a plus than a minus here

the-dr-lazy commented 5 years ago

This change looks like a breaking change to me! I'm not 100% sure; So if you have any other opinion let us know but until then I will change the milestone to v2.0.0.

anilanar commented 5 years ago

I understand the motivation behind DeepImmutable but I've been working with React/Redux apps for 4 years (a huge team and some smaller teams) and I've never seen a PR or a bug that ever tried to mutate action payload/redux state and we use native JS objects/arrays without helper libraries.