angular.copy and angular.equals don't play well with MobX

[thorn0]

Do you want to request a feature or report a bug?

feature

What is the current behavior?

In theory, it should be easy to start working with MobX. It will be fun they said. Just replace plain objects and arrays with observables and the existing code shouldn't notice much difference. Just a couple of tweaks might be needed. Unfortunately, it's not the case with Angular 1.x apps. Watchers get broken when they are passed observable arrays.

If the current behavior is a bug, please provide the steps to reproduce and if possible a minimal demo of the problem via https://plnkr.co or similar (template: http://plnkr.co/edit/tpl:yBpEi4).

$watch ignores changes in observable arrays:
http://plnkr.co/edit/eNBaj5HBvvGdd6PAos8g?p=preview

$watch uses angular.copy and angular.equals internally, and it's these functions that don't work with MobX:

var observableArray = mobx.observable([1, 2, 3]);
var copy = angular.copy(observableArray);
console.log(angular.equals(observableArray, copy)); // true
observableArray.push(100);
console.log(angular.equals(observableArray, copy)); // true!
// `copy` is completely unusable:
copy[1]; // TypeError
copy.length; // TypeError

What is the expected behavior?

Would be great to have a way to teach angular.copy and angular.equals new object types.

What is the motivation / use case for changing the behavior?

I decided to play with MobX in my app to see whether it can make the state management simpler. As the first step, I was going to just replace my model/state objects with observables to see what changes are needed.

Which versions of Angular, and which browser / OS are affected by this issue? Did this work in previous versions of Angular? Please also test with the latest stable and snapshot (https://code.angularjs.org/snapshot/) versions.

Angular 1.6, 1.5

Other information (e.g. stacktraces, related issues, suggestions how to fix)

This issue may sound like #10096, but it's not about specifying an alternative copy and equals implementation on every call to $watch. A way to override this logic globally for all the watchers, directive bindings, etc. is needed.

[gkalpak]

This has nothing to do with MobX (in the sense that it is in no-way MobX-specific). So, the title is unnecessarily specific imo.

This is basically a general feature request about supporting equals and copy for custom/arbitrary objects (which in turn would enable such objects to be used in bindings/watchers).

That said, there have been some concerns about allowing developers to tweak methods that are so fundamental for the entire framework - essentially making it easier for people to shoot themselves in the foot.

TBH, I myself am not very keen on this feature.

[thorn0]

If we keep this issue MobX-specific, we might consider also other ways of solving it. E.g. finding why these functions don't work with observable arrays and fixing the reason. I guess it's #5085. Also it'd be interesting to discuss a deeper integration of MobX with watchers.

[bathos]

Dealing with the unpatchable equals/copy of $watch has become a lot more of a problem in ES6 I think. There’s no such thing as a generic ‘equals’ or ‘clone’ method in JS (not sure if there ever was, really), and the current implementation makes some curiously unsafe assumptions. For example, it doesn’t copy accessors (reasonable default) on the target object, but because it does copy the prototype chain, it effectively copies only accessors which are not own-properties, a very strange result! Commonly, accessors involve some form a private state at the instance level, e.g. symbol keyed shadow properties or a class-wide WeakMap registry keyed on instances. Both of these will break in the clone. One could say, ‘well, you can’t watch an object like that’ ... except I think such objects are more and more common, and few devs actually think about how Angular’s watches work.

While I don’t fully understand all the reservations expressed in the #10096 thread, I’m wondering if a solution of a simple contract would please everybody — e.g. $expressionValue(), which must return a serialization in the form of a primitive value.

The advantages, I think, would be:

• Pretty sure the code changes would be a relatively smaller touch(?)
• I could be mistaken here, but I think the current internal certainties about the comparisons which are valued dearly are (to the degree that matters, anyway?) maintained — ng could yell at you if this method returned a non-primitive value
• Devs wouldn’t need to learn new stuff about the inner mechanics of $watch (compare the custom equals fn / clone fn solution — dev must learn how these are used inside $watch, which is currently something you’d only really pick up from either reading the source or thinking about it really hard while staring out a window).
• I think this would address the spirit of @thorn0’s qualifier, "a way to override this logic globally", just inverted a bit. i.e., while it would not be some global app config thing, the implementor of object Foo could control how Foos get compared, in angular’s eyes, across the board
• I think it would help to encourage/enable inexpensive checks; trying to distill meaningful state to a primitive form naturally promotes looking for places to cut fat.

I feel like this idea is either really awesome or really dumb, but it’s 2:32 AM here and I could go either way.

Edit: Just saw that this same solution was proposed in #5085, but without the only-primitive-value constraint

Edit edit: Whoa ... it already exists ... though it’s a hack. cloneNode ;)

[gkalpak]

If we keep this issue MobX-specific, we might have also other ways of solving it. E.g. finding why these functions don't work with observable arrays and fixing the reason.

@thorn0, the reason this "doesn't work" with MobX is that mobx.observable([...]) returns an object that is not an array (in angular.isArray() terms) and is thus recognized as an object with no enumerable properties. Because of that, copying returns an empty (broken) instance of ObservableArray and comparing that with the original object via equals will always return true (no matter how you modify the latter), since it is essentially the same as equals({}, {}).

We might be able to make MobX ObservableArray work, by relaxing angular.isArray() to:

function isArray(arr) {
  return Array.isArray(arr) || (arr instanceof Array);
}

...which is a breaking change, but a reasonable one imo. But other MobX stuff or other implementations will certainly still not work.

If we want to solve this, then we better do it in a more generic way (instead of having to solve it for each "MobX" every now and then).

@bathos, what you propose is a viable approach (and has been discussed before as you found out). The problem is that everything in copy() and equals() affects the whole framework ad may be run several times per second during a digest. Even small performance hits can quickly sum up, so one needs to be careful.

BTW, taking advantage of cloneNode is also a viable hack 😛

[thorn0]

We might be able to make MobX ObservableArray work, by relaxing angular.isArray() to:
...which is a breaking change, but a reasonable one imo. But other MobX stuff or other implementations will certainly still not work.

👍 I've tested it and at least in my project, this seems to be enough for all the watchers to start working fine with MobX observables. I'll submit a PR.

[bathos]

The isArray thing is something I’ve run into before, and when I opened a ticket about it, it was promptly fixed and I am forever grateful:

#14657

But it was only fixed locally in that one usage. Switching to instanceof for angular.isArray is ideal imo.