New tests (one currently failing): Non-immerable instances

[bennorth]

I recently got tripped up by Immer silently misbehaving when the top-level baseState was a plain object, but within it was a class instance. Had I read the manual more carefully, I would have been aware of the immerable requirement, but nonetheless it would have been helpful if Immer had thrown a noisy error. This PR adds two tests, both testing that an error is thrown if you try to use Immer with an instance of a non-immerable class. One of the tests fails, as no error is thrown for

const Pet = class { constructor(name) { this.name = name } }
const baseState = {pet: new Pet("Rover")}
produce(baseState, draft => { draft.pet.name = "Rex" })

Would it be possible to have Immer throw its [Immer] produce can only be called on things that are draftable... error in this case? I'm afraid I'm not familiar enough with the internals of Immer to propose a fix, but hopefully these tests will be useful.

Thanks!

[codesandbox-ci]

This pull request is automatically built and testable in CodeSandbox.

To see build info of the built libraries, click here or the icon next to each commit SHA.

Latest deployment of this branch, based on commit db40cd7:

Sandbox	Source
Immer sandbox	Configuration

[mweststrate]

The problem with this approach is that it will make it impossible to store arbitrary things in an immer tree, such as a Buffer, DomElement, Promise, etc etc.

[bennorth]

I see what you mean. As I say, I would not have been tripped up if I'd read the docs properly, but if there was a way to tell users of Immer that they're doing something unsupported, that would be helpful. As a very rough suggestion, maybe in the example above,

produce(baseState, draft => { draft.pet.name = "Rex" })

would throw an error, but

produce(baseState, draft => {
  immerAccessNonDraftableProperties(draft).pet.name = "Rex"
})

would be OK? Users can access or even mutate non-draftable properties, but only if they explicitly promise Immer that they know what they're doing.

The docs (the "Classes" page) say Immer does not support exotic objects such as DOM Nodes or Buffers, so what should happen if someone has something non-draftable in their tree? Currently, I think Immer lets you mutate your Buffer, and so the state is mutated, while keeping its identity.

I realise that the introduction section of the docs can't explain every nuance, but currently it says The interesting thing about Immer is that the baseState will be untouched, which isn't the case in the above example. Maybe an update to the "Performance" page would clarify things too; it currently says Immer will convert anything you read in a draft recursively into a draft as well, which I don't think is strictly always true.

(Would it be cleaner to close/reject this PR and move discussion to an issue, if you think there's a useful discussion to be had?)

[mweststrate]

I think your solution still requires people to read the docs to avoid anything unexpected. Indeed this case isn't highlighted upfront in the docs, but storing classes instances in the first place is quite uncommon when the goal is to work with immutable data structures, and most tools, like for example redux middlewares, will require special handling if you do that.

[bennorth]

OK, thanks for the discussion. The main point of my suggestion was that an unexpected error would be better than an unexpected silent mutation of part of the state. Then the user would know to go and read the documentation more carefully.

(For background: I am using Immer indirectly, via easy-peasy. Its quick-start documentation says that you define actions to "Update the state by directly mutating the state argument. Don't worry, under the hood we will convert the operation into an immutable update against your store by using Immer." I was then confused as to why my React component wasn't re-rendering after a change of state.)

[mweststrate]

Ok, rewrote it as issue proposal #686, let me know if it covers what you are pointing at.

[bennorth]

Thanks; your #686 captures the point well.