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

fix($q): treat thrown errors as regular rejections #15213

Closed
Show file tree
Hide file tree
Changes from 2 commits
Commits
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
29 changes: 11 additions & 18 deletions lib/promises-aplus/promises-aplus-test-adapter.js
Original file line number Diff line number Diff line change
Expand Up @@ -9,21 +9,21 @@
minErr,
extend
*/
/* eslint-disable no-unused-vars */

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

var minErr = function minErr(module, constructor) {
function minErr(module, constructor) {
return function() {
var ErrorConstructor = constructor || Error;
throw new ErrorConstructor(module + arguments[0] + arguments[1]);
};
};
}

var extend = function extend(dst) {
function extend(dst) {
for (var i = 1, ii = arguments.length; i < ii; i++) {
var obj = arguments[i];
if (obj) {
Expand All @@ -35,18 +35,11 @@ var extend = function extend(dst) {
}
}
return dst;
};
}
/* eslint-enable */

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

exports.resolved = $q.resolve;
exports.rejected = $q.reject;
exports.deferred = function() {
var deferred = $q.defer();

return {
promise: deferred.promise,
resolve: deferred.resolve,
reject: deferred.reject
};
};
exports.deferred = $q.defer;
12 changes: 9 additions & 3 deletions src/ng/compile.js
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).
Copy link
Contributor

Choose a reason for hiding this comment

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

So you're a "dots at the end of the line" kind of guy eh?
This is not in-keeping with the rest of the codebase

Copy link
Member Author

Choose a reason for hiding this comment

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

Knee-jerk reaction, sorry 😞 I'll amend.

(Although, there are some occurrences and I myself have sneaked in a couple of those lately 😛)

Copy link
Contributor

Choose a reason for hiding this comment

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

I did a search of the src folder for /\w+\.$/ and didn't find any - although there were lots of matches for sentences in the docs and I might have missed the code ones.

Copy link
Member Author

@gkalpak gkalpak Oct 4, 2016

Choose a reason for hiding this comment

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

I was a little more lucky with my search (without even taking the test/ directory into account, because of too many occurrences) 😃 :

