Improves type safety in immer.d.ts and fixes 97

[ghost]

Summary

• Fixed two-arg produce() curries being returned as one-arg curries
• Added a one-arg produce() curry signature
• Current state may now be Readonly via new type MaybeReadonly
• produce() and curries now return Readonly
• Added Draft that ensures the draft is always writable (fixes 97)
• Reversed the order of produce() signatures so that the most
  specific is used first
• Added non-doc style comments to curry-producing produce() variants
• Added a note to the varadic signature of produce() about Typescript
  issue 5453

Examples

import produce from "immer";
import { Draft } from "immer";

////
// produce() now returns a Readonly<S> so that mutations of frozen state
// are caught at compile-time.
////
interface MyState { value: number; }

let state: MyState = { value: 0 };

// Both `this` inside a recipe and the new state object are now of type
// Readonly<S>.
// Before, assignments to properties of `this` and `next` would have been
// undetected, now the compiler emits an error:
//   error TS2540: Cannot assign to 'value' because it is a constant or a
//   read-only property.
let next = produce(state, draft => {
    this.value = 1; // Compile error
    draft.value++; // Fine thanks to Draft<S>
});
next.value = 4; // Compile error

////
// The first argument to produce() and curries is now S|Readonly<S> and the
// drafts object is of type Draft<S>.
//
// Additionally the order of the currying function signatures has been
// reversed. This means that the most specific is selected first.
//
// This fixes https://github.com/mweststrate/immer/issues/97
////

// 0 argument curry, matches: produce<S>
const increment = produce((draft: Draft<MyState>) => {
    draft.value++;
});
next = increment(next);

// 1 argument curry, matches: produce<S, A>
// Likewise for the 2 and 3 argument variations.
const assign = produce((draft: Draft<MyState>, value: number) => {
    draft.value = value;
});
next = assign(next, 42); // Fine
next = assign(next); // Compile error

const generic = produce((draft: Draft<MyState>, a: string, b: number, c: string, d: number) => {
    console.log(typeof a); // string
    console.log(typeof b); // number
    console.log(typeof c); // string again
    console.log(typeof d); // number again
    draft.value = b + d;
});
// see: https://github.com/Microsoft/TypeScript/issues/5453
generic(next); // NOT a compile error

[olavim]

The following case fails:

interface State extends Readonly<{
  foo: {
    readonly val: boolean;
  }
}> {}

const state: State = {
  foo: {
    val: true
  }
};

produce(state, (draft: Draft<State>) => {
  draft.foo.val = false; // error TS2540: Cannot assign to 'val' because it is a constant or a read-only property.
});

Also, my IDE completely fails to give me suggestions for draft. Dunno why that is.

[ghost]

Yes but this is only because you have a readonly within a readonly. That's very unlikely to happen. Readonly is only used to make something readonly that isn't. In practice you'd declare the individual properties to be readonly. Draft doesn't protect against that however, in that case all readonly properties become readwrite in the draft - that isn't ideal either but it's still a greater level of type safety than before.

[ghost]

You do need to explicitly specify the type of draft with this change, either as a type param to the produce invocation or by declaring the type of the argument(s) of the function you pass to it. Otherwise it will default to any as per the signatures of produce. I'd like to remove that default but it wouldn't be backwards compatible.

Edit: "of the function" -> "of the argument(s) of the function"

[olavim]

Yes but this is only because you have a readonly within a readonly. That's very unlikely to happen.

Likeliness has nothing to do with it. Someone will have a reason to do it, and this change won't support it.

Plus it's easy to come up with a use case in Redux:

export interface AuthState extends Readonly<{
  user: {
    email: string;
    name: string;
  }
}> {}

export interface RouterState extends Readonly<{
  pathname: string;
}> {}

interface RootState extends Readonly<{
  auth: AuthState;
  router: RouterState;
}> {}

Now, there might be a reducer operating on RootState and a reducer operating on AuthState. In both cases we want the state being operated on to be readonly. The problem is, if the reducer operating on RootState tries to produce a mutable version of it, it will wrongly receive a mutable RootState with readonly AuthState and RouterState.

This was also the reason (one reason anyway) why my PR wasn't accepted.

[ghost]

You're right. Pick (and therefore Draft, as it's essentially the same) preserve the readonliness of properties.

This will never be possible. Typescript 2.8 is (I believe) introducing the ability to add/remove readonly/optional "flags" of a property however that also breaks interfaces where there are properties that must STAY readonly.

Maybe there's another way but I'm spent thinking about this for now.

[olavim]

My proposal is

recipe: (this: S, draftState: S | any) => void

Although it doesn't strongly enforce types, it removes the read-onlyness while IDEs can still give suggestions. Until we can recursively remove read-only flags from interfaces, I think this isn't so bad.

[mweststrate]

@tilastokeskus isn't it simpler to define your types in two stages?

// use this type for the producers
export interface AuthState {
  // whatever
}

// use this type everywhere outside
export type AuthStateRO = Readonly<AuthState> 

[mweststrate]

Shouldn't this be Draft<S> as well? (it's the same thing)

[ghost]

Ah no because that would allow modification of the original state. With it being Readonly<S> the compiler complains if this is modified in the recipe.

interface State { foo: string }
const first = { foo: "bar" };
const second = produce(state, draft => {
  this.foo = "can't do this, compiler error because 'this.foo' is readonly";
  draft.foo = "this works fine though";
});
second.foo = "this also produces a compiler error";
const third = produce(second, draft => {
  draft.foo = "but this does work, woo!";
});

The only time I can see this patch posing a problem is if someone defines their interfaces as completely readonly from the start. TS 2.8 is adding the ability to add/remove the readonly modifier when creating a new interface that references the type of a property of another interface (as in Draft/Pick/Readonly etc) but even then it wouldn't be possible to obtain the original readonly status.

As you said in your comment above the sensible thing to do is to define the interfaces in two steps. That also allows some properties to be readonly permanently such as:

interface State {
  readonly appName: string;
  someValue: number;
}

const first: State = { appName: "My App", someValue: 0 };
const second = produce(first, draft => {
  draft.appName = "compiler error here";
  draft.someValue++; // fine
});

If there's a chance there are people already using immer who have made everything readonly from the start this would be a breaking change.

[ghost]

Sorry @mweststrate when you said "they're the same" - they are literally the same object! My understanding was that this is the original state. So you could do:

const original = { foo: 123, diff: 0 };
const next = produce(original, draft => {
  draft.foo += someValue;
  draft.diff = this.foo - draft.foo;
});

But that's not the case. Though perhaps it should be?

Edit: Because you might not always have a reference to the original in a producer function created via produce(func) but might need it. This would be another breaking change but you don't need me to say that!

[olavim]

@mweststrate

isn't it simpler to define your types in two stages?

I would rather not make special types if immer is the only reason to do so. I'd much rather just say

produce(original, (draft: AuthState | any) => { ... });

and wait for typescript 2.8 in the hopes of being able to remove readonly properties recursively.

[mweststrate]

@ogwh I cherrypicked all the changes except the ones around readonly state. Would you mind checking whether that could be implemented accurately and recursively in TS 2.8 and updating this PR?

Thanks!

[mweststrate]

closing as most of the changes has been cherrypicked. @ogwh / @olavim if any of you is interested in solving the read only problem feel free to submit an additional PR. It can now be properly expressed I think since TS 2.8 has been released :)

[Zjaaspoer]

Solving the readonly typings would really be great. In one blow that would make this library the de-facto immutability library for typescript, as you will have a simple and concise, yet typesafe way of mutating your immutable objects.