[v1.10.3] Immutable<T> broke my codebase

[knpwrs]

Hey, there! I'm the guy that provided the original mapped type for removing readonly modifiers in TypeScript over in #161.

It's awesome to see this library gain so much traction since then, though the typings have also evolved to the point where I can barely recognize them. The latest patch release of immer broke a library I wrote (my tests pass using immer@1.10.2 but not immer@1.10.3). It appears to be purely a typing thing, but I'm having a heck of a time resolving the issues. I was hoping somebody who has worked on the typings more recently than I have could give some pointers/feedback/maybe a PR against my project (or maybe we might determine that the typings are broken and not my usage and then we can fix the problem here).

The issue Greenkeeper opened against my repo can be seen here: knpwrs/redux-ts-utils#1

This is the code that was working with immer@1.10.2:

export default function handleAction<S, AC extends TsActionCreator<any> = any>(
  ac: AC,
  re: TsReducer<S, ReturnType<AC>>,
  s?: S,
): Reducer<S, ReturnType<AC>> {
  return produce((draft: Draft<S>, action: ReturnType<AC>) => {
    if (action.type === ac.type) {
      re(draft, action);
    }
  }, s);
}

Lots of red all over the place. Here's where I've gotten so far with immer@1.10.3:

export default function handleAction<S, AC extends TsActionCreator<any> = any>(
  ac: AC,
  re: TsReducer<S, ReturnType<AC>>,
  s?: S,
): Reducer<S, ReturnType<AC>> {
  return produce<S, Draft<S>, [ReturnType<AC>]>((draft, action) => {
    if (action.type === ac.type) {
      re(draft, action);
    }
  }, s);
}

Now the only thing that is red is that second-to-last line where I'm passing s as the default value to produce. TypeScript complains:

Argument of type 'S | undefined' is not assignable to parameter of type 'S'.
  Type 'undefined' is not assignable to type 'S'.

Nothing I'm trying is resolving the issue (even type casting or the dreaded ! operator which results in more errors). I want s to remain an optional parameter.

Any ideas?

[aleclarson]

As a quick fix, you can do produce(...) as any. Can you come up with a simpler repro that we can add as a test case?

[knpwrs]

I wound up having to do s as S as well as casting the whole produce() call as any:

  return produce<S, Draft<S>, [ReturnType<AC>]>((draft, action) => {
    if (action.type === ac.type) {
      re(draft, action);
    }
  }, s as S) as any; // see https://github.com/mweststrate/immer/issues/289

Can you come up with a simpler repro that we can add as a test case?

I'll see if I can come up with something real quick.

[knpwrs]

Nothing immediately comes to mind for a simple reproducible test case, but if I come up with something I'll open a PR. In the meantime Greenkeeper keeps a watchful eye.

Thanks for the help!

[aleclarson]

Type 'S | undefined' is not assignable to type 'Immutable<Draft<S>> | undefined'.

The issue is that Immutable<Draft<S>> can't be simplified to S because it's a generic parameter.

By default, Immer freezes any object it makes a copy of. I recently changed the type signature of produce to better reflect this. I changed its return type to be a deeply immutable version of the draft state type. Clearly, this has broken some assumption(s) that some users (like yourself) have made about Immer. Now the question is, would it be better to (1) remove the Immutable<T> type I've added and give users the responsibility of making their object types deeply immutable, or (2) figure something else out.

Having produce always return a deeply immutable type is preferrable IMO, because you're "not supposed to" be making mutations outside of Immer.

Does Redux not enforce immutability in its type defs?

[aleclarson]

This works on my end:

  return produce((draft: Draft<S>, action: ReturnType<AC>) => {
    if (action.type === ac.type) {
      re(draft, action);
    }
  }, s) as any;

[knpwrs]

No, the readonly modifier does not appear anywhere in redux: https://github.com/reduxjs/redux/blob/632da6c902d2637082f197669a7e81bd3aaba7b3/index.d.ts

[knpwrs]

This works on my end

Ah, nice. That gets me back to the original non-diamond syntax. I was moving forward with moving the types to the diamond.

[aleclarson]

No, the readonly modifier does not appear anywhere in redux: https://github.com/reduxjs/redux/blob/632da6c902d2637082f197669a7e81bd3aaba7b3/index.d.ts

Interesting. Yet, the Redux docs say immutability is required. Maybe Redux should enforce immutability in its types like Immer does?

cc @gaearon @markerikson

[markerikson]

I'll quote my post The Tao of Redux, Part 1: Implementation and Intent:

The core Redux createStore function itself puts only two limitations on how you must write your code: actions must be plain objects, and they must contain a defined type field. It does not care about immutability, serializability, or side effects, or what the value of the type field actually is.

That said, the commonly used pieces around that core, including the Redux DevTools, React-Redux, React, and Reselect, do rely on proper use of immutability, serializable actions/state, and pure reducer functions. The main application logic may work okay if these expectations are ignored, but it's very likely that time-travel debugging and component re-rendering will break. These also will affect any other persistence-related use cases as well.

It's also important to note that immutability, serializability, and pure functions are not enforced in any way by Redux. It's entirely possible for a reducer function to mutate its state or trigger an AJAX call. It's entirely possible for any other part of the application to call getState() and modify the contents of the state tree directly. It's entirely possible to put promises, functions, Symbols, class instances, or other non-serializable values into actions or the state tree. You are not supposed to do any of those things, but it's possible.

Having said that, our new redux-starter-kit package now adds middleware by default that will check for mutations and non-serializable values in the state, as an attempt to help enforce proper practices.

[aleclarson]

@markerikson In the context of TypeScript, I don't see the benefit of not enforcing immutability. You can easily use as any as an escape hatch.

[markerikson]

I don't use TS yet myself, so I don't know what the typings do or don't say.

[aleclarson]

Have you (or Dan or anyone) done a Twitter poll asking which Redux users are using TypeScript with it? Anyway, if Redux users aren't making a fuss about lack of deep immutability enforcement, then it's probably not a big issue. Anyone using Redux and Immer together will need to use as any when using generic parameters (in certain edge cases, that is).

[jineshshah36]

I don't understand what the purpose of https://github.com/mweststrate/immer/releases/tag/v1.10.3 was, but it has broken every single produce I'm using throughout my TS app. The types seem to be broken because it converts everything to any types. It looks like the Immutable generic type is not written correctly