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

Commit e13eeab

Browse files
gkalpakgkalpak
committed
fix($q): treat thrown errors as regular rejections
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. Fixes #3174 Fixes #14745 Closes #15213 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.
1 parent 823295f commit e13eeab

File tree

9 files changed

+180
-203
lines changed

9 files changed

+180
-203
lines changed

Diff for: ‎lib/promises-aplus/promises-aplus-test-adapter.js

+11-18
Original file line numberDiff line numberDiff line change
@@ -9,21 +9,21 @@
99
minErr,
1010
extend
1111
*/
12-
/* eslint-disable no-unused-vars */
1312

14-
var isFunction = function isFunction(value) {return typeof value === 'function';};
15-
var isPromiseLike = function isPromiseLike(obj) {return obj && isFunction(obj.then);};
16-
var isObject = function isObject(value) {return value != null && typeof value === 'object';};
17-
var isUndefined = function isUndefined(value) {return typeof value === 'undefined';};
13+
/* eslint-disable no-unused-vars */
14+
function isFunction(value) { return typeof value === 'function'; }
15+
function isPromiseLike(obj) { return obj && isFunction(obj.then); }
16+
function isObject(value) { return value !== null && typeof value === 'object'; }
17+
function isUndefined(value) { return typeof value === 'undefined'; }
1818

19-
var minErr = function minErr(module, constructor) {
19+
function minErr(module, constructor) {
2020
return function() {
2121
var ErrorConstructor = constructor || Error;
2222
throw new ErrorConstructor(module + arguments[0] + arguments[1]);
2323
};
24-
};
24+
}
2525

26-
var extend = function extend(dst) {
26+
function extend(dst) {
2727
for (var i = 1, ii = arguments.length; i < ii; i++) {
2828
var obj = arguments[i];
2929
if (obj) {
@@ -35,18 +35,11 @@ var extend = function extend(dst) {
3535
}
3636
}
3737
return dst;
38-
};
38+
}
39+
/* eslint-enable */
3940

4041
var $q = qFactory(process.nextTick, function noopExceptionHandler() {});
4142

4243
exports.resolved = $q.resolve;
4344
exports.rejected = $q.reject;
44-
exports.deferred = function() {
45-
var deferred = $q.defer();
46-
47-
return {
48-
promise: deferred.promise,
49-
resolve: deferred.resolve,
50-
reject: deferred.reject
51-
};
52-
};
45+
exports.deferred = $q.defer;

Diff for: ‎src/ng/compile.js

+4
Original file line numberDiff line numberDiff line change
@@ -3131,6 +3131,10 @@ function $CompileProvider($provide, $$sanitizeUriProvider) {
31313131
childBoundTranscludeFn);
31323132
}
31333133
linkQueue = null;
3134+
}).catch(function(error) {
3135+
if (error instanceof Error) {
3136+
$exceptionHandler(error);
3137+
}
31343138
}).catch(noop);
31353139

