arrayContentDidChange/arrayContentWillChange can no longer be overriden

[workmanw]

Carry over from this PR: #13534

In that PR, arrayContentDidChange and arrayContentWillChange were modified to make them implemented and invoked as pure functions (as opposed to object-oriented methods). That PR is problematic for ember-data-model-fragments. The root of the problem is that because replace (and others) no longer call this.arrayContentDidChange(...), instead calling arrayContentDidChange(this, ...), we are unable to override this function in our subclasses array. See here: array/stateful.js#L178-L190.

There was some discussion about about whether or not this was a breaking change because the API is not changing, just the behavior of a function.

If possible, we should try to sort this out before Ember 2.8 ships, otherwise we'll have an upgrade blocker to resolve in ember-data-model-fragments.

[Serabe]

Thank you for taking the time to open this!

[mmun]

In general, one should assume that framework provided methods are not overridable unless explicitly documented so. In other words, borrowing from Java, they are final. We should improve our documentation in this regard.

Nevertheless, as 2.8 is an LTS we should fix this. It may be sufficient to restore the old behaviour only for classes that implement the Array mixin, like ArrayProxy in this case.

[workmanw]

In general, one should assume that framework provided methods are not overridable unless explicitly documented so. In other words, borrowing from Java, they are final. We should improve our documentation in this regard.

Yea I can certainly understand this. I'm definitely not asserting that the right solution is to restore old functionality (though admittedly it would make my life easier). More than anything else I wanted to bring this up and kick off a discussion.

---

I think the reason I brought this up to begin with was because it impacts a data structure / interfact that is often extended.

If we step back and look, this specific case is not unique. Along with arrayContent{Did,Will}Change, removeAt was also converted to a pure function. BUT it was done with more care. Take a look at the popObject, shiftObject and removeObject implementations on the MutableArray mixin. They all continue to call this.removeAt(...) rather than using the newly refactored removeAt(this, ...). Why is that?

The answer is simply because Ember.ArrayProxy provides it's own implementation for removeAt, see: system/array_proxy.js. If Ember.MutableArray were to use the removeAt pure function, rather than the class method, then Ember.ArrayProxy would have to also reimplement popObject, shiftObject and removeObject.

It feels arbitrary to me to say that it's okay to override removeAt and not arrayContent{Did,Will}Change.

[mmun]

@workmanw This is because the work is only partially complete. The goal is to be able to avoid putting anything on the native Array prototype if Array prototype extensions are disabled.

I doubt that anyone is reopening the NativeArray/MutableArray mixins or the Array prototype so there shouldn't be any problem with my proposed strategy.

[mmun]

and yes, ArrayProxy would have to reimplement all those methods as you said if it wanted to remain "calling behaviour compatible". We might not want to bother doing that in 2.9. But for 2.8 LTS I'd like to avoid the breaking to this addon.

[rwjblue]

I think this was fixed by #14117, can someone confirm against canary builds?

[workmanw]

@rwjblue WOOT 🙌 . Confirmed fixed with: 2.9.0-master+f97598ea. Thank you!