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

Resource.get xhr is not executed when updating to Angular 1.1.4 #2438

Closed
tkremmel opened this issue Apr 18, 2013 · 23 comments
Closed

Resource.get xhr is not executed when updating to Angular 1.1.4 #2438

tkremmel opened this issue Apr 18, 2013 · 23 comments
Milestone

Comments

@tkremmel
Copy link

The code below works fine in 1.1.3, but does nothing in 1.1.4. It only prints "after get" and it seems as the get branch is never entered. From the debugging console I can see that no xhr request is issued.

app.factory('gridData', function($resource) {

    var csrf = 'CSRFTOKENEXAMPLE';

    return $resource('/list-management/rest/list/items/1/:itemID', {itemID:'@id'},{

         get: {method:'GET'},

         query: {method:'GET', isArray:true},

         save:{method:'POST', headers: {'X-CSRFToken' : csrf }},

     });

});


 gridData.get({itemID:item.id}, function(result) {
    //request ok

    console.log("in get status ok");

    dataView.updateItem(result.id, result);

    dataView.refresh();

 }, function(response) {

     //404 or bad

    console.log("bad request");

    if(response.status === 404) {

    }
 });

 console.log("after get");
@tkremmel
Copy link
Author

Interestingly the get call is executed if wrapped into a $scope.$apply call:

