[WIP] Add listener/watcher/observer assertions, many bug fixes

[mmun]

Remaining work

• Check that these assertions don't break apps.
• Figure out how to fix the remaining test failure in Ember.computed.sort. It will likely require completing Add Weakmap – Weak from Map -> Obj, not Obj -> Map #12224 and Fix for pr 12215 #12221.

---

This PR adds assertions to adding/removing listeners/watchers/observers in order to catch several bugs. I strongly believe that these strict assertions will help catch many classes of bugs that were difficult to catch before. In fact, it has already exposed several bugs in the existing test suite.

The new assertions boil down to:

1. Don't allow adding the same observer twice.
2. Don't allow removing an observer that has not been added.

Here's an example of 1.

let user = {};
function nameChanged() { }
Ember.addObserver(user, 'name', nameChanged);
Ember.addObserver(user, 'name', nameChanged); // This throws

and of 2.

let user = {};
Ember.removeObserver(user, 'name', 'nameChanged'); // This throws

[mmun]

The ember-data tests pass flawlessly with this build of ember (good job ember-data team!). I tested it in one my apps and the acceptance tests did not pass. It looks like a bug in the observer logic of ember-simple-auth, unfortunately.

I'm not too sure how to proceed now. I think that these asserts are extremely valuable, but we can't hard-break addons (even though they may already be soft-broken).

[mmun]

Another win! The ArrayProxy bug that was fixed in this PR (and caught by the new asserts!) fixes the variation of #12475 that I was running into. See these JSBins:

http://jsbin.com/fezola/1/edit?html,js,output (running 2.3.0)
http://jsbin.com/yiduru/edit?html,js,output (running this build of ember)

In the second JSBin note how the numbers show up correctly. There are more detailed "steps to reproduce" contained inside the JSBin.

This build also fixes a similar bug in a JSBin mentioned in #12475 (comment).

I will close my other bug fix attempt #12860 in favor of this one.

[mmun]

Since I don't see this PR getting merged for some time, I will likely extract the array proxy bug fix into a separate PR so we can close #12475 as soon as possible.

[rwjblue]

JSCS rules are failing for the lack of space between 'i' and '+' (lines below also).

[rwjblue]

I'm in favor of landing these assertions on the canary cycle, and specifically calling them out in the 2.5.0-beta blog post. That should give folks enough time (full canary and beta cycle) to fix any addons.

[mixonic]

@rwjblue @mmun are they worth an intimate-API-usage deprecation message in 2.4?

[rwjblue]

@mixonic - It is tricky. Even though things seem to be working properly there are almost certainly bugs (as shown by the fix to ArrayProxy that these assertions helped to discover). I do not think we are deprecating an intimate API here, we are providing an early warning that there is a bug.

I agree that breaking apps is not great though. Perhaps this could be a warning for at least one release (2.5.0 most likely) or even an LTS release (which I am not sure when the first will be), then changed to an assert. That would give quite a bit more time to deal with these in addons/etc, and still allow folks to change that warning into an error (via debug handlers feature)...

[mixonic]

@rwjblue yeah I suggest 2.4 based on the idea it will be LTS.

The is no "intimate API", agreed, however there is what I'd call "intimate behavior" such as removeObserver working when there is no observer installed. I think we're on the same page.

[rwjblue]

The is no "intimate API", agreed, however there is what I'd call "intimate behavior" such as removeObserver working when there is no observer installed. I think we're on the same page.

Yes, agreed. I definitely do not want to wait until 3.0 to change to asserts, but treating this like the removal of intimate API (and waiting for an LTS to switch to asserts) definitely makes sense.

[mixonic]

To be clear @rwjblue and I suggest you add warnings (not deprecations) in 2.4, then change them to asserts in 2.5.

[homu]

☔ The latest upstream changes (presumably #12894) made this pull request unmergeable. Please resolve the merge conflicts.

[rwjblue]

Seems like this may have missed the train. I would still like to land these as warnings, but at this point they will need to land in 2.6/2.7 and we can remove after 2.8 (next LTS).

@mmun - Do you think you can take this to completion?

[mmun]

@rwjblue I forgot about this. I'll try to land it piece by piece. I can do the watching warnings separately from the listening warnings. I don't really like that the addToListeners doesn't correctly check all the way up parent classes because of laziness. :/

[mmun]

I'll close it for now.