[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

[aleclarson]

@jineshshah36 That sounds awful. Can you provide some examples of how you're using Immer? I would love to help you out. 😉

[jineshshah36]

I appreciate the quick response! Here is an example:

import { produce } from "immer"

interface X {
  a: {
    [k: string]: {
      id: string
      message: string
      created_at: string
      updated_at: string
      client_id: string
      owner_id: string
      creator_id: string
      creator: {
        first_name: string
        last_name: string
      }
      deleted_at: null | string
      reminder_at: null | string
      notified_at: null | string
      via: null | string
      notification_ids: null | string[]
    }
  }
}

const x = (state: X, action: { type: "test" }) =>
  produce(state, draft => {
    switch (action.type) {
      case "test": {
        draft.a.test = {
          id: "",
          message: "",
          created_at: "",
          updated_at: "",
          client_id: "",
          owner_id: "",
          creator_id: "",
          creator: {
            first_name: "",
            last_name: "",
          },
          deleted_at: null,
          reminder_at: null,
          notified_at: null,
          via: null,
          notification_ids: ["hi"],
        }
      }
    }
  })

the type of x is:

(state: X, action: { type: "test" }) => { readonly a: any }

[aleclarson]

@jineshshah36 The easiest workaround is doing produce(...) as X.

Is there a reason you don't want Immer to return a deeply immutable object (other than your code isn't setup for it currently)?

You're not really supposed to mutate your state outside of an Immer producer. TypeScript excels at saving you from accidental mutations in specific areas of your code. And there's always as any as an escape hatch! 🔥

I'm re-opening this issue so others can find it easier.

[jineshshah36]

I agree, but the type of x should be:

(state: X, action: { type: "test" }) => {
  readonly a: {
    [k: string]: Readonly<{
      id: string
      message: string
      created_at: string
      updated_at: string
      client_id: string
      owner_id: string
      creator_id: string
      creator: {
        first_name: string
        last_name: string
      }
      deleted_at: null | string
      reminder_at: null | string
      notified_at: null | string
      via: null | string
      notification_ids: null | string[]
    }>
  }
}

not:

(state: X, action: { type: "test" }) => { readonly a: any }

Using "as" is somewhat an anti-pattern because it breaks type-safety.

[aleclarson]

Using "as" is somewhat an anti-pattern because it breaks type-safety.

It's definitely not the first thing you should jump to. ;)

What version of TypeScript are you on?

[jineshshah36]

I'm on 3.1.6

[markerikson]

So it looks like this issue may be relevant to me after all.

We're working on converting https://github.com/reduxjs/redux-starter-kit to TS. The PR adds "typing tests". One of them is currently exploding:

    Semantic: D:/Projects/redux/redux-starter-kit/src/createReducer.ts (51, 5) Type 'Immutable<S>' is not assignable to type 'S'.
      Type 'S | (S extends AtomicObject ? S : S extends ReadonlyArray<any> ? S[number][] extends S ? { [P in keyof S]: ReadonlyArray<Immutable<S[number]>>; }[keyof S] : ImmutableTuple<S> : { readonly [P in keyof S]: Immutable<S[P]>; })' is not assignable to type 'S'.
        Type 'S extends AtomicObject ? S : S extends ReadonlyArray<any> ? S[number][] extends S ? { [P in keyof S]: ReadonlyArray<Immutable<S[number]>>; }[keyof S] : ImmutableTuple<S> : { readonly [P in keyof S]: Immutable<S[P]>; }' is not assignable to type 'S'.
          Type '(Function & object & S) | (String & object & S) | (Number & object & S) | (Boolean & object & S) | (RegExp & object & S) | (Date & object & S) | (Promise<any> & object & S) | ... 4 more ... | (S extends ReadonlyArray<...> ? S[number][] extends S ? { [P in keyof S]: ReadonlyArray<...>; }[keyof S] : ImmutableTuple<......' is not assignable to type 'S'.
            Type 'S extends ReadonlyArray<any> ? S[number][] extends S ? { [P in keyof S]: ReadonlyArray<Immutable<S[number]>>; }[keyof S] : ImmutableTuple<S> : { readonly [P in keyof S]: Immutable<S[P]>; }' is not assignable to type 'S'.
              Type '(S[number][] extends S ? { [P in keyof S]: ReadonlyArray<Immutable<S[number]>>; }[keyof S] : ImmutableTuple<S>) | { readonly [P in keyof S]: Immutable<S[P]>; }' is not assignable to type 'S'.
                Type 'S[number][] extends S ? { [P in keyof S]: ReadonlyArray<Immutable<S[number]>>; }[keyof S] : ImmutableTuple<S>' is not