https://github.com/angular/angular.js/blob/823295f/compare-master-to-stable.js#L75-L81
https://github.com/angular/angular.js/blob/823295f/compare-master-to-stable.js#L93-L104
https://github.com/angular/angular.js/blob/823295f/compare-master-to-stable.js#L110-L124
https://github.com/angular/angular.js/blob/823295f/compare-master-to-stable.js#L132-L142
https://github.com/angular/angular.js/blob/823295f/compare-master-to-stable.js#L154-L155
https://github.com/angular/angular.js/blob/823295f/compare-master-to-stable.js#L160-L164
https://github.com/angular/angular.js/blob/823295f/docs/app/test/errorSpec.js#L6-L7
https://github.com/angular/angular.js/blob/823295f/i18n/e2e/i18n-e2e.js#L73-L82
https://github.com/angular/angular.js/blob/823295f/i18n/e2e/i18n-e2e.js#L171-L172
https://github.com/angular/angular.js/blob/823295f/i18n/src/closureI18nExtractor.js#L93-L100
https://github.com/angular/angular.js/blob/823295f/lib/grunt/utils.js#L206-L207
https://github.com/angular/angular.js/blob/823295f/src/Angular.js#L1406-L1409
https://github.com/angular/angular.js/blob/823295f/src/Angular.js#L1425-L1433
https://github.com/angular/angular.js/blob/823295f/src/AngularPublic.js#L173-L174
https://github.com/angular/angular.js/blob/823295f/src/AngularPublic.js#L218-L223
https://github.com/angular/angular.js/blob/823295f/src/jqLite.js#L153-L154
https://github.com/angular/angular.js/blob/823295f/src/jqLite.js#L412-L413
https://github.com/angular/angular.js/blob/823295f/src/ng/cacheFactory.js#L66-L67
https://github.com/angular/angular.js/blob/823295f/src/ng/compile.js#L3292-L3293
https://github.com/angular/angular.js/blob/823295f/src/ng/exceptionHandler.js#L25-L26
https://github.com/angular/angular.js/blob/823295f/src/ng/http.js#L893-L894
https://github.com/angular/angular.js/blob/823295f/src/ng/interpolate.js#L111-L112
https://github.com/angular/angular.js/blob/823295f/src/ng/sce.js#L42-L44
https://github.com/angular/angular.js/blob/823295f/src/ng/directive/ngClass.js#L300-L301
https://github.com/angular/angular.js/blob/823295f/src/ng/directive/ngClass.js#L359-L362
https://github.com/angular/angular.js/blob/823295f/src/ng/directive/ngClass.js#L407-L410
https://github.com/angular/angular.js/blob/823295f/src/ng/directive/ngCloak.js#L46-L49
https://github.com/angular/angular.js/blob/823295f/src/ng/directive/ngModel.js#L94-L95
https://github.com/angular/angular.js/blob/823295f/src/ng/directive/ngModel.js#L160-L161
https://github.com/angular/angular.js/blob/823295f/src/ng/filter/filters.js#L74-L75
https://github.com/angular/angular.js/blob/823295f/src/ng/filter/filters.js#L563-L570
https://github.com/angular/angular.js/blob/823295f/src/ngAria/aria.js#L56-L57
https://github.com/angular/angular.js/blob/823295f/src/ngCookies/cookies.js#L19-L20
https://github.com/angular/angular.js/blob/823295f/src/ngCookies/cookieStore.js#L3-L4
https://github.com/angular/angular.js/blob/823295f/src/ngResource/resource.js#L431-L432
https://github.com/angular/angular.js/blob/823295f/src/ngResource/resource.js#L477-L478
https://github.com/angular/angular.js/blob/823295f/src/ngResource/resource.js#L489-L490
https://github.com/angular/angular.js/blob/823295f/src/ngRoute/route.js#L28-L30
https://github.com/angular/angular.js/blob/823295f/src/ngRoute/route.js#L680-L685
https://github.com/angular/angular.js/blob/823295f/src/ngRoute/route.js#L727-L728
https://github.com/angular/angular.js/blob/823295f/src/ngRoute/route.js#L753-L760
https://github.com/angular/angular.js/blob/823295f/src/ngSanitize/sanitize.js#L110-L116
https://github.com/angular/angular.js/blob/823295f/src/ngSanitize/sanitize.js#L122-L123
https://github.com/angular/angular.js/blob/823295f/src/ngSanitize/sanitize.js#L131-L132
https://github.com/angular/angular.js/blob/823295f/src/ngSanitize/sanitize.js#L423-L433
https://github.com/angular/angular.js/blob/823295f/src/ngSanitize/filter/linky.js#L91-L92
https://github.com/angular/angular.js/blob/823295f/src/ngSanitize/filter/linky.js#L98-L99
https://github.com/angular/angular.js/blob/823295f/src/ngSanitize/filter/linky.js#L107-L108
https://github.com/angular/angular.js/blob/823295f/src/ngSanitize/filter/linky.js#L115-L117
https://github.com/angular/angular.js/blob/823295f/src/ngSanitize/filter/linky.js#L122-L124
https://github.com/angular/angular.js/blob/823295f/src/ngSanitize/filter/linky.js#L752-L755

Copy link
Contributor

Choose a reason for hiding this comment

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

Oh well...

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);
Copy link
Contributor

Choose a reason for hiding this comment

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

This catch shouldn't be needed. The one above it is already catching all errors and is not re-throwing or returning a rejected promise, so this should be unreachable.

Copy link
Member Author

Choose a reason for hiding this comment

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

Depends on what the $exceptionHandler does. Throwing from the $exceptionHandler (as happens in tests) results in a Possibly Unhandled Rejection error.

Copy link
Contributor

Choose a reason for hiding this comment

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

Ah ok, never dawned on me that would be a valid thing to do.


return function delayedNodeLinkFn(ignoreChildLinkFn, scope, node, rootElement, boundTranscludeFn) {
var childBoundTranscludeFn = boundTranscludeFn;
Expand Down
9 changes: 3 additions & 6 deletions src/ng/q.js
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,15 +416,14 @@ 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 (isObject(val) || isFunction(val)) then = val.then;
if (isFunction(then)) {
this.promise.$$state.status = -1;
then.call(val, resolvePromise, rejectPromise, simpleBind(this, this.notify));
Expand All @@ -436,7 +434,6 @@ function qFactory(nextTick, exceptionHandler, errorOnUnhandledRejections) {
}
} catch (e) {
rejectPromise(e);
exceptionHandler(e);
}

function resolvePromise(val) {
Expand Down
84 changes: 45 additions & 39 deletions src/ng/templateRequest.js
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
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 '));
Copy link
Contributor

Choose a reason for hiding this comment

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

I wonder if we should have a helper function for checking the exception handler message?

Copy link
Member Author

Choose a reason for hiding this comment

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

It would be helpful indeed. I'll add one.

});
});

Expand Down
14 changes: 4 additions & 10 deletions test/ng/httpSpec.js
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