Custom Class support in state tree

[wonderwhy-er]

• Feature request: ______
  • I am willing to implement this myself

Tried my hand on it quickly and you can find it here
wonderwhy-er@0fcc078

Issue is that it does not throw on non Date objects now

Also for classes that use private techniques this wont work correctly, probably needs second symbol that allows providing custom proxy that is aware of private state.

Not sure how to proceed so far.

[jbojcic1]

Any news on this?

[pmilic021]

The initiative is great and it is really needed! Immer has (IMO) the cleanest way of writing object updates, but without this, its kind of useless for TS.

With that said, what is blocking you? Perhaps some of us could help?

[wonderwhy-er]

From my side. I refactored part of my private project on to immer with that fork I linked to and made part of the state work like this.

There I encountered two other problems one of which is related.
So if I have a
class Task {
private state:boolean = false;
public complete() {
this.status = true;
}
}

This will not work with Immer as in 'complete' method I will be editing this and not something that came from draft. Editing this there is weird anyways as it kinda is not immutable while we probably want to freeze the object anyways to not allow accident mutations.

And getting draft equivalent of this for mutation is tricky. I ended up having pretty weird callbacks/getters chain to the top of the state to open produce transactions, get draft and from it traverse back to 'this' equivalent and make mutations on it.

This all made me think of what is good pattern for mutating methods on classes here + that there should be some helper.

It seems to me it is wroth collecting some use cases collection to better understand the problem so I chose to continue trying to refactor my mutable class based state to immer and see what patterns I will end up with.

On a side note other problem I encountered was that Immer works with trees only. And I ma in bad need for multi parent branches.

Aka if we speak about tasks then
I have all tasks list
completed tasks list
then edit task in whole list should also change task in completed list

this is different problem altogether, and I am deep in trying to write a prototype that would allow multi parent nodes and thus this use case

[wonderwhy-er]

1. In short I have doubts about real use case for classes, if we only use immutable ones then it is ok, but trying to preserve mutation interface on nested classes level looks pretty weird at the moment
2. I also have no use for custom Proxy implementation and just use Immer proxy on classes and it works fine
3. I see two approaches to marking classes as usable by immer. Ether symbol based I did in that fork or allow registering classes in immer along with shallow copy and proxy implementations.

I guess to get quick results I can make PR that does 3rd thing and does not address 1 and 2 and we can address them later

Would be interesting to hear more feedback on those things to see where it is better to go

[mweststrate]

Working classes won't really fit the working model of immer for a few reasons

1. To be able to clone the class needs to have an argumentless constructor (or a special clone symbol as suggested)
2. All the fields in itself need to be cloneable again (in case they are modified during the produce), so no streams, socket connection, functions etc (or they are not allowed to be modified during produce)
3. All methods would need to be applied to the proxy, not the original this. This is doable with proxies, unless the functions are bound
4. The classes cannot have circular references / must be direct acyclic graphs
5. It will result in a confusing mix, the immer guarantees that state is an immutable tree never fully works, if a class has a reference to something that is intrinsically mutable, like a DOM element or so.

In other words, there are a lot of limitations, really hard to comprehend edge cases when working with classes. Unless classes are basically restrained to be just records or structs. But that kinda defeats the whole purpose as in that case an interface would have sufficed anyways.

In other words, using immer should follow from the more fundamental design decision that you want your state to be an immutable, single value tree. If that architectural decision is made, using classes is a quite unlogical / confusing implementation of the design, and should be avoided anyway. Imho.

In contrast, when your state is designed to be mutable, using immer (on the mutable parts) is just confusing as well, as it does seem to guarantee things it doesn't really guarantee in such cases.

In other words, I think using immer together with classes leads to confusing scenarios at best, and becomes incomprehensible very quickly at worst.

@pmilic021 I don't get entirely what you mean when saying immer is unusable with TS? Immer has first class support for TS and strongly typed state tree, which is one of it's main benefits.

[wonderwhy-er]