31363140
return function delayedNodeLinkFn(ignoreChildLinkFn, scope, node, rootElement, boundTranscludeFn) {

Diff for: ‎src/ng/q.js

+3-6
Original file line numberDiff line numberDiff line change
@@ -9,8 +9,8 @@
99
* A service that helps you run functions asynchronously, and use their return values (or exceptions)
1010
* when they are done processing.
1111
*
12-
* This is an implementation of promises/deferred objects inspired by
13-
* [Kris Kowal's Q](https://github.com/kriskowal/q).
12+
* This is a [Promises/A+](https://promisesaplus.com/)-compliant implementation of promises/deferred
13+
* objects inspired by [Kris Kowal's Q](https://github.com/kriskowal/q).
1414
*
1515
* $q can be used in two fashions --- one which is more similar to Kris Kowal's Q or jQuery's Deferred
1616
* implementations, and the other which resembles ES6 (ES2015) promises to some degree.
@@ -366,7 +366,6 @@ function qFactory(nextTick, exceptionHandler, errorOnUnhandledRejections) {
366366
}
367367
} catch (e) {
368368
deferred.reject(e);
369-
exceptionHandler(e);
370369
}
371370
}
372371
} finally {
@@ -417,15 +416,14 @@ function qFactory(nextTick, exceptionHandler, errorOnUnhandledRejections) {
417416
} else {
418417
this.$$resolve(val);
419418
}
420-
421419
},
422420

423421
$$resolve: function(val) {
424422
var then;
425423
var that = this;
426424
var done = false;
427425
try {
428-
if ((isObject(val) || isFunction(val))) then = val && val.then;
426+
if (isObject(val) || isFunction(val)) then = val.then;
429427
if (isFunction(then)) {
430428
this.promise.$$state.status = -1;
431429
then.call(val, resolvePromise, rejectPromise, simpleBind(this, this.notify));
@@ -436,7 +434,6 @@ function qFactory(nextTick, exceptionHandler, errorOnUnhandledRejections) {
436434
}
437435
} catch (e) {
438436
rejectPromise(e);
439-
exceptionHandler(e);
440437
}
441438

442439
function resolvePromise(val) {

Diff for: ‎src/ng/templateRequest.js

+45-39
Original file line numberDiff line numberDiff line change
@@ -60,53 +60,59 @@ function $TemplateRequestProvider() {
6060
*
6161
* @property {number} totalPendingRequests total amount of pending template requests being downloaded.
6262
*/
63-
this.$get = ['$templateCache', '$http', '$q', '$sce', function($templateCache, $http, $q, $sce) {
63+
this.$get = ['$exceptionHandler', '$templateCache', '$http', '$q', '$sce',
64+
function($exceptionHandler, $templateCache, $http, $q, $sce) {
6465

65-
function handleRequestFn(tpl, ignoreRequestError) {
66-
handleRequestFn.totalPendingRequests++;
66+
function handleRequestFn(tpl, ignoreRequestError) {
67+
handleRequestFn.totalPendingRequests++;
6768

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

77-
var transformResponse = $http.defaults && $http.defaults.transformResponse;
78+
var transformResponse = $http.defaults && $http.defaults.transformResponse;
7879

79-
if (isArray(transformResponse)) {
80-
transformResponse = transformResponse.filter(function(transformer) {
81-
return transformer !== defaultHttpResponseTransform;
82-
});
83-
} else if (transformResponse === defaultHttpResponseTransform) {
84-
transformResponse = null;
85-
}
80+
if (isArray(transformResponse)) {
81+
transformResponse = transformResponse.filter(function(transformer) {
82+
return transformer !== defaultHttpResponseTransform;
83+
});
84+
} else if (transformResponse === defaultHttpResponseTransform) {
85+
transformResponse = null;
86+
}
8687

87-
return $http.get(tpl, extend({
88-
cache: $templateCache,
89-
transformResponse: transformResponse
90-
}, httpOptions))
91-
.finally(function() {
92-
handleRequestFn.totalPendingRequests--;
93-
})
94-
.then(function(response) {
95-
$templateCache.put(tpl, response.data);
96-
return response.data;
97-
}, handleError);
88+
return $http.get(tpl, extend({
89+
cache: $templateCache,
90+
transformResponse: transformResponse
91+
}, httpOptions))
92+
.finally(function() {
93+
handleRequestFn.totalPendingRequests--;
94+
})
95+
.then(function(response) {
96+
$templateCache.put(tpl, response.data);
97+
return response.data;
98+
}, handleError);
9899

99-
function handleError(resp) {
100-
if (!ignoreRequestError) {
101-
throw $templateRequestMinErr('tpload', 'Failed to load template: {0} (HTTP status: {1} {2})',
102-
tpl, resp.status, resp.statusText);
100+
function handleError(resp) {
101+
if (!ignoreRequestError) {
102+
resp = $templateRequestMinErr('tpload',
103+
'Failed to load template: {0} (HTTP status: {1} {2})',
104+
tpl, resp.status, resp.statusText);
105+
106+
$exceptionHandler(resp);
107+
}
108+
109+
return $q.reject(resp);
103110
}
104-
return $q.reject(resp);
105111
}
106-
}
107112

108-
handleRequestFn.totalPendingRequests = 0;
113+
handleRequestFn.totalPendingRequests = 0;
109114

110-
return handleRequestFn;
111-
}];
115+
return handleRequestFn;
116+
}
117+
];
112118
}

Diff for: ‎test/ng/compileSpec.js

+42-34
Original file line numberDiff line numberDiff line change
@@ -1854,17 +1854,18 @@ describe('$compile', function() {
18541854
));
18551855

18561856

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

1862-
expect(function() {
1863-
$httpBackend.flush();
1864-
}).toThrowMinErr('$compile', 'tpload', 'Failed to load template: hello.html');
1865-
expect(sortedHtml(element)).toBe('<div><b class="hello"></b></div>');
1866-
}
1867-
));
1862+
$httpBackend.flush();
1863+
1864+
expect(sortedHtml(element)).toBe('<div><b class="hello"></b></div>');
1865+
expect($exceptionHandler.errors[0].message).toMatch(
1866+
/^\[\$compile:tpload] Failed to load template: hello\.html/);
1867+
})
1868+
);
18681869

18691870

18701871
it('should prevent multiple templates per element', function() {
@@ -1878,13 +1879,15 @@ describe('$compile', function() {
18781879
templateUrl: 'template.html'
18791880
}));
18801881
});
1881-
inject(function($compile, $httpBackend) {
1882+
inject(function($compile, $exceptionHandler, $httpBackend) {
18821883
$httpBackend.whenGET('template.html').respond('<p>template.html</p>');
1883-
expect(function() {
1884-
$compile('<div><div class="sync async"></div></div>');
1885-
$httpBackend.flush();
1886-
}).toThrowMinErr('$compile', 'multidir', 'Multiple directives [async, sync] asking for template on: ' +
1887-
'<div class="sync async">');
1884+
1885+
$compile('<div><div class="sync async"></div></div>');
1886+
$httpBackend.flush();
1887+
1888+
expect($exceptionHandler.errors[0].message).toMatch(new RegExp(
1889+
'^\\[\\$compile:multidir] Multiple directives \\[async, sync] asking for ' +
1890+
'template on: <div class="sync async">'));
18881891
});
18891892
});
18901893

