Immer: Should produce always freeze its return value?

[aleclarson]

Which guarantee is more important?

• produce always freezes objects it returns
• produce is a no-op when the producer is a no-op

Have a look at the following behavior: (https://codesandbox.io/s/1o8x91w5l4)

let obj: object

// 1. do nothing with draft of a mutable object
Object.isFrozen(
  produce({}, () => undefined)
) // => false

// 2. return the draft of a mutable object
Object.isFrozen(
  produce({}, draft => draft)
) // => false

// 3. replace null with a mutable object
obj = {}
Object.isFrozen(
  produce(null, () => obj)
) // => false

// 4. replace a mutable object with a mutable object
obj = {}
Object.isFrozen(
  produce({}, () => obj)
) // => true

// 5. return the original, mutable object
obj = {}
Object.isFrozen(
  produce(obj, () => obj)
) // => true

// 6. your average producer
Object.isFrozen(
  produce({}, draft => {
    draft.a = 1
  })
) // => true

Which of these cases have unexpected behavior?

Case 1 seems wrong if you believe produce should always freeze objects before returning them.

Case 2, 3 seem wrong. Why shouldn't the return value be frozen here?

Case 4, 5, 6 seem right. The return value is frozen when it's an object.

@mweststrate WDYT?

[mweststrate]

Some thoughts

The general idea is that freezing is a favor we offer, but not a guarantee

The second idea is that we don't do a full freeze, as that is super expensive. We just freeze when we are creating the new objects. In theory, if all state updates are done with producers, that means that all state will be frozen, but that state that is not processed, is also not frozen (there are actually some redux middlewares that violate the normal constraints and attach meta data to the tree :-().

So the reason for freezing is touching. We don't objects we didn't create, as in that case produce would basically have a side effect, which I think would be ugly; we promise not to modify the base state, but actually we do modify it, namely we freeze it.

In other words, I think all results are correct :-). Other semantics would lead to immer to have side effects, by either touching the base state or objects that are obtained from the closure (like example 3), rather than being created in immer itself. (One could argue that draft => ({}) should freeze, but it is impossible to detect whether that returned object is a new one, or one coming from a closure)

[aleclarson]

I agree about keeping produce side-effect free. :D

Why does case 3 not behave identically to case 4?

[aleclarson]

Here's a sandbox with all the cases: https://codesandbox.io/s/1o8x91w5l4

[mweststrate]

Good question, checking a second time, I think both 4 and 5 are actually bugs and should not produce a frozen state, as the freeze objects not created by immer.

On the other hand, one could argue that freezing at least the root always will help people to find bugs, even though that effectively creates a side effect. But that is something that can easily be done in user land, for example by calling deepFreeze after each produce

[aleclarson]

Yeah, those are the bugs we're looking for.

I agree that Immer's current behavior is the "right way".

[aleclarson]

Should be added to the docs:

Whenever an object is copied by produce, the copy is deep-freezed. This is the only time that Immer will freeze anything.

I found more test cases.

import {produce} from "immer"

// 7. add a new property whose value is a mutable object
Object.isFrozen(
    produce({}, draft => {
        draft.a = {b: 1}
    }).a
) // => true

// 8. add a new property with two levels of nested objects
Object.isFrozen(
    produce({}, draft => {
        draft.a = {b: {c: 1}}
    }).a.b
) // => true

// 9. return a mutable object that contains another object
Object.isFrozen(
    produce({}, draft => {
        return {a: {b: 1}}
    }).a
) // => true

// 10. return a mutable object with three levels of nested objects
Object.isFrozen(
    produce({}, draft => {
        return {a: {b: {c: 1}}}
    }).a.b
) // => true

The sandbox is also updated.

[aleclarson]

It seems like cases 7 & 8 should never return true.

edit: Same goes for 9 & 10 if we decide to _not_ always freeze the return value.

[aleclarson]

Another one.

// 11. return a non-root draft
console.log(
  Object.isFrozen(
    produce({ a: { b: 1 } }, draft => {
      return draft.a
    })
  )
) // => false

I'm going to add test cases for all of these.

edit: a60a6fa08f0015ca66d8e3161a8197982fdba417