@mweststrate
You are mostly correct, especially in case of this complexifying design too much.
Playing with the implementation I did make some things from your list work, 1. 2. are not a problem depending on how you solve it. Reminds of how to react allows impure components hidden behind pure interfaces when you are wrapping something like google maps.

Now I did encounter 3. and solved it in a way I do not like.
Starting here I started to doubt and think like you, aka classes and immutable tree are not a good match.

Now it is 4. that I dug in to. Even if I drop class usage for the state I am still interested in immutable multi-parent graphs. I have a quick implementation that allows tracking multiple parents but it fails some tests for now and I do not have time for last month for side programming sadly but am planning to continue fixing issues with it. But even when done there could be large performance issues with that.
All in all, even if it is gonna work there are large doubts it is gonna perform that well. Still, want to see how bad result will be :D

Have seen any implementations of immutable graph structures anywhere? ImmutableJS does not do that, neither I found any good examples in other languages.

[mweststrate]

Updates the docs a bit to explain why Immer does not support classes

[ianstormtaylor]

In other words, there are a lot of limitations, really hard to comprehend edge cases when working with classes. Unless classes are basically restrained to be just records or structs. But that kinda defeats the whole purpose as in that case an interface would have sufficed anyways.

Could you explain the alternative you're suggesting here? Is there no way to have immer deal with immutable records/structs that also happen to have their own prototype with helpful methods?

[mweststrate]

Correct, very simply put, the most trivial pattern like this wouldn't work:

class X {
  y

  constructor(num) {
     this.y = num
  }

  add(num) {
    this.y += num
  }
}

class Z {
  x: new X(2)
}

produce(new Z(), draft => {
  draft.x.add(2)
})

After this, immer should have cloned both an instance of X and an instance of Z, but it is very hard to generalize the concept of cloning for classes, because immer cannot know how the constructor for X should have been called.

[ianstormtaylor]

@mweststrate thanks for much for the quick response! That makes sense to me I think. I guess, my question then is, for someone coming from Immutable.js, how can we handle the Record use case with immer? Is there no way?

I see lots of benefits in immer, and I'm just trying to figure out how to maintain some of the benefits of Record at the same time. Having the related methods be directly on the prototype makes for a really nice developer experience I think.

[mweststrate]

@ianstormtaylor Record doesn't really support having a prototype as well doesn't it? As in; preserving it will being cloned etc (sure you can throw functions in the mix, but that works with immer as well)

[ianstormtaylor]

@mweststrate I'm not sure actually. It sounds like I may not be understanding how immer is able to use functions, so maybe this is already solved?

Given a simple Record use case like:

class Point extends Record({
  key: null,
  offset: null,
  path: null,
}) {
  get isSet() {
    return this.key != null && this.path != null && this.offset != null
  }

  isInNode(node) {
    if (!this.isSet) return false
    if (node.object === 'text' && node.key === this.key) return true
    if (node.hasNode(this.key)) return true
    return false
  }

  setOffset(offset) {
    return this.set('offset', offset)
  }
}

This example is pulled from my existing Immutable.js Record usage, and shows a few different things I'm curious about, but can't quite figure out what the recommended immer way is:

1. An instance getter.
2. An instance method.
3. Returning a new instance from an instance method.

Is this making any sense at all? I might be off the rails 😄

For context, I'm researching this for Slate on account of this discussion.

[mweststrate]

This is what ImmutableJS does, it preserves the prototype for object types:

https://github.com/facebook/immutable-js/blob/f71ccd5db2016e0dbed72838ab81b931b5e90e9e/src/Record.js#L216

We could do the same for immer I guess, but there are some limitations: It has to be clear that constructors should take no args, and it has to be clear on which occassions we should stop recursion (e.g, doing the same trick for Buffer would probably blow up).

[ianstormtaylor]

@mweststrate interesting!

At least from my own perspective, having the ability to have Record-like instances would be a big pro, since it makes defining and using more complex internal concepts easy. I think Record is one of the most-liked things about Immutable.js and would make switching easier. But obviously I don't know immer so I can't know how easy/hard it would be, and what the limitations are.

[aleclarson]

Anyone that cares about class support in Immer, please chime in over here: #202 (comment)