refactor($http): remove unneeded call to $apply

[thorn0]

This $apply call was needed only for tests, so it's been moved to $httpBackend.flush. An example of such a test can be found in ngIncludeSpec.js. It fails if $digest isn't synchronously invoked after each response. See also the comments I included in the code of the tests.

BREAKING CHANGE: $httpProvider.useApplyAsync was removed

Closes #13108
Closes #13111

[gkalpak]

@thorn0, I finally got around to taking a proper look. It generally lgtm, with the exceptions of the changes to tests. It's too much imo (and this doesn't properly reflect the fact that the actual implications in behaviour are minimal).
I will submit a cleaned up (imo) version of the test-related changes later, so we can discuss further.

@petebacondarwin, I guess we are OK with the BC for v1.5.x. Also, wondering if, by essentially making applyAsync the default (and only) behaviour, we are breaking someone's app. I would be very surprized if someone relied on the fact that there would be a digest after each and every response (and before any other response's promise gets resolved).
All things considered, I don't think it's worth getting it into v1.4.x (but it's not difficult either).
(If we decided to backport it, we could avoid the BC of removing useApplyAsync(), by replacing it with a noop (and possibly logging a deprecation message).)

So, are we OK with having applyAsync as the default (and only) behaviour for v1.5.x ?

[gkalpak]

See gkalpak@e00f10c (wrt to the test changes).

[thorn0]

@gkalpak My main objection against your cleanup is that if we leave $httpBackend.flush unchanged, we'll for sure break a lot of unit tests. Just like in your version of that test in ngIncludeSpec, it's totally counter-intutive that $httpBackend.flush() must be invoked multiple times in a row for everything to start working.

I would be very surprized if someone relied on the fact that there would be a digest after each and every response

Actually, lots of code relies on this. And this code is unit tests. That's why $httpBackend.flush has to be accordingly modified.

[jprince]

My main objection against your cleanup is that if we leave $httpBackend.flush unchanged, we'll for sure break a lot of unit tests. Just like in your version of that test in ngIncludeSpec, it's totally counter-intutive that $httpBackend.flush(); must be invoked multiple times in a row for everything to start working.

FWIW I stumbled upon this PR while investigating why setting useApplyAsynch(true) when upgrading to 1.4.x has caused a few of my unit tests to fail. Thankfully someone had done a nice writeup on this issue (http://www.klaascuvelier.be/2015/09/angular-applyasync-httpbackend/) which helped me identify the remediation. At any rate, I agree with @thorn0 that the "solution" of invoking $httpBackend.flush(); multiple times is unintuitive.

[petebacondarwin]

We are not going to get this into 1.5.0.

[jbedard]

What ever happened to this? What do people think about something like this or maybe just removing useApplyAsync (and making it always true)? It would be nice to remove such flags...

[thorn0]

Can we probably at least deprecate useApplyAsync (and scope#$applyAsync) in 1.7?

[gkalpak]

What is wrong with scope#$applyAsync()?

[thorn0]

Internally it's used only in $http for useApplyAsync. As a public API, it's very confusing because it's not clear at all what its purpose is and why one would prefer it over $evalAsync.

[petebacondarwin]

Thanks for this @thorn0 - at this point in the project's lifecycle we don't feel that this adds enough value to be worth the potential breaking change. So we are going to close this as won't merge.

[thorn0]

@petebacondarwin We should've at least removed/deprecated $httpProvider.useApplyAsync, which does nothing but, as per the docs, promises 'significant performance improvement'.

[gkalpak]

AFAICT, with useApplyAsync(false) there can indeed be more digests triggered. So, useApplyAsync(true) should indeed offer better performance.

[thorn0]

Oh, right. So why not make it the default behavior?

[gkalpak]

Because it might be a breaking change. In retrospect, I'm not sure why we didn't do it for 1.7.0 😕