$scope.$apply( function(){
    gridData.get({itemID:item.id}, function(result) {
    console.log("in get");
    dataView.updateItem(result.id, result);
    dataView.refresh();
 }, function(response) {
   //404 or bad
   console.log("bad request");
    }
)

See this discussion for details: https://groups.google.com/forum/?fromgroups=#!topic/angular/MkXDfkPDI_g

@admhemed
Copy link

same problem. your solution works as well.

@michaelahlers
Copy link

I've seen this, too. The following fails (the callback is never invoked).

Customers.query({
  terms : terms
}, function (results) {
  $log.log('Callback.', results)
})

But the following works fine.

$scope.$apply(function () {
  Customers.query({
    terms : query.term
  }, function (results) {
    $log.log('Callback.', results)
  })
})

Customers in this case is a $resource object.

Now, this is getting called from outside the Angular digest cycle, but it has worked prior to 1.1.4.

@aflock
Copy link

aflock commented May 15, 2013

Seeing this as well, any update on if this is intended behavior? Also, any hunches on what change caused this? (presumably changes to the promise that is associated with $resource)

@michaelahlers
Copy link

@aflock (and others), on second thought, this probably is intended behavior. In my use case, the block I shared I called from a jQuery plugin. Any other AngularJS call that participates in the digest lifecycle would have the same requirement (to be wrapped in $apply). Why would $resource objects differ?

@aflock
Copy link

aflock commented May 16, 2013

I don't think this is intended behavior. Take a look at this fiddle http://jsfiddle.net/s3THR/7/ .
If you run it with 1.1.4, the request is not run until a digest cycle is applied through ng-click.
If you downgrade to 1.1.1-3, the request is run as soon as the event is received.
My guess is, $resource was changed in a way that now requires a digest cycle to run.
This change isn't listed in changelog.md, so if it is intended behavior, it isn't documented anywhere.

To your point @michaelahlers why should resource be bound into a digest lifecycle? Shouldn't it be able to function independently? That's a facet of this I am unsure of, but my gut says it should be able by itself.

@michaelahlers
Copy link

@aflock, I seem to remember that an upcoming enhancement for $resource was that it's methods will now return promises. Those are most certainly bound to the lifecycle.

@c0bra
Copy link

c0bra commented May 16, 2013

I definitely think this is a bug in $http, possibly introduced in 4ae4681.

  1. Here is @aflock's jsfiddle (which uses $resource) turned into a plunker: http://plnkr.co/edit/s25EEQ86RCTN2AvJNSmi?p=preview The main point is that the XHR get is not executed until AFTER the next $digest cycle. If you change the angular.js script tag from 1.1.4 to 1.1.3 it will work as expected.
  2. Here is a modification of the plunker above using $http instead of $resource: http://plnkr.co/edit/BP2oxBxMGte1axB2gZmx?p=preview It fails in the same manner, and will work if you once again change from 1.1.4 to 1.1.3.
  3. Here is yet another modification using $q and $timeout instead of $http: http://plnkr.co/edit/YcBoIpBW2yXuulmtI01h?p=preview It works fine.

@c0bra
Copy link

c0bra commented May 16, 2013

git bisect confirms, issue introduced in 4ae4681

@aflock
Copy link

aflock commented May 16, 2013

It seems this was a known change introduced by this PR #2130, but not listed in the change log. Not sure if the consequences were totally known.

@mokesmokes
Copy link

@michaelahlers, I doubt this was intended behavior since the natural expectation is that $http and $resource run in the Angular execution context - and thus an additional $apply should actually cause an error. If for some reason this was intended, then as we can see from the numerous bug reports re this issue, there is widespread confusion. Personally - I reverted to 1.0.6 rather than modifying my code, with the expectation that future versions will be fixed back to "expected" and documented behavior. It would be nice to get guidance from the core team on this.

@mokesmokes
Copy link

So we still see the differences in behavior between 1.1.5 and 1.0.7 - I tried @c0bra's Plunkers above with the new releases. So my guess is that this is the new $http/$resource behavior. Can we _please_ get clarity and guidance on this from the core team? What are the new best practices for $http and $resource? I doubt the solution is to wrap $http calls with $apply - feels like a complete hack.
Thanks!

@mokesmokes
Copy link

It's passing testing because in resourceSpec.js we have this:

afterEach(function() {
    $httpBackend.verifyNoOutstandingExpectation();
});

Where verifyNoOutstandingExpectation() contains $rootScope.$digest(). httpSpec.js calls $rootScope.$digest() for good measure just prior to the verifyNoOutstandingExpectation() call :)
So either the docs need to be updated to show the required digest, or this should be fixed. What's the performance impact due to the extra required digest() on every server request?

@aflock
Copy link

aflock commented May 23, 2013

@mokesmokes I'm not sure it will negatively impact performance, I believe the digest is simply no longer done by $http, now that it is wrapped in a promise to allow chaining. It is a breaking change though, and I like @c0bra's suggestion in here: #2431. Unfortunately I'm not sure if we'll get any word from the angular team without a pull request :(

@michaelahlers
Copy link

I'm restating my position at this point, but this behavior is logical for the reasons @aflock gives. Any time you invoke Angular operations from external libraries, you've got to do this. It is a bug, however, if you need to wrap $http or $resource calls in $apply if you're invoking them from, say, an ng-click handler. Otherwise, this is expected behavior.

@aflock
Copy link

aflock commented May 23, 2013

The fact is that you didn't have to do this in testing up until now, or in activities that were not part of a $digest cycle, e.g. in an event handler.

@mokesmokes
Copy link

@aflock, following the code path in 1.0.6, there was no prior digest cycle, and intuitively none was needed. In 1.0.6 the $http flow basically checks the cache, and if the request is not cached it does the expected - issues an xhr in completely standard fashion. Digest cycles should only be required at the completion to update the bindings. The new flow is not only breaking, in my opinion it's counterintuitive with the entire API. I'm not saying it's necessarily bad - I am saying the logic is far from intuitive (and I admittedly don't get it), and an in depth explanation, code samples, etc are urgently needed. I would love to upgrade to 1.1.5, but holding off until I thoroughly understand how we're supposed to use $http and $resource.

@aflock
Copy link

aflock commented May 23, 2013

@mokesmokes completely agree. perhaps @mhevery , @vojtajina or @IgorMinar could shed some light?

@mokesmokes
Copy link

@michaelahlers - I respectfully disagree with your last post - e.g. $timeout. For $timeout, $apply() is called unless you explicitly set invokeApply to false. This is the case in 1.0.6 and 1.1.5. You normally need to wrap code in $apply only when you update the model outside the Angular context. Angular functions are expected, by default, to run in Angular context, as $http and $resource prior to this change, and $timeout et. al in all versions.

