refactor($http): coalesce calls to $rootScope.$apply WIP

[caitp]

Current state of this: support for evaluating expressions like $evalAsync has not been added -- but it's
trivial to add this.

There are some mock helpers added to enable better testing, although they aren't documented since they
are not likely to be useful to users of the library.

Currently, there's a configuration method to allow users to define the debounce time for $http responses,
but I'm not opposed to removing this, as it doesn't seem particularly useful under normal circumstances.

[mary-poppins]

Thanks for the PR! Please check the items below to help us merge this faster. See the contributing docs for more information.

• Uses the issue template (#7634)

If you need to make changes to your pull request, you can update the commit with git commit --amend.
Then, update the pull request with git push -f.

Thanks again for your help!

[caitp]

Could not inject this directly, due to circular dependencies

[lgalfaso]

I agree that having a solution that works for $http (and any other service that may run into this same issue) would be very nice. Now I do have a few open questions:

1/ I find odd the circular dependency between $rootScope and $timeout
2/ I wonder what should be done in this scenario:

• some process calls $applyAsync(1000)
• less than 1000ms later, something happens and $digest is called

Should there be a second $digets?

3/ Another scenario

• some process calls $applyAsync(100000000)
• another process calls $applyAsync(10)

Why should the second process wait for 100000000 for the apply to happen? Having different behaviors depending in the way this function is being called can have some unexpected consequences

[caitp]

Yeah, I'm not really set in stone on the "configurable debounce time" thing, the default timeout may be good enough for many cases.

Or maybe instead, there could be a different debounce time for each service. I'm not set on all that. Currently only $http uses it at all, anyways.

As for the circular dependency, well you can do this with just $browser.defer() --- but this kind of sucks for testing purposes

Actually, browser.defer probably is good enough, since the timeout mock just wraps it.

[IgorMinar]

There should be only single delay value used by everyone and it should mean "delay at most for X ms".

[IgorMinar]

@kseamon could you test this on DFA?

The workaround that Caitlin came up with means that tests remain mostly unaffected, which is quite nice even though I'd like the implementation to be less hacky.

[caitp]

@kseamon any word on this? It would be good to hear back regarding this =)

[kseamon]

Hi.

I had a working but incomplete demo as of December. Not sure how useful it
is at this point, but let me export it to github...

https://github.com/kseamon/angular.js/tree/http-coalesce

The main thing that is missing is having $apply flush the $applyAsync queue
and cancel the timeout.

Well, that and unit tests, which are ultimately what scared me off of the
task. Getting existing tests that depend on $http to pass while accurately
testing the async behavior is not going to be fun, I fear.

-Karl

On Thu, Jul 31, 2014 at 1:54 PM, Caitlin Potter notifications@github.com
wrote:

@kseamon https://github.com/kseamon any word on this? It would be good
to hear back regarding this =)

—
Reply to this email directly or view it on GitHub
#7634 (comment).

[jeffbcross]

Shouldn't be named like a constant

[kseamon]

Not sure if it's intentionally being left out, but in my unfinished version from a while back, I also had the cookies service use async apply.

[jeffbcross]

@IgorMinar and I discussed this at some length. We're going to sit down with @kseamon next Wednesday 8/20 to run some experiments and get some more data behind the approach. Here are some initial thoughts, but these are just for discussion at this point:

• New policy should be opt-in. Current approach could break tests which are using $timeout for something else. All timeouts would be flushed automatically because of default to Infinity.
• Nix APPLY_ASYNC_WAIT. The delay should not be a specified time (or at least not configurable). Should just use browser.defer similar to @kseamon's branch. Reasoning: limited utility. There's a small range of possible delays which would be useful before becoming a noticeable bottleneck, not worth the flexibility of specifying 10ms vs 20ms. browser.defer will typically end up being 4ms-10ms.
• For consistency, $applyAsync should accept expressions, and add to its own queue, similarly to how @kseamon handled it. Igor thinks $http should add response handlers to this queue, so the scope isn't initially evaluated in a state that becomes stale at the time of coalesced apply.
• $resource, $timeout, $interval should have similar coalescing support
• If digest is already happening at time that request is resolved, ignore coalescing. Especially useful when loading things from cache.
• I think coalescing should be something opted into during config, with each request being able to opt-in or out, overriding the global setting. E.g. $httpProvider.defaults.coalesceApply = true. This would allow Angular to take advantage of it for template loading. Not sure if @IgorMinar agrees with the approach.

[caitp]

My thoughts:

1. Making more separate queues for things is probably not what we want to do. This complicates the codebase more than is needed, the no-brainer is to make a single queue work "better", rather than having different ones for each component. If there are going to be different ones for each component, they should all work the same way.
2. Making coalesced apply opt-in is probably not the smart thing to do. Realistically, having different branches to travel is going to make the code slightly more complicated for no good reason, and I can't really think of a time where you'd not want to do this when it's possible to. All it means is that if we get multiple responses in a short timeframe, they're all handled at the same time, rather than separately. This shouldn't be configured per-application or per-request.
   We could make it configurable, but it's just going to make the api more complicated, and at this point I don't think angular really needs that.
3. $resource is built ontop of $http, so it pretty much gets whatever gimmick we shiv into $http by default.
   For $timeout and $interval, it might be a bit weird to see the DOM update a fraction of a second later than the handler runs, but then again nobody is likely to notice that. Will have to experiment and see how noticeable it really is IMO.
4. If digest is already happening at time that request is resolved, ignore coalescing. Especially useful when loading things from cache. - I'm not sure this ever actually happens. I don't believe the browser should be dispatching events while one event handler is already running, if that is happening, that would strike me as a browser bug. Have to ask anne about that.
5. +1 for removing the configurable APPLY_ASYNC_WAIT.

So, from my POV, I really don't think we should be making this configurable per-request. and I really don't think it should be configurable at all --- unless there's a use case where you'd want to use it most of the time but occasionally not, I don't see the value in making the API more complicated for this. Making the angular api even crazier is really not something that I think we should be doing unless people ask for it. What does the WG think about that?