Skip to content
This repository was archived by the owner on Apr 12, 2024. It is now read-only.

Added invokeApply parameter to $http to skip apply #12557

Closed
wants to merge 2 commits into from
Closed

Added invokeApply parameter to $http to skip apply #12557

wants to merge 2 commits into from

Conversation

Utsav2
Copy link
Contributor

@Utsav2 Utsav2 commented Aug 12, 2015

There are a bunch of times when we don't want a $rootScope.$apply after an http request succeeds. This stackoverflow question gives a good use case: polling.
We can also use this to do scope specific digests, which really helps performance and snappiness on mobile devices.

It also gives us a little more control over the API, and also makes it similar to $timeout.

@realityking
Copy link
Contributor

Small FYI, if you just push again to your branch the pull request will updated. No need to create a new one.

Is there a reason why invokeApply is a parameter and not just another option?

@Utsav2
Copy link
Contributor Author

Utsav2 commented Aug 12, 2015

It's similar to how the $timeout and $interval services, but no other reason. I could change it to use the config object if that's preferable.

@Narretz Narretz added this to the 1.5.x - migration-facilitation milestone Aug 21, 2015
@thorn0
Copy link
Contributor

thorn0 commented Oct 16, 2015

This change isn't enough to prevent the digest from happening because resolving a $q deferred schedules a digest via $evalAsync. $$q should be used instead of $q when invokeApply is true.

As a side note, I wonder why the explicit call to $apply is needed in $http at all? Does anyone know? It looks like a rudiment left from the time when promises weren't used inside $http.

@langdonx
Copy link

langdonx commented Dec 9, 2015

This looks great and will be a great option to improve performance for those of us still doing http polling.

@Utsav2 any chance you can modify the pull request to use $$q in lieu of of $q to get things moving again?

@dragosrususv
Copy link

Can we try to push this in one of the near-future minor releases? / CC @Utsav2

@gkalpak
Copy link
Member

gkalpak commented Mar 7, 2016

As @thorn0 mentioned, this is not enough to prevent the $digest. We need to also somehow swap $q for $$q depending on the invokeApply argument (hacky demo showcasing swapping $q for $$q).

@colinmorelli
Copy link

As @thorn0 mentioned, this is not enough to prevent the $digest. We need to also somehow swap $q for $$q depending on the invokeApply argument (hacky demo showcasing swapping $q for $$q).

Correct me if I'm wrong here, but isn't it enough to just swap $q for $$q in the service - regardless of the parameter value? As mentioned, $http is already going to conditionally invoke $apply with this change. No reason that $q needs to do anything.

@thorn0
Copy link
Contributor

thorn0 commented Sep 28, 2016

@colinmorelli Actually, the $apply() call modified by this PR isn't needed at all because it does nothing. It's just a leftover, and there is a PR to remove it: #13128 (and the issue #13108). After it gets merged, the invokeApply option can be implemented by simple switching between $q and $$q like it's done in $timeout.

@colinmorelli
Copy link

I know this is a larger conversation...but that feels backwards to me. If anything is going to be doing $rootScope.$apply, my opinion is that it should be at the highest level possible. In other words, I'd sooner opt for having $http always use $$q, and schedule digests where necessary than have a low level promise service have a side effect of causing the $rootScope to be digested.

All that said, I'd still like to be able to get this in. Having $http constantly pushing $rootScope changes is painful.

@thorn0
Copy link
Contributor

thorn0 commented Sep 28, 2016

Why do you think $http itself needs to schedule digests? A digest is needed when there is a probability that something was changed in the model. $http doesn't change any scopes. The callbacks of the promises do.

@colinmorelli
Copy link

colinmorelli commented Sep 28, 2016

I think what you said is exactly my point: $http doesn't change any scopes. And $q certainly does not. The callbacks that are provided to them do. In an ideal world, I actually don't think $http should schedule digests either. I'd rather personally have control over which scopes are digested in a callback (think something like thenInScope($scope) as an addition to the promise API).

But, that said, I definitely don't think that creating and resolving a promise with $q should, by default, cause a dirty check to be run across the entire application. $q is a low level building block in angular. It has a plethora of use cases. Now we have $$q and $q because sometimes you don't want $rootScope.$apply to be run, which to me is a clear sign that $q was probably doing something outside of it's scope (no pun intended) to begin with.

I understand why it was done. I think the simplicity of being able to just call .then() and have it "just work" makes sense, especially for early adopters. But as applications grow, the side effect of digesting the entire application every time an $http response comes back becomes less desirable.

@thorn0
Copy link
Contributor

thorn0 commented Sep 28, 2016

Changing the behavior of $q would be too much of a breaking change. So using $$q (or native Promise) and adding the invokeApply option everywhere is the only thing that can be done about it. Apart from$timeout and $http, what else needs it?

@poshest
Copy link
Contributor

poshest commented Oct 27, 2016

+1

I know this is a little off topic, but I noticed that because $http is used to fetch templates (be they triggered by $http.get, ng-include or component templateUrls) there is a $rootScope.$digest() on every one. So if I have 20 templates loading during application bootstrap, there are 20 $rootScope.$digest()s.

Is a digest necessary after templates load? If not, and the invokeApply option was added here (and I would love it for polling, and other reasons) surely we can avoid the digest on every template fetch?

@khaledosman
Copy link

Any updates on this?

@petebacondarwin petebacondarwin modified the milestones: 1.7.0, Backlog Feb 26, 2018
@petebacondarwin
Copy link
Contributor

We feel that this is somewhat of a corner case that is not required for the vast majority of situations.
So we do not intend to implement this feature. Thanks for the work on this @Utsav2.

@poshest
Copy link
Contributor

poshest commented Feb 27, 2018

Oh nuts! What a shame. Well, it's a corner case, except for... probably most non-trivial apps. But I understand the need to rationalise... the resource allocation on Angular 1.x must be asymptoting rapidly to 0. :P

alexcrown pushed a commit to colvir/angular.js that referenced this pull request Aug 15, 2018
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.