Design pattern: testable models

[sisp]

I've been thinking how to design model classes that are easily testable, especially unit-testable. In my experience, models easily become coupled because of parent-child relationships, so testing a model in isolation is not straightforward. For instance, I have a model with a computed property that depends on some other model instance in the tree, e.g. a parent node.

@model('myApp/Child')
class Child extends Model({ /* ... */ }) {
  @computed
  public get value(): number {
    const parent = findParent<Parent>(this, n => n instanceof Parent)
    if (!parent) {
      throw new Error('Child must have a parent of type Parent')
    }
    return parent.value * 2 // or whatever
  }
}

@model('myApp/Parent')
class Parent extends Model({
  value: prop<number>()
}) {}

Now, if I want to unit-test Child::value, I would need to mock findParent (e.g. using Jest), but since findParent is not dependency-injected, it's not that easy, is it?

@xaviergonz How would you do it? Would it help to retrieve the parent in a separate method (or getter)? E.g.:

@model('myApp/Child')
class Child extends Model({ /* ... */ }) {
  @computed
  public get value(): number {
    const parent = this.parent
    if (!parent) {
      throw new Error('Child must have a parent of type Parent')
    }
    return parent.value * 2 // or whatever
  }

  public get parent(): Parent | undefined {
    return findParent<Parent>(this, n => n instanceof Parent)
  }
}

Perhaps then Child::parent could be mocked more easily?

[xaviergonz]

To be honest, I think that a child relying on a parent to do something is usually an anti pattern (it would be like a react component relying on a parent component to be there to work), so what I'd do is move that value function to the parent itself with the child as parameter, that way always keeping a parent->child dependency, e.g.

@model('myApp/Child')
class Child extends Model({ /* ... */ }) {
}

@model('myApp/Parent')
class Parent extends Model({
  value: prop<number>()
}) {
  getChildWhatever(child: Child) { return child.whatever * this.value }
}

and in case you want to keep it computed (for memo reasons or whatever) then

@model('myApp/Child')
class Child extends Model({ /* ... */ }) {
}

@model('myApp/Parent')
class Parent extends Model({
  value: prop<number>()
}) {
  private memoMap = new WeakMap<Child, IComputedValue<number> /* don't remember the exact type */>
  getChildWhatever(child: Child) {
    let c = memoMap.get(child)
    if (!c) {
      c = computed(() => { child.whatever * this.value })
      memoMap.set(child, c)
    }
    return c.get()
  }
}

The memo map thing could actually be wrapped into a nice abstraction so it doesn't need to be repeated often.

that being said, I think making parent a getter like you got on your second example is a good idea (performance tip, you can make that get parent computed and it might become memoized when observed)

But all of this is just my opinion :)

[sisp]

I agree that a child->parent dependency is bad and I'd like to avoid it, too.

In my case, there is a child->parent dependency, though, because the property of a child depends on information from the parent. My previous example was too simple (I tried to keep it short) and, thus, didn't outline my problem properly. Since you've mentioned React and I've also been trying to map the way React works to mobx-keystone (same problem with MST), let me give a simple React example that I'd like to translate to mobx-keystone (conceptually):

function Parent() {
  return <Child value={10} />
}

function Child({ value }: { value: number }) {
  return <span>{value * 2}</span>
}

In this contrived example, Child receives a value: number and multiplies it by 2. Any parent component which uses Child can pass some number to the Child's value prop and Child will do its thing. This way, Child and Parent are nicely decoupled.

Now, I'd like to have a Child class which has a computed property value which (in this contrived example) multiplies a value by 2, but this value is provided by Parent (e.g. through a property called value or a callback that the parent passes to Child - how would the latter actually work in practice?). So in fact, Child depends on Parent.

Perhaps a slightly better example using mobx-keystone is this:

@model('myApp/Child')
class Child extends Model({ /* ... */ }) {
  @computed
  public get value(): number {
    return /* parent should tell me the value */ * 2
  }
}

@model('myApp/Parent')
class Parent extends Model({
  child: prop<Child>()
}) {}

Do you see what I mean?

[sisp]

The way I'm dealing with this problem right now is to implement an abstract class (e.g. ChildBase) which contains (1) shared logic among all subclasses and (2) abstract methods for things that are tightly coupled to some parent. For each usage of ChildBase with a different parent dependency, I extend this abstract base class and implement the missing methods that require tight coupling to a specific parent (using, e.g., findParent). It feels a bit wrong (with the same arguments you provided against this relationship direction) but works and I don't know of a good alternative. But let's see, perhaps you do. ;-)

[xaviergonz]

I was thinking, besides the currently added onChildAttachedTo, would it also make sense to add a second optional parameter to new Model({}, {here}) that would be accessible through onInit(here) in order to inject other stuff besides props? For example, that second parameter could be used to inject a getter when using new

[xaviergonz]

hmm, the problem now that I think about it is that it wouldn't be serialized and would break serialization through getSnapshot / fromSnapshot, so nevermind, guess it is better to stick to onChildAttachedTo if the property is dynamic / onInit() if it is not

[xaviergonz]

@sisp I think the just released contexts solves this issue better than the other alternatives :) https://mobx-keystone.netlify.com/contexts

It kind of mimics react context providers

[sisp]

This looks like a great idea! Thanks a lot for coming up with and implementing this feature! I think models will be much more reusable this way and obviously much better testable. :-) Looking forward to trying it out!

[xaviergonz]

Hope you like it :) Feel free to close the issue this solves it.

[xaviergonz]

Closing for now, feel free to reopen it!