Skip to content

Commit

Permalink
fix($q): treat thrown errors as regular rejections
Browse files Browse the repository at this point in the history 
Previously, errors thrown in a promise's `onFulfilled` or `onRejected` handlers were treated in a
slightly different manner than regular rejections:
They were passed to the `$exceptionHandler()` (in addition to being converted to rejections).

The reasoning for this behavior was that an uncaught error is different than a regular rejection, as
it can be caused by a programming error, for example. In practice, this turned out to be confusing
or undesirable for users, since neither native promises nor any other popular promise library
distinguishes thrown errors from regular rejections.
(Note: While this behavior does not go against the Promises/A+ spec, it is not prescribed either.)

This commit removes the distinction, by skipping the call to `$exceptionHandler()`, thus treating
thrown errors as regular rejections.

**Note:**
Unless explicitly turned off, possibly unhandled rejections will still be caught and passed to the
`$exceptionHandler()`, so errors thrown due to programming errors and not otherwise handled (with a
subsequent `onRejected` handler) will not go unnoticed.

BREAKING CHANGE:

Previously, throwing an error from a promise's `onFulfilled` or `onRejection` handlers, would result
in passing the error to the `$exceptionHandler()` (in addition to rejecting the promise with the
error as reason).

Now, a thrown error is treated exactly the same as a regular rejection. This applies to all
services/controllers/filters etc that rely on `$q` (including built-in services, such as `$http` and
`$route`). For example, `$http`'s `transformRequest/Response` functions or a route's `redirectTo`
function as well as functions specified in a route's `resolve` object, will no longer result in a
call to `$exceptionHandler()` if they throw an error. Other than that, everything will continue to
behave in the same way; i.e. the promises will be rejected, route transition will be cancelled,
`$routeChangeError` events will be broadcasted etc.

Fixes angular#3174
Fixes angular#14745
  • Loading branch information
@gkalpak
gkalpak committed Oct 3, 2016
1 parent cf241c4 commit e3b3815
Show file tree
Hide file tree
Showing 8 changed files with 143 additions and 171 deletions.
12 changes: 9 additions & 3 deletions src/ng/compile.js
View file
Original file line number Diff line number Diff line change
Expand Up @@ -3052,8 +3052,8 @@ function $CompileProvider($provide, $$sanitizeUriProvider) {

$compileNode.empty();

$templateRequest(templateUrl)
.then(function(content) {
$templateRequest(templateUrl).
then(function(content) {
var compileNode, tempTemplateAttrs, $template, childBoundTranscludeFn;

content = denormalizeTemplate(content);
Expand Down Expand Up @@ -3131,7 +3131,13 @@ function $CompileProvider($provide, $$sanitizeUriProvider) {
childBoundTranscludeFn);
}
linkQueue = null;
}).catch(noop);
}).
catch(function(error) {
if (error instanceof Error) {
$exceptionHandler(error);
}
}).
catch(noop);

return function delayedNodeLinkFn(ignoreChildLinkFn, scope, node, rootElement, boundTranscludeFn) {
var childBoundTranscludeFn = boundTranscludeFn;
Expand Down
13 changes: 4 additions & 9 deletions src/ng/q.js
View file
Original file line number Diff line number Diff line change
Expand Up @@ -9,8 +9,8 @@
* A service that helps you run functions asynchronously, and use their return values (or exceptions)
* when they are done processing.
*
* This is an implementation of promises/deferred objects inspired by
* [Kris Kowal's Q](https://github.com/kriskowal/q).
* This is a [Promises/A+](https://promisesaplus.com/)-compliant implementation of promises/deferred
* objects inspired by [Kris Kowal's Q](https://github.com/kriskowal/q).
*
* $q can be used in two fashions --- one which is more similar to Kris Kowal's Q or jQuery's Deferred
* implementations, and the other which resembles ES6 (ES2015) promises to some degree.
Expand Down Expand Up @@ -366,7 +366,6 @@ function qFactory(nextTick, exceptionHandler, errorOnUnhandledRejections) {
}
} catch (e) {
deferred.reject(e);
exceptionHandler(e);
}
}
} finally {
Expand Down Expand Up @@ -417,26 +416,22 @@ function qFactory(nextTick, exceptionHandler, errorOnUnhandledRejections) {
} else {
this.$$resolve(val);
}

},

$$resolve: function(val) {
var then;
var that = this;
var done = false;
try {
if ((isObject(val) || isFunction(val))) then = val && val.then;
if (isFunction(then)) {
if ((isObject(val) || isFunction(val)) && isFunction(val.then)) {
this.promise.$$state.status = -1;
then.call(val, resolvePromise, rejectPromise, simpleBind(this, this.notify));
val.then(resolvePromise, rejectPromise, simpleBind(this, this.notify));
} else {
this.promise.$$state.value = val;
this.promise.$$state.status = 1;
scheduleProcessQueue(this.promise.$$state);
}
} catch (e) {
rejectPromise(e);
exceptionHandler(e);
}

function resolvePromise(val) {
Expand Down
84 changes: 45 additions & 39 deletions src/ng/templateRequest.js
View file
Original file line number Diff line number Diff line change
Expand Up @@ -60,53 +60,59 @@ function $TemplateRequestProvider() {
*
* @property {number} totalPendingRequests total amount of pending template requests being downloaded.
*/
this.$get = ['$templateCache', '$http', '$q', '$sce', function($templateCache, $http, $q, $sce) {
this.$get = ['$exceptionHandler', '$templateCache', '$http', '$q', '$sce',
function($exceptionHandler, $templateCache, $http, $q, $sce) {

function handleRequestFn(tpl, ignoreRequestError) {
handleRequestFn.totalPendingRequests++;
function handleRequestFn(tpl, ignoreRequestError) {
handleRequestFn.totalPendingRequests++;

// We consider the template cache holds only trusted templates, so
// there's no need to go through whitelisting again for keys that already
// are included in there. This also makes Angular accept any script
// directive, no matter its name. However, we still need to unwrap trusted
// types.
if (!isString(tpl) || isUndefined($templateCache.get(tpl))) {
tpl = $sce.getTrustedResourceUrl(tpl);
}
// We consider the template cache holds only trusted templates, so
// there's no need to go through whitelisting again for keys that already
// are included in there. This also makes Angular accept any script
// directive, no matter its name. However, we still need to unwrap trusted
// types.
if (!isString(tpl) || isUndefined($templateCache.get(tpl))) {
tpl = $sce.getTrustedResourceUrl(tpl);
}

var transformResponse = $http.defaults && $http.defaults.transformResponse;
var transformResponse = $http.defaults && $http.defaults.transformResponse;

if (isArray(transformResponse)) {
transformResponse = transformResponse.filter(function(transformer) {
return transformer !== defaultHttpResponseTransform;
});
} else if (transformResponse === defaultHttpResponseTransform) {
transformResponse = null;
}
if (isArray(transformResponse)) {
transformResponse = transformResponse.filter(function(transformer) {
return transformer !== defaultHttpResponseTransform;
});
} else if (transformResponse === defaultHttpResponseTransform) {
transformResponse = null;
}

return $http.get(tpl, extend({
cache: $templateCache,
transformResponse: transformResponse
}, httpOptions))
.finally(function() {
handleRequestFn.totalPendingRequests--;
})
.then(function(response) {
$templateCache.put(tpl, response.data);
return response.data;
}, handleError);
return $http.get(tpl, extend({
cache: $templateCache,
transformResponse: transformResponse
}, httpOptions))
.finally(function() {
handleRequestFn.totalPendingRequests--;
})
.then(function(response) {
$templateCache.put(tpl, response.data);
return response.data;
}, handleError);

function handleError(resp) {
if (!ignoreRequestError) {
throw $templateRequestMinErr('tpload', 'Failed to load template: {0} (HTTP status: {1} {2})',
tpl, resp.status, resp.statusText);
function handleError(resp) {
if (!ignoreRequestError) {
resp = $templateRequestMinErr('tpload',
'Failed to load template: {0} (HTTP status: {1} {2})',
tpl, resp.status, resp.statusText);

$exceptionHandler(resp);
}

return $q.reject(resp);
}
return $q.reject(resp);
}
}

handleRequestFn.totalPendingRequests = 0;
handleRequestFn.totalPendingRequests = 0;

return handleRequestFn;
}];
return handleRequestFn;
}
];
}
76 changes: 42 additions & 34 deletions test/ng/compileSpec.js
View file
Original file line number Diff line number Diff line change
Expand Up @@ -1854,17 +1854,18 @@ describe('$compile', function() {
));


it('should throw an error and clear element content if the template fails to load', inject(
function($compile, $httpBackend, $rootScope) {
$httpBackend.expect('GET', 'hello.html').respond(404, 'Not Found!');
element = $compile('<div><b class="hello">content</b></div>')($rootScope);
it('should throw an error and clear element content if the template fails to load',
inject(function($compile, $exceptionHandler, $httpBackend, $rootScope) {
$httpBackend.expect('GET', 'hello.html').respond(404, 'Not Found!');
element = $compile('<div><b class="hello">content</b></div>')($rootScope);

expect(function() {
$httpBackend.flush();
}).toThrowMinErr('$compile', 'tpload', 'Failed to load template: hello.html');
expect(sortedHtml(element)).toBe('<div><b class="hello"></b></div>');
}
));
$httpBackend.flush();

expect(sortedHtml(element)).toBe('<div><b class="hello"></b></div>');
expect($exceptionHandler.errors[0].message).toMatch(
/^\[\$compile:tpload] Failed to load template: hello\.html/);
})
);


it('should prevent multiple templates per element', function() {
Expand All @@ -1878,13 +1879,15 @@ describe('$compile', function() {
templateUrl: 'template.html'
}));
});
inject(function($compile, $httpBackend) {
inject(function($compile, $exceptionHandler, $httpBackend) {
$httpBackend.whenGET('template.html').respond('<p>template.html</p>');
expect(function() {
$compile('<div><div class="sync async"></div></div>');
$httpBackend.flush();
}).toThrowMinErr('$compile', 'multidir', 'Multiple directives [async, sync] asking for template on: ' +
'<div class="sync async">');

$compile('<div><div class="sync async"></div></div>');
$httpBackend.flush();

expect($exceptionHandler.errors[0].message).toMatch(new RegExp(
'^\\[\\$compile:multidir] Multiple directives \\[async, sync] asking for ' +
'template on: <div class="sync async">'));
});
});

Expand Down Expand Up @@ -2667,14 +2670,15 @@ describe('$compile', function() {
);

it('should not allow more than one isolate/new scope creation per element regardless of `templateUrl`',
inject(function($httpBackend) {
inject(function($exceptionHandler, $httpBackend) {
$httpBackend.expect('GET', 'tiscope.html').respond('<div>Hello, world !</div>');

expect(function() {
compile('<div class="tiscope-a; scope-b"></div>');
$httpBackend.flush();
}).toThrowMinErr('$compile', 'multidir', 'Multiple directives [scopeB, tiscopeA] ' +
'asking for new/isolated scope on: <div class="tiscope-a; scope-b ng-scope">');
compile('<div class="tiscope-a; scope-b"></div>');
$httpBackend.flush();

expect($exceptionHandler.errors[0].message).toMatch(new RegExp(
'^\\[\\$compile:multidir] Multiple directives \\[scopeB, tiscopeA] ' +
'asking for new/isolated scope on: <div class="tiscope-a; scope-b ng-scope">'));
})
);

Expand Down Expand Up @@ -8875,28 +8879,29 @@ describe('$compile', function() {
'<div class="foo" ng-transclude></div>' +
'</div>',
transclude: true

}));

$compileProvider.directive('noTransBar', valueFn({
templateUrl: 'noTransBar.html',
transclude: false

}));
});

inject(function($compile, $rootScope, $templateCache) {
inject(function($compile, $exceptionHandler, $rootScope, $templateCache) {
$templateCache.put('noTransBar.html',
'<div>' +
// This ng-transclude is invalid. It should throw an error.
'<div class="bar" ng-transclude></div>' +
'</div>');

expect(function() {
element = $compile('<div trans-foo>content</div>')($rootScope);
$rootScope.$apply();
}).toThrowMinErr('ngTransclude', 'orphan',
'Illegal use of ngTransclude directive in the template! No parent directive that requires a transclusion found. Element: <div class="bar" ng-transclude="">');
element = $compile('<div trans-foo>content</div>')($rootScope);
$rootScope.$digest();

expect($exceptionHandler.errors[0][1]).toBe('<div class="bar" ng-transclude="">');
expect($exceptionHandler.errors[0][0].message).toMatch(new RegExp(
'^\\[ngTransclude:orphan] Illegal use of ngTransclude directive in the ' +
'template! No parent directive that requires a transclusion found. Element: ' +
'<div class="bar" ng-transclude="">'));
});
});

Expand Down Expand Up @@ -9706,12 +9711,15 @@ describe('$compile', function() {
transclude: 'element'
}));
});
inject(function($compile, $httpBackend) {
inject(function($compile, $exceptionHandler, $httpBackend) {
$httpBackend.expectGET('template.html').respond('<p second>template.html</p>');

$compile('<div template first></div>');
expect(function() {
$httpBackend.flush();
}).toThrowMinErr('$compile', 'multidir', /Multiple directives \[first, second\] asking for transclusion on: <p .+/);
$httpBackend.flush();

expect($exceptionHandler.errors[0].message).toMatch(new RegExp(
'^\\[\\$compile:multidir] Multiple directives \\[first, second] asking for ' +
'transclusion on: <p '));
});
});

Expand Down
14 changes: 4 additions & 10 deletions test/ng/httpSpec.js
View file
Original file line number Diff line number Diff line change
Expand Up @@ -1941,7 +1941,7 @@ describe('$http', function() {


it('should increment/decrement `outstandingRequestCount` on error in `transformRequest`',
inject(function($exceptionHandler) {
function() {
expect(incOutstandingRequestCountSpy).not.toHaveBeenCalled();
expect(completeOutstandingRequestSpy).not.toHaveBeenCalled();

Expand All @@ -1954,15 +1954,12 @@ describe('$http', function() {

expect(incOutstandingRequestCountSpy).toHaveBeenCalledOnce();
expect(completeOutstandingRequestSpy).toHaveBeenCalledOnce();

expect($exceptionHandler.errors).toEqual([jasmine.any(Error)]);
$exceptionHandler.errors = [];
})
}
);


it('should increment/decrement `outstandingRequestCount` on error in `transformResponse`',
inject(function($exceptionHandler) {
function() {
expect(incOutstandingRequestCountSpy).not.toHaveBeenCalled();
expect(completeOutstandingRequestSpy).not.toHaveBeenCalled();

Expand All @@ -1976,10 +1973,7 @@ describe('$http', function() {

expect(incOutstandingRequestCountSpy).toHaveBeenCalledOnce();
expect(completeOutstandingRequestSpy).toHaveBeenCalledOnce();

expect($exceptionHandler.errors).toEqual([jasmine.any(Error)]);
$exceptionHandler.errors = [];
})
}
);
});

Expand Down
Loading

0 comments on commit e3b3815

Please sign in to comment.