assignable to type 'S'.
                  Type '{ [P in keyof S]: ReadonlyArray<Immutable<S[number]>>; }[keyof S] | ImmutableTuple<S>' is not assignable to type 'S'.
                    Type 'ReadonlyArray<Immutable<S[number]>>' is not assignable to type 'S'.

Looks related.

Suggestions?

[aleclarson]

@jineshshah36 Have you tried with 3.2.2? No reason not to upgrade, AFAICT.

@markerikson I'll give it a look in the morning! 😊

[jineshshah36]

Actually, 3.2 breaks JSX resolution which breaks many react related libraries and apps. As a result, I can’t upgrade right now. In addition, if it does work in 3.2, this update should be a breaking change not a bugfix. I’ll try on 3.2 when I’m in front of a computer.

[aleclarson]

Actually, 3.2 breaks JSX resolution which breaks many react related libraries and apps.

Is there an issue tracking this in the TypeScript repo?

In addition, if it does work in 3.2, this update should be a breaking change not a bugfix.

I think 3.2 fixed a bug that 3.1 had (involving recursive types). If it were a 3.2+ feature, you'd think it would be mentioned in Breaking Changes or the Release Notes.

Users of 3.1 should pin their Immer version to 1.10.2 for now, at least.

[alitaheri]

This broke a lot of code, why should we enforce immutibility at type level?

I think produce should return the exact type it consumes.

I understand that it's a good practice, but this means a lot of things:
I can't reuse interfaces for both mutable and immutable data structures.
I have to explicitly type the draft as it's broken.
Arrays are now ReadonlyArrays which is very painful to work with.

If someone want's to explicitly type their state as readonly let them be. why should we dictate the way programs are built?

@mweststrate @aleclarson There are way too many constraints imposed by this change. Please don't let this awesome project drift away from it's community.

P.S. @aleclarson Please write exhaustive ts tests before changing the types, there are too many ts breaks and amendments recently 😅

[knpwrs]

Is there a reason you don't want Immer to return a deeply immutable object (other than your code isn't setup for it currently)?

I personally think it makes sense for immer to transparently return whatever it is given. Before #161 it wasn't even possible to use readonly, requiring readonly (or enforcing it on the output) seems like a semver major change.

[aleclarson]

• I can't reuse interfaces for both mutable and immutable data structures.
• I have to explicitly type the draft as it's broken.
• Arrays are now ReadonlyArrays which is very painful to work with.

Please provide examples to back these claims. Thanks!

 

If someone want's to explicitly type their state as readonly let them be. why should we dictate the way programs are built?

The rationale is that deep immutability is better, because it fits the default behavior of Immer. If you're using setAutoFreeze(false), then you need to "cast" the result of your produce calls back to the mutable type you're expecting.

I wouldn't be opposed to adding a mutableProduce export that has less strict typing. Then you could do import { mutableProduce as produce } from 'immer'. Thoughts?

 

Please write exhaustive ts tests before changing the types, there are too many ts breaks and amendments recently

Not sure how to verify the exhaustiveness of the current test suite. Any recommendations? Please contribute some tests that you feel are missing.

 

I personally think it makes sense for immer to transparently return whatever it is given. Before #161 it wasn't even possible to use readonly, requiring readonly (or enforcing it on the output) seems like a semver major change.

Apart from pesky generic types, how else is deep immutability causing issues? I'm leaning towards the opinion that any code broken by deep immutability wasn't written correctly in the first place, but feel free to prove me wrong.

[knpwrs]

I'm leaning towards the opinion that any code broken by deep immutability wasn't written correctly in the first place, but feel free to prove me wrong.

I wouldn't disagree on principle, but I think immer falls more on the "tool" end of the spectrum of tool <=> opinionated framework, if that makes sense.

I'd also still argue that such a change would merit a semver major release.

[aleclarson]

I wouldn't disagree on principle, but I think immer falls more on the "tool" end of the spectrum of tool <=> opinionated framework, if that makes sense.

Immer is specifically a tool for working with deeply immutable trees, so it's not really pushing an opinion on anyone by enforcing immutability in its type defs, IMO.

I'd also still argue that such a change would merit a semver major release.

This seems like a gray area. While this change obviously breaks some assumptions, it's questionable whether those assumptions were ever officially supported in the first place. I'm open to moving this change to 2.0 if @mweststrate agrees or you can provide a compelling use case that deep immutability prevents.

[jineshshah36]

I can confirm that #289 (comment) compiles on 3.2.2. As a result, I think simply bumping immer to 2.0 and saying that 2.0 requires 3.2 or higher is the best move. I have seen this done by many other frameworks. The problem is that everyone wrote their code expecting the type to not be readonly, and all of a sudden it is. This should not be a problem for the compiled JS as we shouldn't be mutating our data anyways. However, simple cases no longer compile. Example:

import { produce } from "immer"
import { Reducer } from "redux"

interface BarAction {
  type: "bar"
}

