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

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

Closed
wants to merge 1 commit into from
Closed

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

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
34 changes: 26 additions & 8 deletions src/ngMock/angular-mocks.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -37,10 +37,30 @@ angular.mock.$Browser = function() {
self.$$lastUrl = self.$$url; // used by url polling fn
self.pollFns = [];

// TODO(vojta): remove this temporary api
self.$$completeOutstandingRequest = angular.noop;
self.$$incOutstandingRequestCount = angular.noop;

// Testability API

var outstandingRequestCount = 0;
var outstandingRequestCallbacks = [];
self.$$incOutstandingRequestCount = function() { outstandingRequestCount++; };
self.$$completeOutstandingRequest = function(fn) {
try {
fn();
} finally {
outstandingRequestCount--;
if (!outstandingRequestCount) {
while (outstandingRequestCallbacks.length) {
outstandingRequestCallbacks.pop()();
}
}
}
};
self.notifyWhenNoOutstandingRequests = function(callback) {
if (outstandingRequestCount) {
outstandingRequestCallbacks.push(callback);
} else {
callback();
}
};

// register url polling fn

Expand All @@ -65,6 +85,8 @@ angular.mock.$Browser = function() {
self.deferredNextId = 0;

self.defer = function(fn, delay) {
// Note that we do not use `$$incOutstandingRequestCount` or `$$completeOutstandingRequest`
// in this mock implementation.
delay = delay || 0;
self.deferredFns.push({time:(self.defer.now + delay), fn:fn, id: self.deferredNextId});
self.deferredFns.sort(function(a, b) { return a.time - b.time;});
Expand Down Expand Up @@ -166,10 +188,6 @@ angular.mock.$Browser.prototype = {

state: function() {
return this.$$state;
},

notifyWhenNoOutstandingRequests: function(fn) {
fn();
}
};

Expand Down
14 changes: 13 additions & 1 deletion src/ngRoute/route.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -7,6 +7,7 @@
var isArray;
var isObject;
var isDefined;
var noop;

/**
* @ngdoc module
Expand Down Expand Up @@ -54,6 +55,7 @@ function $RouteProvider() {
isArray = angular.isArray;
isObject = angular.isObject;
isDefined = angular.isDefined;
noop = angular.noop;

function inherit(parent, extra) {
return angular.extend(Object.create(parent), extra);
Expand Down Expand Up @@ -350,7 +352,8 @@ function $RouteProvider() {
'$injector',
'$templateRequest',
'$sce',
function($rootScope, $location, $routeParams, $q, $injector, $templateRequest, $sce) {
'$browser',
function($rootScope, $location, $routeParams, $q, $injector, $templateRequest, $sce, $browser) {

/**
* @ngdoc service
Expand Down Expand Up @@ -680,6 +683,8 @@ function $RouteProvider() {

var nextRoutePromise = $q.resolve(nextRoute);

$browser.$$incOutstandingRequestCount();

nextRoutePromise.
then(getRedirectionData).
then(handlePossibleRedirection).
Expand All @@ -700,6 +705,13 @@ function $RouteProvider() {
if (nextRoute === $route.current) {
$rootScope.$broadcast('$routeChangeError', nextRoute, lastRoute, error);
}
}).finally(function() {
// Because `commitRoute()` is called from a `$rootScope.$evalAsync` block (see
// `$locationWatch`), this `$$completeOutstandingRequest()` call will not cause
// `outstandingRequestCount` to hit zero. This is important in case we are redirecting
// to a new route which also requires some asynchronous work.

$browser.$$completeOutstandingRequest(noop);
});
}
}
Expand Down
9 changes: 9 additions & 0 deletions test/e2e/fixtures/ng-route-promise/index.html
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,9 @@
<html ng-app="lettersApp">
<body>
<div ng-view></div>

<script src="angular.js"></script>
<script src="angular-route.js"></script>
<script src="script.js"></script>
</body>
</html>
43 changes: 43 additions & 0 deletions test/e2e/fixtures/ng-route-promise/script.js
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,43 @@
'use strict';

angular.
module('lettersApp', ['ngRoute']).
config(function($routeProvider) {
$routeProvider.
otherwise(resolveRedirectTo('/foo1')).
when('/foo1', resolveRedirectTo('/bar1')).
when('/bar1', resolveRedirectTo('/baz1')).
when('/baz1', resolveRedirectTo('/qux1')).
when('/qux1', {
template: '<ul><li ng-repeat="letter in $resolve.letters">{{ letter }}</li></ul>',
resolve: resolveLetters()
}).
when('/foo2', resolveRedirectTo('/bar2')).
when('/bar2', resolveRedirectTo('/baz2')).
when('/baz2', resolveRedirectTo('/qux2')).
when('/qux2', {
template: '{{ $resolve.letters.length }}',
resolve: resolveLetters()
});

// Helpers
function resolveLetters() {
return {
letters: function($q) {
return $q(function(resolve) {
window.setTimeout(resolve, 1000, ['a', 'b', 'c', 'd', 'e']);
});
}
};
}

function resolveRedirectTo(path) {
return {
resolveRedirectTo: function($q) {
return $q(function(resolve) {
window.setTimeout(resolve, 250, path);
});
}
};
}
});
33 changes: 33 additions & 0 deletions test/e2e/tests/ng-route-promise.spec.js
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,33 @@
'use strict';

describe('ngRoute promises', function() {
beforeEach(function() {
loadFixture('ng-route-promise');
});

it('should wait for route promises', function() {
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
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

😆

browser.manage().timeouts().setScriptTimeout(1500);
browser.waitForAngular().then(function() {
fail('waitForAngular() should have timed out, but didn\'t');
}, function(error) {
expect(error.message).toContain('Timed out waiting for asynchronous Angular tasks to finish');
});
});

it('should wait for route promises when navigating to another route', function() {
browser.setLocation('/foo2');
expect(element(by.tagName('body')).getText()).toBe('5');
});

afterEach(function(done) {
// Restore old timeout limit
browser.getProcessedConfig().then(function(config) {
return browser.manage().timeouts().setScriptTimeout(config.allScriptsTimeout);
}).then(done);
});
});
184 changes: 184 additions & 0 deletions test/ngRoute/routeSpec.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2082,4 +2082,188 @@ describe('$route', function() {
expect(function() { $route.updateParams(); }).toThrowMinErr('ngRoute', 'norout');
}));
});

describe('testability', function() {
it('should wait for $resolve promises before calling callbacks', function() {
var deferred;

module(function($provide, $routeProvider) {
$routeProvider.when('/path', {
template: '',
resolve: {
a: function($q) {
deferred = $q.defer();
return deferred.promise;
}
}
});
});

inject(function($location, $route, $rootScope, $httpBackend, $$testability) {
$location.path('/path');
$rootScope.$digest();

var callback = jasmine.createSpy('callback');
$$testability.whenStable(callback);
expect(callback).not.toHaveBeenCalled();

deferred.resolve();
$rootScope.$digest();
expect(callback).toHaveBeenCalled();
});
});

it('should call callback after $resolve promises are rejected', function() {
var deferred;

module(function($provide, $routeProvider) {
$routeProvider.when('/path', {
template: '',
resolve: {
a: function($q) {
deferred = $q.defer();
return deferred.promise;
}
}
});
});

inject(function($location, $route, $rootScope, $httpBackend, $$testability) {
$location.path('/path');
$rootScope.$digest();

var callback = jasmine.createSpy('callback');
$$testability.whenStable(callback);
expect(callback).not.toHaveBeenCalled();

deferred.reject();
$rootScope.$digest();
expect(callback).toHaveBeenCalled();
});
});

it('should wait for resolveRedirectTo promises before calling callbacks', function() {
var deferred;

module(function($provide, $routeProvider) {
$routeProvider.when('/path', {
resolveRedirectTo: function($q) {
deferred = $q.defer();
return deferred.promise;
}
});
});

inject(function($location, $route, $rootScope, $httpBackend, $$testability) {
$location.path('/path');
$rootScope.$digest();

var callback = jasmine.createSpy('callback');
$$testability.whenStable(callback);
expect(callback).not.toHaveBeenCalled();

deferred.resolve();
$rootScope.$digest();
expect(callback).toHaveBeenCalled();
});
});

it('should call callback after resolveRedirectTo promises are rejected', function() {
var deferred;

module(function($provide, $routeProvider) {
$routeProvider.when('/path', {
resolveRedirectTo: function($q) {
deferred = $q.defer();
return deferred.promise;
}
});
});

inject(function($location, $route, $rootScope, $httpBackend, $$testability) {
$location.path('/path');
$rootScope.$digest();

var callback = jasmine.createSpy('callback');
$$testability.whenStable(callback);
expect(callback).not.toHaveBeenCalled();

deferred.reject();
$rootScope.$digest();
expect(callback).toHaveBeenCalled();
});
});

it('should wait for all route promises before calling callbacks', function() {
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@gkalpak This is the unit test which I wrote to test for the race condition. This unit tests fails, but this doesn't appear to be a problem with real browsers. I did essentially the same thing in a real browser, and it correctly called the callback only after both promises were resolved. I'll look into the error in the unit testing setup after lunch

var deferreds = {};

module(function($provide, $routeProvider) {
// While normally `$browser.defer()` modifies the `outstandingRequestCount`, the mocked
// version (provided by `ngMock`) does not. This doesn't matter in most tests, but in this
// case we need the `outstandingRequestCount` logic to ensure that we don't call the
// `$$testability.whenStable()` callbacks part way through a `$rootScope.$evalAsync` block.
// See ngRoute's commitRoute()'s finally() block for details.
$provide.decorator('$browser', function($delegate) {
var oldDefer = $delegate.defer;
var newDefer = function(fn, delay) {
var requestCountAwareFn = function() { $delegate.$$completeOutstandingRequest(fn); };
$delegate.$$incOutstandingRequestCount();
return oldDefer.call($delegate, requestCountAwareFn, delay);
};

$delegate.defer = angular.extend(newDefer, oldDefer);

return $delegate;
});

addRouteWithAsyncRedirect('/foo', '/bar');
addRouteWithAsyncRedirect('/bar', '/baz');
addRouteWithAsyncRedirect('/baz', '/qux');
$routeProvider.when('/qux', {
template: '',
resolve: {
a: function($q) {
var deferred = deferreds['/qux'] = $q.defer();
return deferred.promise;
}
}
});

// Helpers
function addRouteWithAsyncRedirect(fromPath, toPath) {
$routeProvider.when(fromPath, {
resolveRedirectTo: function($q) {
var deferred = deferreds[fromPath] = $q.defer();
return deferred.promise.then(function() { return toPath; });
}
});
}
});

inject(function($browser, $location, $rootScope, $route, $$testability) {
$location.path('/foo');
$rootScope.$digest();

var callback = jasmine.createSpy('callback');
$$testability.whenStable(callback);
expect(callback).not.toHaveBeenCalled();

deferreds['/foo'].resolve();
$browser.defer.flush();
expect(callback).not.toHaveBeenCalled();

deferreds['/bar'].resolve();
$browser.defer.flush();
expect(callback).not.toHaveBeenCalled();

deferreds['/baz'].resolve();
$browser.defer.flush();
expect(callback).not.toHaveBeenCalled();

deferreds['/qux'].resolve();
$browser.defer.flush();
expect(callback).toHaveBeenCalled();
});
});
});
});