createViewModel doesn't work correctly with setters for computed values

[alex3683]

In my model I have several computed getters and accordingly setters, to derive the plain values from.

Simple example:

import { computed, observable } from 'mobx';

export default class Car {

  @observable public colorCode = 'red';

  @computed
  public get colorInstance(): any {
    return { colorCode: this.colorCode };
  }

  public set colorInstance(colorInstance: any) {
    this.colorCode = colorInstance.colorCode;
  }
}

If I now create a view model from it using createViewModel, changes applied to the colorCode property are directly visible in the view. If I instead apply changes by using the colorInstance setter, it seems to have no effect. See here for the full, running example: https://codesandbox.io/s/reverent-dew-pb6mh

When looking at the generated getters and setters in the code, the observation makes sense: computed values are always read from localComputedValues, while changes are always written to localValues. Hence, in my example colorInstance is written to the localValues colorInstance but read from localComputedValues, where it will never be up-to-date.

Is this a bug or is it intended to work like that and I made a mistake somewhere?

If you need further details, feel free to ask.

[urugator]

I think it's a bug, would the following help?

// https://github.com/mobxjs/mobx-utils/blob/1576286745c6229b34e1cf67353c3ccce1c6a843/src/create-view-model.ts#L56-L59
if (isComputedProp(model, key)) {  
  const modelComputedBox = _getAdministration(model, key) // Fixme: there is no clear api to get the derivation
  
  const get = modelComputedBox.derivation.bind(this);
  const set = modelComputedBox.setter 
     ? modelComputedBox.setter.bind(this) 
     : undefined;

  const localComputedBox = computed(get, { set });
      
  this.localComputedValues.set(key, localComputedBox);
}

// https://github.com/mobxjs/mobx-utils/blob/1576286745c6229b34e1cf67353c3ccce1c6a843/src/create-view-model.ts#L72-L78
set: action((value: any) => {
  if (isComputedProp(model, key)) {
     this.localComputedValues.get(key).set(value);
  } else if (value !== this.model[key as keyof T]) {
    this.localValues.set(key, value)
  } else {
    this.localValues.delete(key)
  }
})

[alex3683]

@urugator
Jepp, that seems to fix the issue 👍🏼

[mweststrate]

@alex3683 interested in making a PR with this fix, including a test?

[alex3683]

@mweststrate Theoretically yes, but I guess I won't have the time to do this. I can try to do it, but I cannot promise any release date. Boss, wife and kids, all want me to do something else instead ;-)

[M-ZubairAhmed]

@alex3683 would you mind if i contribute to this one?

[urugator]

@mweststrate btw note that the local computed ignores original computed's options (like equal), could also lead to some issues...

[alex3683]

@M-ZubairAhmed Go ahead. I don't want to block this.

[mweststrate]

@urugator, yeah there are an unlimited amount of potential issues with prototypes, getters, enumerability and view models.... This createViewModel is really a 'best default', and if it doesn't suffice, one should build their own view model by hand. Not entirely sure where to draw the line :).

So another solution would be to completely forbid the usage of getters / setters in view models....

[alex3683]

So another solution would be to completely forbid the usage of getters / setters in view models....

Please don't. I use them a lot to transform string references into instances of other models and back. I think this is one of the greatest mobx features (including all the others ;-))

[ahoisl]

I fixed this issue independently in one of my projects, then saw this issue and now did go forward and merged my implementation with the proposed fix in this thread. See and please review the open PR.

Additionally, if one is interested in a "deep" view model, one that is also able to transparently work with nested objects and arrays, then I could go forward and try to merge my private implementation into the mobx-utils, too...

[NaridaL]

Fixed by #286