export interface FooState {
  foo: {
    [k: string]: {
      bar: string[]
    }
  }
}

export const notes: Reducer<FooState, BarAction> = (
  state = { foo: {} },
  action,
) =>
  produce(state, draft => {
    switch (action.type) {
      case "bar": {
        draft.foo.bar.bar = ["hi"]

        return
      }
    }
  })

This does not compile because produce converts FooState into { foo: { readonly [k: string]: { readonly bar: ReadonlyArray<string> } } }, and in TS ReadonlyArray<string> is not assignable to string[]

[aleclarson]

I appreciate the feedback very much! I wonder, would you guys have provided feedback if I asked before making this change? Often, people are only willing to give feedback after something breaks, since it gets brought to their immediate attention. Though, I promise this wasn't my intention.

For now, I will revert this change and consider less heavy-handed approaches.

Again, thanks for the feedback! ❤️

[jineshshah36]

I am definitely willing to give feedback moving forward if you would like. You’ve shown yourself to be quite understanding and patient, which I appreciate. Thanks for building an awesome lib!

[aleclarson]

🎉 This issue has been resolved in version 1.10.5 🎉

The release is available on:

• npm package (@latest dist-tag)
• GitHub release

Your semantic-release bot 📦🚀

[alitaheri]

@aleclarson Thank you for fixing this on such short notice 👍 👍

Although the decision has been made, I will provide real world examples to back my claim:

I work on a project based on openfin, which means there are a lot of web-based electron windows running. the communication between them is based on IPC. to simplify state management I have to broadcast every redux action across the entire platform, this means that each action and reducer can be shared between many component of different project structures (some are react+redux, some are angular, some are jquery), this is an outcome of mixing modern and legacy code. I have to integrate a lot of data structure between redux and the old way, this means I need typescript interfaces to be used for both sides. I never break the redux's principles, but I need the data structures to be shared across them. to migrate the entire project will take months. To fix the typings I have to change a lot of code. and not just add some readonly to interfaces, I will need to cast in a LOT of places.

I'm saying the world is not perfect, and as engineers we should not limit but provide options. I understand the novelty of redux's immutability principles and I follow them very thoroughly. But not at the type level. I use redux-immutable-state-invariant to make sure I'm fine at run-time.

I would suggest adding a typing principle section to readme and ask people to make their interfaces readonly using the Immutable helper. But not limit them.

This is because the freedom of choice covers more use-cases than strict principles. My argument is validated by even the core typescript team, they put every restrictive rule behind a flag. explicit any typing, unused locals, etc. etc.

This is because as engineers we should not enforce ideologies like religion but instead simply provide solutions. immer is the best solution to the headache of working with immutable data structures in a mutable language. It shouldn't enforce it's ideas, but simply just provide solution.

What if I wanna use immer outside of redux, I can right? why shouldn't I be able to do that? I wanna use immer in a mutable environment (I will always disable auto freeze) simply because I want a modified clone sometimes. immer can solve that too.

About the draft:

the draft didn't remove readonly for me, doesn't matter now

About the readonly arrays:

readonly array is not array, that means any function that requires arrays should now only accept readonly arrays, other libraries aren't built with immer in mind! This is what i mean by being hard on a community, by enforcing these rules composibility is compromised!

The rationale is that deep immutability is better, because it fits the default behavior of Immer. If you're using setAutoFreeze(false), then you need to "cast" the result of your produce calls back to the mutable type you're expecting.

better? yeah it's better, I absolutely agree with that, but think bigger. not every use-case fits this. and forcing a lot of typescasts on the user because the library dev says what's better and what's not is not very good for a community. and by a lot I mean thousands, I have 500 reducer functions using immer, they work just fine the way they are, I've beed using immer for 8 months, and I'm very happy with it the way it is, never had any issues with typings until this change.

Not sure how to verify the exhaustiveness of the current test suite. Any recommendations? Please contribute some tests that you feel are missing.

If I had time I certainly would have given back, unfortunately I don't. I just mean be a bit more careful. I have no right to say anything here, I should write tests myself instead of bragging. I take this back 😅

@aleclarson If you still disagree with me, the solution is simple open a poll and get people to vote on it. Let the community decide.

P.S. Thanks for helping maintain this great library without which I would have wasted a lot of time debugging accidental mutations. I don't mean any offense in anyway, these are just professional discussions. Your contributions are very well appreciated ❤️ ❤️

[aleclarson]

@alitaheri I appreciate you taking the time to respond despite this issue being resolved! 👏 It's good to know what Immer users expect from the library. Cheers 🍻

[mweststrate]

I totally lost track of this issue, but in the future (Immer 5.3 and up) their will be always a bail out on the produced types through the utilities castDraft and castImmutable which makes it possible to obtain either variant of the source type you want.