Immer: Use createDraft() inside produce() for better type-safety

[marcello3d]

🚀 Feature Proposal

The ability to use createDraft (or similar) inside produce to import immutable structures into a draft.

Motivation

Currently there is no way to assign an immutable structure to a draft property. Further, if a ReadonlyArray is involved, type errors prevent this.

We're using this to copy an existing immutable structures into draft instances during a produce (e.g. from other object trees or initial values stored in global consts).

Minimal example:

type Foo = Readonly<{
    a: number;
}>;
type Model = Readonly<{
    foo: Foo;
}>;

const immutableFoo: Foo = { a: 1 };
immutableFoo.a = 2; // type error

const model: Model = { foo: { a: 0 } };
const model2 = produce(model, (draft) => {
    draft.foo = immutableFoo; // no type error, but should!
    draft.foo.a = 3; // unexpectedly mutates immutableFoo! 😧
});
console.log(immutableFoo); // mutated!
console.log(model2);

With a ReadonlyArray we get expected type errors:

type Foo = ReadonlyArray<number>;
type Model = Readonly<{
    foo: Foo;
}>;

const immutableFoo: Foo = [0];
immutableFoo.push(1); // type error

const model: Model = { foo: [] };
const model2 = produce(model, (draft) => {
    draft.foo = immutableFoo; // type error
    draft.foo.push(3); // unexpectedly mutates immutableFoo 😭
});
console.log(immutableFoo);
console.log(model2);

Example

It is almost solved by using the createDraft feature:

type Foo = ReadonlyArray<number>;
type Model = Readonly<{
    foo: Foo;
}>;

const immutableFoo: Foo = [0];
const model: Model = { foo: [] };
const model2 = produce(model, (draft) => {
    draft.foo = createDraft(immutableFoo); // no more type error! 😀
    draft.foo.push(3);
});
console.log(immutableFoo); // not mutated! 😀😀
console.log(model2); // has the new value 😀😀😀
console.log(isDraft(model2.foo)); // still a draft 😞

