Mobx: ObservableArray methods should return another ObservableArray instead of Array?

[andykog]

I think, when calling standard array methods, you should get another ObservableArray to avoid inconsistency:

class MyAnimalsStore() {
  @observable animals = [];
  @computed get animals() {
    return this.animals; // returns ObservableArray
  }
  @computed get squirrels() {
    return this.animals.filter(a => a.isSquirrel); // returns Array
  }
}

PS: This way idiomatic myObservableArray.slice() should be replaced with Array.from(myObservableArray)

[urugator]

Related #166

[mweststrate]

@andykog can you elaborate on why you want this? (I can imagine several different things). Just for consistentcy or do you actually need the array to be observable if returned from a computed?

[andykog]

@mweststrate, this is just the matter of consistency. I don't actually need it to be observable. If constructing new ObservableArray is not acceptable due to performance overhead, then I think returning instance of simple class like ObservableArraySubset with the same api (so it has toJS method, doesn't passes Array.isArray() check...) would be much better then returning native Array.

[andykog]

Example:
Assume, that VendorReactComponent throws error if fed with ObservableArray:

class MyStore {
  @observable $items = [];

  @computed get items() {
     return this.$items.filter(x => x.legacy !== true);
   }
}

class MyComponent extends Component {
  render() { return <VendorReactComponent items={myStore.items} />; }
}

If somebody removes .filter(x => x.legacy !== true) in the store, MyComponent will unexpectedly break.

[urugator]

What observable(object) does if object is already observable? Does it simply return object? Maybe one could get some "safety" by doing it like this:

@computed get items() {  
  // return observable(this.$items.filter(x => x.legacy !== true));
  return observable(this.$items); // ensure it's observable   
}

The idea is: Do I see term "observable"?
Yes -> it's 100% observable.
No -> I should either add term observable to make things clear or leave it alone if it's not suppose to be observable.
Not exactly bullet proof, but it might help preventing some errors :)

ObservableArraySubset still introduces some (negligible?) overhead, because one have to call slice() in order to get plain array, which currently isn't neccessary.

I would like to know what would be the intended behavior in the ideal situation, where ObservableArray is actually JS Array, for example implemented via proxies:
Has observable (proxified) array extended API?
Does filter also returns an (proxified) array with extended API? Is returned array observable and observable how?

[mweststrate]

@urugator yes if it is already observable you get literally the same thing back. Filter does return an old fashioned array. It is not observable itself for the simple reason that it shouldn't change anymore in the future; it is a one time snapshot (instead, the computed should re-run, giving you a freshly filtered object when needed).

(Performance wise it might be interesting to have filter produce an dereived observable array that is synced smartly, see #166, but so far there seems to be hardly a need for this)

[andykog]

Just to make sure, that my proposition is understood right: its not only about filterbut about concistency when working with array data in mobx.
I suggest that any method except toJS returns MobxArray (in the manner of Immutable.js List)

As an option, that could be implemented in a single class, pseudocode:

class MobxArray {
  $values = [];
  isObservable = false;

  constructor(initialValues = [], observalble = false) {
    if (isObservableArray(initialValues)) initialValues = initialValues.toJS();
    this.$values = initialValues;
    if (observalble) this.makeObservable();
  }

  toJS() {
    if (this.isObservable) this.$mobx.atom.reportObserved();
    return this.$values
  }
  slice() {
    if (this.isObservable) this.$mobx.atom.reportObserved();
    return new MobxArray(this.$values.slice(arguments));
  }
  filter() {
    if (this.isObservable) this.$mobx.atom.reportObserved();
    return new MobxArray(this.$values.filter(arguments));
  }
  concat(arr) {
    if (isObservableArray(arr)) arr = arr.toJS();
    if (this.isObservable) this.$mobx.atom.reportObserved();
    return new MobxArray(this.$values.concat(arr));
  }
  get length() {
    if (this.isObservable) this.$mobx.atom.reportObserved();
    return this.$values.length
  }

  makeObservable() {
    this.isObservable = true;
    this.$mobx = new ObservableArrayAdministration(this.$values);
  }
  observe() {
    if (this.observable == false) {
      this.makeObservable();
    }
    //...
  }
}

That may also address issues like #460

[urugator]

I just don't like the ambiguity. I would rather introduce MobxArray as a thin JS Array wrapper, having extended mobx array API, all methods return MobxArray instead of Array.
Then ObservableArray extends MobxArray, method signatures (return type) stays the same as in parent. Aside from that, I agree with proposal.

I would also introduce mobx.isMobxArray and mobx.isArray:

function isMobxArray(arg) {      
  return arg instanceof MobxArray; // don't actually use instanceof operator but some duck type check     
}
function isArray(arg) {
  return Array.isArray(arg) || isMobxArray(arg);
}

[mweststrate]

isArrayLike was introduced in some MobX 2.6 version, making it easy to recognize (observable) arrays. Besides that I think this issue is mainly a duplicate of #166. I see some points in @andykog's arguments, but I fear a third array type will only add confusion, and two types of arrays make the distinction 'state' and 'derived' (or non / temporarily state) more clear. Closing for now if you guys don't mind!