[BUGFIX release] ArrayProxy#content should not be a computed property

[mmun]

Fixes #12475.

UPDATE: This reasoning isn't correct. See #12860 (comment).

This commit restores the contract of ArrayProxy content being a simple property instead of a computed property. This prevents accidental clobberings such as in ember-data:

https://github.com/emberjs/data/blob/v2.3.3/addon/-private/system/record-arrays/record-array.js#L44

This commit also removes a private hook arrangedContentArrayWillChange and partially removes another private hook arrangedContentWillChange. In the interest of keeping this bugfix commit minimal, we will continue removing the will* hooks on canary.

As the bug was caused by ember-data clobbering a descriptor, there is no simple way to test this within ember. I would recommend adding an integration test to ember-data that covers this case.

cc @krisselden @stefanpenner @ef4 @fivetanley

[ef4]

We still don't have a root cause analysis of this bug. I suspect it is revealing a problem deeper in ember-metal. Maybe one of the ones @stefanpenner uncovered recently.

[Edit to add: I was referring to https://github.com//pull/12832, but it's unclear to me whether there are any real remaining bugs there once we stopped trolling ourselves with array prototype extension issues in the tests themselves.]

CP clobbering should cause the arrangedContentArrayWillChange and arrangedContentWillChange hooks to not fire, but this PR removes those hooks anyway. So what was it about the presence of the CP that caused dropped events?

[mmun]

Should we hold off on merging this until we've attempted to find the root cause? I'm looking now :)

[mmun]

@ef4: Actually, I got it backwards. The problem is not with clobbering. In fact, the places where ember-data clobbers the content descriptor actually work around the bug :P. The bug occurs when the content descriptor is not clobbered! For example, with the PromiseArray in ember-data.

The bug seems to occur in the call to this.arrangedContentArrayWillChange(...). When I removed this line my app started working. I haven't looked into why exactly this breaks, but I also don't really care too much because it's only to support before observers.

[stefanpenner]

I would like to review this. But won't have time until Monday

[rwjblue]

@stefanpenner - Have you had a chance to review?

[mmun]

@rwjblue: Once @stefanpenner signs off I will update the commit message. It does not have the correct reasoning.

[stefanpenner]

I haven't looked into why exactly this breaks, but I also don't really care too much because it's only to support before observers.

I would like to know why this breaks. Unfortunately I didn't end up having time tonight.
To accept this bugfix, we should have a corresponding regression test case. Something codifying @mmun's scenario, so future refactoring take it into account.

[stefanpenner]

we should also mark arrangedContent as something that will be deprecated

[mmun]

The old array proxy set up before and "after" observers for arrangedContent. These two observers eventually call Array#arrayContentWillChange and Array#arrayContentDidChange respectively. These two Array methods are used to send array events (the events you are subscribing to when you use array.addArrayObserver). They also have the side effect of tearing down / setting up each proxy observers respectively. See here and here.

Consequently, every call to arrayContentWillChange is meant to be followed by a call to arrayContentDidChange so that the each proxy can setup observers on the new content array.

The ArrayProxy bug occurred when we moved the logic out of a before observer and into a setter.

The problem with this is that setters are always called, but before observers are only called if they are being watched. Since we left the "after" observer logic the way it was, this caused an asymmetry in calls to arrayContentWillChange and arrayContentDidChange.

As stated earlier, the extra call to arrayContentWillChange caused the each proxy to try remove observers from each of its items, which caused watch counts to needless decrement, which in turn caused the watcher count to hit 0 prematurely and thus prevent observers from firing.

Sorry for the messy explanation. It's a tricky bug to explain. Especially at 4 AM 🌝

[rwjblue]

@mmun - Thanks for the detailed walk through!

[ef4]

Thanks @mmun 👏

[rwjblue]

@mmun - Can you PR the fix from 7552f6f with [BUGFIX release] so we can get the fix released?

[rwjblue]

For those interested, this should end up in 2.4.0-beta.2 and 2.3.1.