Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Make "DeepImmutable" optional. #58

Closed
LouizFC opened this issue May 17, 2019 · 7 comments · Fixed by #86
Closed

Make "DeepImmutable" optional. #58

LouizFC opened this issue May 17, 2019 · 7 comments · Fixed by #86
Assignees
Labels
enhancement New feature or request
Milestone

Comments

@LouizFC
Copy link
Contributor

LouizFC commented May 17, 2019

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 the-dr-lazy added enhancement New feature or request help wanted Extra attention is needed labels May 18, 2019
@the-dr-lazy the-dr-lazy added this to the v1.4.1 milestone May 18, 2019
@the-dr-lazy
Copy link
Owner

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
Copy link
Contributor

Haaxor1689 commented May 19, 2019

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
Copy link
Contributor Author

LouizFC commented May 19, 2019

@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
Copy link
Owner

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

@LouizFC
Copy link
Contributor Author

LouizFC commented May 20, 2019

@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
Copy link
Owner

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.

@the-dr-lazy the-dr-lazy modified the milestones: v1.5.0, v2.0.0 May 28, 2019
@the-dr-lazy the-dr-lazy self-assigned this Jun 10, 2019
@the-dr-lazy the-dr-lazy removed the help wanted Extra attention is needed label Jun 10, 2019
@anilanar
Copy link

anilanar commented Jun 12, 2019

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.

the-dr-lazy pushed a commit that referenced this issue Jun 12, 2019
BREAKING CHANGE: The input/prev state argument in createHandlerMap (AKA handleAction) and returning
reducer of createReducer will not obligated to be Immutable data structure by Deox which in turn can
lead to changes in handler's and reducer's return type.

closes #58 and #55
the-dr-lazy pushed a commit that referenced this issue Jun 15, 2019
BREAKING CHANGE: The input/prev state argument in createHandlerMap (AKA handleAction) and returning
reducer of createReducer will not obligated to be Immutable data structure by Deox which in turn can
lead to changes in handler's and reducer's return type.

closes #58 and #55
the-dr-lazy pushed a commit that referenced this issue Jun 15, 2019
BREAKING CHANGE: The input/prev state argument in createHandlerMap (AKA handleAction) and returning
reducer of createReducer will not obligated to be Immutable data structure by Deox which in turn can
lead to changes in handler's and reducer's return type.

closes #58 and #55
the-dr-lazy pushed a commit that referenced this issue Jun 15, 2019
# [2.0.0](v1.4.1...v2.0.0) (2019-06-15)

### Features

* export immutibility type helpers in public API ([b723d35](b723d35))
* make DeepImmutable optional in input/prev state ([0f35d7f](0f35d7f)), closes [#58](#58) [#55](#55)

### BREAKING CHANGES

* The input/prev state argument in createHandlerMap (AKA handleAction) and returning
reducer of createReducer will not obligated to be Immutable data structure by Deox which in turn can
lead to changes in handler's and reducer's return type.
the-dr-lazy pushed a commit that referenced this issue Jun 15, 2019
# [2.0.0](v1.4.1...v2.0.0) (2019-06-15)

### Features

* export immutibility type helpers in public API ([b723d35](b723d35))
* make DeepImmutable optional in input/prev state ([0f35d7f](0f35d7f)), closes [#58](#58) [#55](#55)

### BREAKING CHANGES

* The input/prev state argument in createHandlerMap (AKA handleAction) and returning
reducer of createReducer will not obligated to be Immutable data structure by Deox which in turn can
lead to changes in handler's and reducer's return type.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants