Skip to content

Commit e3b3815

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. 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
1 parent cf241c4 commit e3b3815

File tree

8 files changed

+143
-171
lines changed

8 files changed

+143
-171
lines changed

‎src/ng/compile.js

+9-3
Original file line numberDiff line numberDiff line change
@@ -3052,8 +3052,8 @@ function $CompileProvider($provide, $$sanitizeUriProvider) {
30523052

30533053
$compileNode.empty();
30543054

3055-
$templateRequest(templateUrl)
3056-
.then(function(content) {
3055+
$templateRequest(templateUrl).
3056+
then(function(content) {
30573057
var compileNode, tempTemplateAttrs, $template, childBoundTranscludeFn;
30583058

30593059
content = denormalizeTemplate(content);
@@ -3131,7 +3131,13 @@ function $CompileProvider($provide, $$sanitizeUriProvider) {
31313131
childBoundTranscludeFn);
31323132
}
31333133
linkQueue = null;
3134-
}).catch(noop);
3134+
}).
3135+
catch(function(error) {
3136+
if (error instanceof Error) {
3137+
$exceptionHandler(error);
3138+
}
3139+
}).
3140+
catch(noop);
31353141

31363142
return function delayedNodeLinkFn(ignoreChildLinkFn, scope, node, rootElement, boundTranscludeFn) {
31373143
var childBoundTranscludeFn = boundTranscludeFn;

‎src/ng/q.js

+4-9
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,26 +416,22 @@ function qFactory(nextTick, exceptionHandler, errorOnUnhandledRejections) {
417416
} else {
418417
this.$$resolve(val);
419418
}
420-
421419
},
422420

423421
$$resolve: function(val) {
424-
var then;
425422
var that = this;
426423
var done = false;
427424
try {
428-
if ((isObject(val) || isFunction(val))) then = val && val.then;
429-
if (isFunction(then)) {
425+
if ((isObject(val) || isFunction(val)) && isFunction(val.then)) {
430426
this.promise.$$state.status = -1;
431-
then.call(val, resolvePromise, rejectPromise, simpleBind(this, this.notify));
427+
val.then(resolvePromise, rejectPromise, simpleBind(this, this.notify));
432428
} else {
433429
this.promise.$$state.value = val;
434430
this.promise.$$state.status = 1;
435431
scheduleProcessQueue(this.promise.$$state);
436432
}
437433
} catch (e) {
438434
rejectPromise(e);
439-
exceptionHandler(e);
440435
}
441436

442437
function resolvePromise(val) {

‎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
}

‎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

‎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)