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
wants to merge 3 commits into from
Closed

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

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
e3b3815
fix($q): treat thrown errors as regular rejections
gkalpak Sep 30, 2016
58cc8a5
fixup! fix($q): treat thrown errors as regular rejections
gkalpak Oct 4, 2016
0c4266d
fixup! fix($q): treat thrown errors as regular rejections
gkalpak Oct 5, 2016
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
View file
Open in desktop
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;
4 changes: 4 additions & 0 deletions src/ng/compile.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3131,6 +3131,10 @@ function $CompileProvider($provide, $$sanitizeUriProvider) {
childBoundTranscludeFn);
}
linkQueue = null;
}).catch(function(error) {
if (error instanceof Error) {
$exceptionHandler(error);
}
}).catch(noop);

return function delayedNodeLinkFn(ignoreChildLinkFn, scope, node, rootElement, boundTranscludeFn) {
Expand Down
9 changes: 3 additions & 6 deletions src/ng/q.js
View file
Open in desktop
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
View file
Open in desktop
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
Open in desktop
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
View file
Open in desktop
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