xaviergonz / mobx-keystone

A MobX powered state management solution based on data trees with first class support for Typescript, support for snapshots, patches and much more
https://mobx-keystone.js.org
MIT License
555 stars 25 forks source link

setterAction does not trigger computed #143

Closed swayf closed 4 years ago

swayf commented 4 years ago

I have strange example

import {Model, model, modelAction, prop} from 'mobx-keystone';
import {computed, autorun, action} from 'mobx';

@model('P')
export class P extends Model({
  y: prop(0, {setterAction: true}),
}) {
  @modelAction
  setY(n: number) {
    this.y = n;
  }

  @computed
  get test() {
    return this.y * 5;
  }
}

const t = new P({});
autorun(() => console.log(t.test));
autorun(() => console.log(t.y));

t.setY(5);

// t.y equal 5
// t.$.y equals 5
// t.test equals 25
// autorun logging gives 25 and 5

// but 
t.y = 10;

// t.y equal 10
// t.$.y has old value 5
// t.test has old value 25
// autorun logging gives nothing new

is it bug, or I use this feature in wrong way?

swayf commented 4 years ago

if we take as example setterAction.test.ts and add line

  p.y = 5
  expect(p.y).toBe(5)
  expect(p.$.y).toBe(5) // <-- new line

it works. but after adding computed value in the model

@model("P")
export class P extends Model({
  y: prop(0, { setterAction: true }),
}) {
  @modelAction
  setY(n: number) {
    this.y = n
  }

  // new computed getter
  @computed
  get computedValue() {
    return this.y * 10;
  }
}
  p.y = 5
  expect(p.y).toBe(5)
  expect(p.$.y).toBe(5) // <-- new line does not work any more
swayf commented 4 years ago

the problem place is here:

export function internalApplySet<O extends object>(
  this: O,
  fieldName: string | number,
  value: any
): void {
  if (isObservable(this)) {
    set(this, fieldName, value)
  } else {
    ;(this as any)[fieldName] = value
  }
}

set(this, fieldName, value) does not works right in this case as example, if I change it to

export function internalApplySet<O extends object>(
  this: O,
  fieldName: string | number,
  value: any
): void {
    if (isObservable(this)) {
        if (isObservable((this as any).$)) {
            set((this as any).$, fieldName, value)
        } else {
            set(this, fieldName, value)
        }
    }
    else {
        ;(this as any)[fieldName] = value
    }
}

the extended tests works, even with autorun and computed all old tests also. but I am not sure if it is right way to solve the problem =)

xaviergonz commented 4 years ago

Hey, thanks for the detailed bug report! I'll release v0.43.1 with a fix soon