fix(ngRoute): make route.resolve count as a pending request #14159

sjelin · 2016-03-02T00:26:00Z

What kind of change does this PR introduce? (Bug fix, feature, docs update, ...)

Make angular count pending resolve values as pending request, for the purposes of making whenStable more accurate.

What is the current behavior? (You can also link to an open issue here)

Currently, if someone uses promises to asynchronously set a resolve, this is not counted as a pending request. The result is that callbacks to whenStable can be invoked before these variables are available. This was causing protractor users problems (see angular/protractor#789 (comment) for details)

What is the new behavior (if this is a feature change)?

Waiting for these variables now counts as a pending request.

Does this PR introduce a breaking change?

I guess you could imagine a breaking change where someone wanted a whenStable callback to be invoked before the variables resolved.

Please check if the PR fulfills these requirements
The commit message follows our guidelines: https://github.com/angular/angular.js/blob/master/CONTRIBUTING.md#commit-message-format
Tests for the changes have been added (for bug fixes / features)
Docs have been added / updated (for bug fixes / features)

Not sure tests are necessary for such a small change. And since this brings behavior in line with user expectations not sure about the docs either.

Other information:

gkalpak · 2016-03-05T08:02:05Z

(Note to self: Related to #13782.)

gkalpak · 2016-07-15T21:47:22Z

@sjelin, this seems reasonable. It needs rebasing and definitely some tests (ideally both unit and e2e).
(I wouldn't consider it a breaking change, but this is debatable 😄)

Would you be interested in moving this forward?

sjelin · 2016-09-08T22:33:39Z

Hi, sorry, yeah - I'll take a look.

sjelin · 2016-12-21T21:13:01Z

Update: I'm back on this. Just rebased, I'll add some tests after lunch

sjelin · 2017-01-03T23:10:39Z

@gkalpak I finally added tests! Sorry for not prioritizing this

gkalpak

I left some comments. We also miss an e2e test.
_{(@sjelin, feel free to ping me if you need any help.)}

gkalpak · 2017-01-04T09:59:13Z

src/ngRoute/route.js

+        return $q.all(locals).then(function(locals) {
+          $browser.$$completeOutstandingRequest(function() {});
+          return locals;
+        });


This should be $.all(locals).finally(function() { $browser.$$completeOutstandingRequest(function() {}); });.
Because now you will never reach 0 outstanding requests once a resolve fails in the app.

gkalpak · 2017-01-04T10:00:16Z

src/ngRoute/route.js

@@ -786,7 +788,10 @@ function $RouteProvider() {
        if (angular.isDefined(template)) {
          locals['$template'] = template;
        }
-        return $q.all(locals);
+        return $q.all(locals).then(function(locals) {
+          $browser.$$completeOutstandingRequest(function() {});


I would use angular.noop here. Actually, I would initialize noop = angular.noop (e.g. the same as isArray) above.

gkalpak · 2017-01-04T10:01:43Z

test/ng/testabilitySpec.js

@@ -194,5 +194,33 @@ describe('$$testability', function() {
        $$testability.whenStable(callback);
        expect(callback).toHaveBeenCalled();
      }));
+
+    it('should wait for route promises before calling callbacks', function() {


This test does not belong here. It should be moved to routeSpec.js.

You should also add a test for where a resolve fails/throws.

sjelin · 2017-01-04T22:15:03Z

@gkalpak all comments addressed

gkalpak

While taking another look, I realized that ngRoute has one more async feature (that might not be there when this PR was first created) which is worth taking into account: resolveRedirectTo

Basically, it is possible to decide about a redirection asynchronously (possibly after some delay). It sounds reasonable to take that delay into account too.

Thus, I suggest expanding the outstanding request increment/decrement "scope" to this block, instead of just around resolveLocals. Maybe something like:

function decrementRequestCount() {
  $browser.$$completeOutstandingRequest(noop);
}

...

$browser.$$incOutstandingRequestCount();

nextRoutePromise.
  then(getRedirectionData).
  then(handlePossibleRedirection).
  then(function(keepProcessingRoute) {
    return keepProcessingRoute && nextRoutePromise.
      then(resolveLocals).
      then(function(locals) {
        // after route change
        if (nextRoute === $route.current) {
          if (nextRoute) {
            nextRoute.locals = locals;
            angular.copy(nextRoute.params, $routeParams);
          }

          $rootScope.$broadcast('$routeChangeSuccess', nextRoute, lastRoute);
        }
      });
  }).
  catch(function(error) {
    if (nextRoute === $route.current) {
      $rootScope.$broadcast('$routeChangeError', nextRoute, lastRoute, error);
    }
  }).
  finally(decrementRequestCount);

WDYT, @sjelin?

gkalpak · 2017-01-05T09:09:59Z

src/ngRoute/route.js

@@ -786,7 +790,9 @@ function $RouteProvider() {
        if (angular.isDefined(template)) {
          locals['$template'] = template;
        }
-        return $q.all(locals);
+        return $q.all(locals).finally(function(locals) {


finally callbacks don't take any arguments.

gkalpak · 2017-01-05T09:17:10Z

test/e2e/tests/ng-route-promise.spec.js

+    expect(element.all(by.tagName('li')).count()).toBe(5);
+  });
+  it('should time out if the promise takes long enough', function() {
+    // Don't try this at home kids, I'm a protractor dev


gkalpak · 2017-01-05T09:19:14Z

test/e2e/tests/ng-route-promise.spec.js

+      expect(error.message).toContain('Timed out waiting for asynchronous Angular tasks to finish after 0.001 seconds.');
+    });
+  });
+  afterAll(function() {


It doesn't make much difference atm, but I believe it is safer to do this afterEach spec (just in case someone adds another test after the second it block).

gkalpak · 2017-01-05T09:22:39Z

test/e2e/tests/ng-route-promise.spec.js

+  beforeEach(function() {
+    loadFixture('ng-route-promise');
+  });
+  it('should wait for promises in resolve blocks', function() {


Add newlines before it blocks.

gkalpak · 2017-01-05T09:26:20Z

test/e2e/fixtures/ng-route-promise/index.html

+    <script src="angular.js"></script>
+    <script src="angular-route.js"></script>
+    <script src="script.js"></script>
+  </head>


Any particular reason for the <head> element? Why not just put the scripts in <body> (for brevity and consistency with other tests)?

Just habit - I'll remove it

gkalpak · 2017-01-05T09:27:21Z

test/e2e/fixtures/ng-route-promise/script.js

+        }
+      }
+    }).
+    otherwise({


Nit: otherwise('/') is shorter 😁

gkalpak · 2017-01-05T09:28:17Z

test/e2e/fixtures/ng-route-promise/script.js

+  $routeProvider.
+    when('/', {
+      template: '<ul><li ng-repeat="letter in letters">{{letter}}</li><ul>',
+      controller: 'LettersCtrl',


Nit: You can get rid of the controller altogether and use $resolve.letters directly in the template.

You don't have to (unless you really want to 😉), but this script could be simply written as:

'use strict'; angular .module('lettersApp', ['ngRoute']) .config(function($routeProvider) { $routeProvider .when('/', { template: '<ul><li ng-repeat="letter in $resolve.letters">{{ letter }}</li></ul>', resolve: { letters: function($q) { return $q(function(resolve) { setTimeout(resolve, 1000, ['a', 'b', 'c', 'd', 'e']); }); } } // Sigh :( - Missing arrow functions already. // resolve: {letters: $q => $q(resolve => setTimeout(resolve, 1000, [...]))} }) .otherwise('/'); });

Will do! An yeah, not having arrow functions is the worse 😢

gkalpak · 2017-01-05T10:13:44Z

(Also, the Travis failures seem legit.)

sjelin

I like the idea of expending the scope of this change to include resolveRedirectTo, but I'm not sure how to handle the following race condition:

A route uses resolveRedirectTo, causing $$incOutstandingRequestCount to be called
Protractor queries for stability, getting enqueued by whenStable
resolveRedirectTo resolves to a new route, thereby calling $$completeOutstandingRequest, and thereby calling the whenStable callbacks. Protractor will get the signal in a few milliseconds, since it takes time for things to go through the selenium server
The new route uses resolve
Protractor, thinking the page is stable, executes some webdriver commands commands even though resolve has not yet resolved
resolve resolves, but too late

sjelin · 2017-01-05T18:30:41Z

src/ngRoute/route.js

@@ -786,7 +790,9 @@ function $RouteProvider() {
        if (angular.isDefined(template)) {
          locals['$template'] = template;
        }
-        return $q.all(locals);
+        return $q.all(locals).finally(function(locals) {


sjelin · 2017-01-05T18:45:54Z

test/e2e/fixtures/ng-route-promise/index.html

+    <script src="angular.js"></script>
+    <script src="angular-route.js"></script>
+    <script src="script.js"></script>
+  </head>


Just habit - I'll remove it

sjelin · 2017-01-05T18:46:57Z

test/e2e/fixtures/ng-route-promise/script.js

+  $routeProvider.
+    when('/', {
+      template: '<ul><li ng-repeat="letter in letters">{{letter}}</li><ul>',
+      controller: 'LettersCtrl',


Will do! An yeah, not having arrow functions is the worse 😢

gkalpak · 2017-01-09T14:18:13Z

I like the idea of expending the scope of this change to include resolveRedirectTo, but I'm not sure how to handle the following race condition: ...

This doesn't seem to be an issue, because of the relative timing of when things happen. The redirection is correctly taken into account (see the test in #15587).

I have incorporated the proposed changes into #15587 (along with a fix in the second test). Feel free cherry-pick the changes into this PR.

sjelin · 2017-01-09T18:41:04Z

Oh, because javascript is synchronous huh? I get it now

sjelin · 2017-01-10T04:38:32Z

test/ngRoute/routeSpec.js

+        $$testability.whenStable(callback);
+        expect(callback).not.toHaveBeenCalled();
+
+        // ngRoute code is wrapped in a $browser.defer call (via $rootScope.evalAsync), which in


@gkalpak this comment is the result of my investigation

Where does the $rootScope.$evalAsync() come from?

Answering my own question: It comes from $location (see here).
So, you were spot on 👍 (which is a good thing, because it confirms that the feature works out-of-the-box outside of unit tests).

I'll mention $location too 😄

gkalpak · 2017-01-10T13:35:16Z

test/e2e/tests/ng-route-promise.spec.js

+    }, function(error) {
+      var errorRe = new RegExp(
+          'Timed out waiting for asynchronous Angular tasks to finish after ' +
+          '(?:1\\.5 seconds|15\\d\\d ?ms)\\.', '');


I know you took this from my PR, but this RegExp is testing more than is necessary (and might give false negatives after a future update). I think it suffixes to have:

expect(error.message).toContain('Timed out waiting for asynchronous Angular tasks to finish');

gkalpak · 2017-01-10T13:36:25Z

test/e2e/tests/ng-route-promise.spec.js

+    // Restore old timeout limit
+    browser.getProcessedConfig().then(function(config) {
+      return browser.manage().timeouts().setScriptTimeout(config.allScriptsTimeout);
+    }).then(function() { done(); });


Couldn't this just be .then(done)?

Probably. In some jasmine setups, passing parameters to the done callback indicates a failed test. This isn't an issue anymore, but I guess it's just a habit of mine now.

gkalpak · 2017-01-10T13:37:07Z

test/ngRoute/routeSpec.js

+        $$testability.whenStable(callback);
+        expect(callback).not.toHaveBeenCalled();
+
+        // ngRoute code is wrapped in a $browser.defer call (via $rootScope.evalAsync), which in


Where does the $rootScope.$evalAsync() come from?

gkalpak · 2017-01-10T16:51:01Z

So, this generally LGTM (now that we have confirmed that the failures where due to unit-test mocks).

Ideally, I would apply some more changes (mainly to tests): 927f355
_{(I don't feel too strongly about it though - feel free to ignore.)}

googlebot · 2017-01-10T19:39:13Z

So there's good news and bad news.

👍 The good news is that everyone that needs to sign a CLA (the pull request submitter and all commit authors) have done so. Everything is all good there.

😕 The bad news is that it appears that one or more commits were authored by someone other than the pull request submitter. We need to confirm that they're okay with their commits being contributed to this project. Please have them confirm that here in the pull request.

Note to project maintainer: This is a terminal state, meaning the cla/google commit status will not change from this state. It's up to you to confirm consent of the commit author(s) and merge this pull request when appropriate.

googlebot · 2017-01-10T19:39:13Z

So there's good news and bad news.

👍 The good news is that everyone that needs to sign a CLA (the pull request submitter and all commit authors) have done so. Everything is all good there.

😕 The bad news is that it appears that one or more commits were authored by someone other than the pull request submitter. We need to confirm that they're okay with their commits being contributed to this project. Please have them confirm that here in the pull request.

Note to project maintainer: This is a terminal state, meaning the cla/google commit status will not change from this state. It's up to you to confirm consent of the commit author(s) and merge this pull request when appropriate.

sjelin · 2017-01-10T19:51:53Z

@gkalpak I adopted your changes but then made some extremely minor ones of my own: 3b106ae

I wrapped the callback in $$completeOutstandingRequest in a try block, which is how it's done in production
I moved some comments around. I wanted some of the explanation to be in src/ngRoute/route.js so that no one looked at that code and wondered "Why is this never causing outstandingRequestCount to hit zero?"
In production, I routed all modifications to outstandingRequestCount through $$incOutstandingRequestCount or $$completeOutstandingRequest. This doesn't fix anything but it just seemed like a good refactor

gkalpak · 2017-01-11T11:06:34Z

src/ng/browser.js

@@ -321,7 +321,7 @@ function Browser(window, document, $log, $sniffer) {
   */
  self.defer = function(fn, delay) {
    var timeoutId;
-    outstandingRequestCount++;
+    self.$$incOutstandingRequestCount();


I don't think this buys us much and is inconsistent with the rest of the file (which either accesses/modifies outstandRequestCount directly or through (the private) completeOutstandingRequest() function).

I disagree but I'll revert this change anyway 😛

gkalpak · 2017-01-11T11:10:50Z

LGTM (Nit: I don't particularly like the 3rd change 😁)

_{@googlebot, I confirm that I am okay with my commits being contributed to this fine project 😃}

googlebot · 2017-01-11T18:57:44Z

CLAs look good, thanks!

googlebot · 2017-01-11T18:57:44Z

CLAs look good, thanks!

sjelin · 2017-01-11T18:58:35Z

@gkalpak alright, should be good to go! I don't have permission to actually do the merge though

Protractor users were having a problem where if they had asynchonous code in a route.resolve variable, Protractor was not waiting for that code to complete before continuing. See angular/protractor#789 (comment) for details Also enhanced ngMock to wait for pending requests before calling callbacks from `$browser.notifyWhenNoOutstandingRequests` Potentially breaking change if someone was relying on route.resolve not blocking `$browser.notifyWhenNoOutstandingRequests`

gkalpak · 2017-01-11T21:17:54Z

Awesome! I can take it from here. Thx for bearing with me till the end 😃

Protractor users were having a problem where if they had asynchonous code in a `route.resolve` or `route.resolveRedirectTo` variable, Protractor was not waiting for that code to complete before continuing. See angular/protractor#789 (comment) for details. This commit fixes it by ensuring that `$browser#outstandingRequestCount` is properly increased/decreased while `$route` (asynchronously) processes a route. Also, enhanced `ngMock` to wait for pending requests, before calling callbacks from `$browser.notifyWhenNoOutstandingRequests()`. Related to angular/protractor#789. Closes #14159

Protractor users were having a problem where if they had asynchonous code in a `route.resolve` or `route.resolveRedirectTo` variable, Protractor was not waiting for that code to complete before continuing. See angular/protractor#789 (comment) for details. This commit fixes it by ensuring that `$browser#outstandingRequestCount` is properly increased/decreased while `$route` (asynchronously) processes a route. Also, enhanced `ngMock` to wait for pending requests, before calling callbacks from `$browser.notifyWhenNoOutstandingRequests()`. Related to angular/protractor#789. Closes angular#14159

googlebot added the cla: yes label Mar 2, 2016

gkalpak added type: feature needs: investigation component: ngRoute frequency: moderate severity: inconvenient labels Mar 5, 2016

gkalpak added this to the 1.5.x milestone Mar 5, 2016

gkalpak self-assigned this Mar 5, 2016

gkalpak mentioned this pull request Jul 15, 2016

Protractor does not wait to resolve promises on routes angular/protractor#789

Closed

gkalpak added needs: work and removed needs: investigation labels Jul 15, 2016

gkalpak modified the milestones: Purgatory, 1.5.x Jul 15, 2016

sjelin force-pushed the routeResolvePending branch from 11956ba to d4114be Compare December 21, 2016 21:12

sjelin force-pushed the routeResolvePending branch 2 times, most recently from 6a73d5b to 7d94d0a Compare January 3, 2017 23:09

gkalpak suggested changes Jan 4, 2017

View reviewed changes

sjelin force-pushed the routeResolvePending branch from 7d94d0a to 3fa796f Compare January 4, 2017 22:13

gkalpak suggested changes Jan 5, 2017

View reviewed changes

sjelin commented Jan 5, 2017

View reviewed changes

gkalpak mentioned this pull request Jan 9, 2017

WIP - feat($route): wait for resolves #15587

Closed

sjelin force-pushed the routeResolvePending branch from 05b2cc7 to aa05cde Compare January 10, 2017 04:32

sjelin commented Jan 10, 2017

View reviewed changes

sjelin force-pushed the routeResolvePending branch from aa05cde to 785c987 Compare January 10, 2017 04:59

gkalpak reviewed Jan 10, 2017

View reviewed changes

googlebot added cla: no and removed cla: yes labels Jan 10, 2017

sjelin force-pushed the routeResolvePending branch 2 times, most recently from 3b106ae to cb55b32 Compare January 10, 2017 19:50

gkalpak reviewed Jan 11, 2017

View reviewed changes

gkalpak approved these changes Jan 11, 2017

View reviewed changes

sjelin force-pushed the routeResolvePending branch from cb55b32 to 4567b9d Compare January 11, 2017 18:57

googlebot added cla: yes and removed cla: no labels Jan 11, 2017

sjelin force-pushed the routeResolvePending branch 2 times, most recently from 26d10a0 to 6aca56b Compare January 11, 2017 19:35

gkalpak closed this in c522a43 Jan 11, 2017

scottohara mentioned this pull request Mar 12, 2017

fix($timeout): verifyNoPendingTasks() throws even for non-timeout pending tasks #14336

Closed

fix(ngRoute): make route.resolve count as a pending request #14159

fix(ngRoute): make route.resolve count as a pending request #14159

Conversation

sjelin commented Mar 2, 2016

gkalpak commented Mar 5, 2016

gkalpak commented Jul 15, 2016

sjelin commented Sep 8, 2016

sjelin commented Dec 21, 2016

sjelin commented Jan 3, 2017

gkalpak left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sjelin commented Jan 4, 2017

gkalpak left a comment • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

gkalpak Jan 5, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

gkalpak commented Jan 5, 2017

sjelin left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

gkalpak commented Jan 9, 2017

sjelin commented Jan 9, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

gkalpak commented Jan 10, 2017

googlebot commented Jan 10, 2017

googlebot commented Jan 10, 2017

sjelin commented Jan 10, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

gkalpak commented Jan 11, 2017

googlebot commented Jan 11, 2017

googlebot commented Jan 11, 2017

sjelin commented Jan 11, 2017

gkalpak commented Jan 11, 2017 • edited Loading

gkalpak left a comment •

edited

Loading

gkalpak Jan 5, 2017 •

edited

Loading

gkalpak commented Jan 11, 2017 •

edited

Loading