@stephen
Copy link

stephen commented Jul 9, 2013

Is there any update on this issue? Is the correct way to resolve this to wrap the call in $scope.$apply() when outside of a regular angular context (e.g. calling from jQuery)?

This gave me a terrible time trying to figure out what was going wrong between moving from 1.0.6 to 1.1.4, it'd be nice to have this behavior documented.

@aflock
Copy link

aflock commented Jul 9, 2013

Yes that's correct, see discussion here: #2431

@stephen
Copy link

stephen commented Jul 9, 2013

@aflock Thanks much. Is there a reason why this issue is still open? It made it seem like the proper resolution for this was not found yet.

@pkozlowski-opensource
Copy link
Member

@StephenCWan You are right, it should get closed now.

IgorMinar added a commit to IgorMinar/angular.js that referenced this issue Aug 22, 2013
This change causes a new $digest to be scheduled in the next tick if
a task was was sent to the $evalAsync queue from outside of a $digest
or an $apply.

While this mode of operation is not common for most of the user code,
this change means that $q promises that utilze $evalAsync queue to
guarantee asynchronicity of promise apis will now also resolve outside
of a $digest, which turned out to be a big pain point for some developers.

The implementation ensures that we don't do more work than needed and
that we coalese as much work as possible into a single $digest.

The use of $browser instead of setTimeout ensures that we can mock out
and control the scheduling of "auto-flush", which should in theory
allow all of the existing code and tests to work without negative
side-effects.

Closes angular#3539
Closes angular#2438
IgorMinar added a commit to IgorMinar/angular.js that referenced this issue Aug 25, 2013
This change causes a new $digest to be scheduled in the next tick if
a task was was sent to the $evalAsync queue from outside of a $digest
or an $apply.

While this mode of operation is not common for most of the user code,
this change means that $q promises that utilze $evalAsync queue to
guarantee asynchronicity of promise apis will now also resolve outside
of a $digest, which turned out to be a big pain point for some developers.

The implementation ensures that we don't do more work than needed and
that we coalese as much work as possible into a single $digest.

The use of $browser instead of setTimeout ensures that we can mock out
and control the scheduling of "auto-flush", which should in theory
allow all of the existing code and tests to work without negative
side-effects.

Closes angular#3539
Closes angular#2438
jamesdaily pushed a commit to jamesdaily/angular.js that referenced this issue Sep 25, 2013
This change causes a new $digest to be scheduled in the next tick if
a task was was sent to the $evalAsync queue from outside of a $digest
or an $apply.

While this mode of operation is not common for most of the user code,
this change means that $q promises that utilze $evalAsync queue to
guarantee asynchronicity of promise apis will now also resolve outside
of a $digest, which turned out to be a big pain point for some developers.

The implementation ensures that we don't do more work than needed and
that we coalese as much work as possible into a single $digest.

The use of $browser instead of setTimeout ensures that we can mock out
and control the scheduling of "auto-flush", which should in theory
allow all of the existing code and tests to work without negative
side-effects.

Closes angular#3539
Closes angular#2438
jamesdaily pushed a commit to jamesdaily/angular.js that referenced this issue Jan 27, 2014
This change causes a new $digest to be scheduled in the next tick if
a task was was sent to the $evalAsync queue from outside of a $digest
or an $apply.

While this mode of operation is not common for most of the user code,
this change means that $q promises that utilze $evalAsync queue to
guarantee asynchronicity of promise apis will now also resolve outside
of a $digest, which turned out to be a big pain point for some developers.

The implementation ensures that we don't do more work than needed and
that we coalese as much work as possible into a single $digest.

The use of $browser instead of setTimeout ensures that we can mock out
and control the scheduling of "auto-flush", which should in theory
allow all of the existing code and tests to work without negative
side-effects.

Closes angular#3539
Closes angular#2438
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

No branches or pull requests

9 participants