@@ -2667,14 +2670,15 @@ describe('$compile', function() {
26672670
);
26682671

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

2673-
expect(function() {
2674-
compile('<div class="tiscope-a; scope-b"></div>');
2675-
$httpBackend.flush();
2676-
}).toThrowMinErr('$compile', 'multidir', 'Multiple directives [scopeB, tiscopeA] ' +
2677-
'asking for new/isolated scope on: <div class="tiscope-a; scope-b ng-scope">');
2676+
compile('<div class="tiscope-a; scope-b"></div>');
2677+
$httpBackend.flush();
2678+
2679+
expect($exceptionHandler.errors[0].message).toMatch(new RegExp(
2680+
'^\\[\\$compile:multidir] Multiple directives \\[scopeB, tiscopeA] ' +
2681+
'asking for new/isolated scope on: <div class="tiscope-a; scope-b ng-scope">'));
26782682
})
26792683
);
26802684

@@ -8875,28 +8879,29 @@ describe('$compile', function() {
88758879
'<div class="foo" ng-transclude></div>' +
88768880
'</div>',
88778881
transclude: true
8878-
88798882
}));
88808883

88818884
$compileProvider.directive('noTransBar', valueFn({
88828885
templateUrl: 'noTransBar.html',
88838886
transclude: false
8884-
88858887
}));
88868888
});
88878889

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

8895-
expect(function() {
8896-
element = $compile('<div trans-foo>content</div>')($rootScope);
8897-
$rootScope.$apply();
8898-
}).toThrowMinErr('ngTransclude', 'orphan',
8899-
'Illegal use of ngTransclude directive in the template! No parent directive that requires a transclusion found. Element: <div class="bar" ng-transclude="">');
8897+
element = $compile('<div trans-foo>content</div>')($rootScope);
8898+
$rootScope.$digest();
8899+
8900+
expect($exceptionHandler.errors[0][1]).toBe('<div class="bar" ng-transclude="">');
8901+
expect($exceptionHandler.errors[0][0].message).toMatch(new RegExp(
8902+
'^\\[ngTransclude:orphan] Illegal use of ngTransclude directive in the ' +
8903+
'template! No parent directive that requires a transclusion found. Element: ' +
8904+
'<div class="bar" ng-transclude="">'));
89008905
});
89018906
});
89028907

@@ -9706,12 +9711,15 @@ describe('$compile', function() {
97069711
transclude: 'element'
97079712
}));
97089713
});
9709-
inject(function($compile, $httpBackend) {
9714+
inject(function($compile, $exceptionHandler, $httpBackend) {
97109715
$httpBackend.expectGET('template.html').respond('<p second>template.html</p>');
9716+
97119717
$compile('<div template first></div>');
9712-
expect(function() {
9713-
$httpBackend.flush();
9714-
}).toThrowMinErr('$compile', 'multidir', /Multiple directives \[first, second\] asking for transclusion on: <p .+/);
9718+
$httpBackend.flush();
9719+
9720+
expect($exceptionHandler.errors[0].message).toMatch(new RegExp(
9721+
'^\\[\\$compile:multidir] Multiple directives \\[first, second] asking for ' +
9722+
'transclusion on: <p '));
97159723
});
97169724
});
97179725

Diff for: ‎test/ng/httpSpec.js

+4-10
Original file line numberDiff line numberDiff line change
@@ -1941,7 +1941,7 @@ describe('$http', function() {
19411941

19421942

19431943
it('should increment/decrement `outstandingRequestCount` on error in `transformRequest`',
1944-
inject(function($exceptionHandler) {
1944+
function() {
19451945
expect(incOutstandingRequestCountSpy).not.toHaveBeenCalled();
19461946
expect(completeOutstandingRequestSpy).not.toHaveBeenCalled();
19471947

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

19551955
expect(incOutstandingRequestCountSpy).toHaveBeenCalledOnce();
19561956
expect(completeOutstandingRequestSpy).toHaveBeenCalledOnce();
1957-
1958-
expect($exceptionHandler.errors).toEqual([jasmine.any(Error)]);
1959-
$exceptionHandler.errors = [];
1960-
})
1957+
}
19611958
);
19621959

19631960

19641961
it('should increment/decrement `outstandingRequestCount` on error in `transformResponse`',
1965-
inject(function($exceptionHandler) {
1962+
function() {
19661963
expect(incOutstandingRequestCountSpy).not.toHaveBeenCalled();
19671964
expect(completeOutstandingRequestSpy).not.toHaveBeenCalled();
19681965

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

19771974
expect(incOutstandingRequestCountSpy).toHaveBeenCalledOnce();
19781975
expect(completeOutstandingRequestSpy).toHaveBeenCalledOnce();
1979-
1980-
expect($exceptionHandler.errors).toEqual([jasmine.any(Error)]);
1981-
$exceptionHandler.errors = [];
1982-
})
1976+
}
19831977
);
19841978
});
19851979

0 commit comments

Comments
 (0)