I could either see createDraft "just working" or a new similar function (maybe asDraft) that provides this behavior (if the behavior doesn't make sense for createDraft).

[aleclarson]

I approve of this proposal. I'll see if I can find time to implement it soon. ™️

It shouldn't be too difficult. Just need to detect when createDraft is called inside a producer, and then add the draft to the producer's drafts array, I think.

[mweststrate]

Please don't use createDraft for two different things. createDraft is a more lowlevel api to create a fresh proxy root, and an alternative for the recipe based signature of produce. Mixing the two concepts and giving createDraft two meanings significantly complicates the cognitive burden of these API's.

Beyond that, I consider it a bad practice to complicate a runtime, just to make TS happier. It is trading static errors for potential runtime bugs, additional complexity and additional bloat. Where the first has clear work arounds (e.g. as any) that can easily be proven to be safe, the solution complicates future maintainenance, can cause confusion and cause all the problems that are much less tangible than a type cast.

If you want to fix this in an API, I recommend to do something along the lines of cast in mobx-state-tree, provide a function in the run time that does nothing, but helps TS with the type inference. Something along these lines:

function asDraft<T>(thing: T): Draft<T> {
  return thing
}

This fixes a problem that only exists on the type level, almost purely in the type system, and the runtime adaption is risk free.

Btw, I expect that some of the other open issues on TS could be solved by also providing the inverse function, that is, when you get a draft but need to pass the immutable version somewhere:

function unDraft<T>(thing: T): T extends Draft<infer U> ? U : T {
  return thing
}

[aleclarson]

@mweststrate It's not only a TypeScript issue. They want to use an existing object (marked as Readonly in TypeScript) as the "seed" for a new object by drafting it first with createDraft (specifically while inside a producer). What are your thoughts on that?

[mweststrate]

That is already possible, but I think I don't understand the question then,
why is produce in the example? Either produce seeds, or createDraft. But it
is unclear to me what it conceptually means if both seed, except for fixing
a TS issue?

Op za 29 jun. 2019 14:43 schreef Alec Larson [e-mail]:

@mweststrate https://github.com/mweststrate It's not only a TypeScript
issue. They want to use an existing object (marked as Readonly in
TypeScript) as the "seed" for a new object by drafting it first with
createDraft. What are you thoughts on that?

—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
https://github.com/immerjs/immer/issues/393?email_source=notifications&email_token=AAN4NBH3DDR7S4CPHZCC7OLP45KGXA5CNFSM4H3V4UHKYY3PNVWWK3TUL52HS4DFVREXG43VMVBW63LNMVXHJKTDN5WW2ZLOORPWSZGODY3YEDI#issuecomment-506954253,
or mute the thread
https://github.com/notifications/unsubscribe-auth/AAN4NBGMRJJZCQQLP6IFSR3P45KGXANCNFSM4H3V4UHA
.

[marcello3d]

I’m totally with you about not complicating the library for TypeScript. We wrote an asDraft function like the one you describe but started getting bugs because it doesn’t copy or create a proxy and the original object can be mutated. (We've since renamed it to UNSAFE_asDraft 🙃)

Here is a non-TypeScript example of the problem (tested on 3.1.3):

const ZERO = { a: 0 };
const result = produce({}, (draft) => {
    draft.foo = ZERO;
    draft.foo.a++; // unexpectedly mutates ZERO :(
});

The same thing happens with createDraft:

const ZERO = { a: 0 };
const draft = createDraft({});
draft.foo = ZERO;
draft.foo.a++;

Looking at this again, what if Immer detected setting an object on a property and converted it to a proxy? (Or on get.) Then it would “just work” without the need to call createDraft.

[aleclarson]

Looking at this again, what if Immer detected setting an object on a property and converted it to a proxy? (Or on get.) Then it would “just work” without the need to call createDraft.

That might be the way to go. It would probably be "on get" to avoid unnecessary drafts.

Though, I don't mind the explicitness of createDraft. It's more of an escape hatch like that. I guess it depends on how often users are in this scenario. On the other hand, using createDraft within a producer without balancing it with a finishDraft call could easily confuse people.

[marcello3d]

Agreed. I think the on get solution has better ergonomics. Makes this issue look more like a bug than feature. ;)

[benmosher]

It looks like the code is explicitly avoiding creating drafts from assigned values during the get hook.

If it instead checked isDraft and created a proxy if it was not, I think that solve the primary runtime issue. Then the type signatures just need to be adjusted to allow setting immutable types in mutating methods and property assignments and I think that closes the loop.

That said, I think it will still have the same issues in the es5 implementation where there are no hooks for fields that didn't exist at when the produce started, since the hooks are created by iterating the base object's property descriptors.

[marcello3d]

Unfortunately I don’t think it’s possible to have different types for setters and getters in typescript: https://github.com/Microsoft/TypeScript/issues/2521

Hence an asDraft casting method would still be needed for TypeScript. Sounds like it might also be needed for es5?

[aleclarson]

As @benmosher pointed out, the "create draft on access of newly added property" proposal is impossible in ES5. So we'll have to wait for immer-lite which will drop ES5 support.

The best workaround is to make an asDraft function that uses clone-deep (or similar) and returns a mutable object, like this:

import cloneDeep = require('clone-deep')
import { Draft } from 'immer'

export function asDraft<T extends object>(obj: T): Draft<T> {
  return cloneDeep(obj as any)
}

Since the original proposal was denied, I'm closing this.

[marcello3d]

Bummer. Guess we’ll have to fork. Any timeline on immer lite?

[aleclarson]

Not that I'm aware of. Anyone is welcome to open a PR to kick things off. 😊

[mweststrate]

Tip: patch-package is sometimes even less work than forking, to have a
local modification to the library

On Tue, Jul 2, 2019 at 1:19 PM Alec Larson [e-mail] wrote:

Not that I'm aware of. Anyone is welcome to open a PR to kick things off.
😊

—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
https://github.com/immerjs/immer/issues/393?email_source=notifications&email_token=AAN4NBHEIKWM3GRSCV4VLYDP5M2UPA5CNFSM4H3V4UHKYY3PNVWWK3TUL52HS4DFVREXG43VMVBW63LNMVXHJKTDN5WW2ZLOORPWSZGODZA52NA#issuecomment-507632948,
or mute the thread
https://github.com/notifications/unsubscribe-auth/AAN4NBEWJ2XKJJLU4IX3GYTP5M2UPANCNFSM4H3V4UHA
.