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

gkalpak · 2016-10-04T08:17:36Z

What kind of change does this PR introduce? (Bug fix, feature, docs update, ...)
Refactor/Fix (depending on your point of view).

What is the current behavior? (You can also link to an open issue here)
Errors thrown in a promise's onFulfilled or onRejected handlers are treated in a
slightly different manner than regular rejections:
They are passed to the $exceptionHandler() (in addition to being converted to rejections).
See also #3174 and #14745.

What is the new behavior (if this is a feature change)?
The distinction is removed, by skipping the call to $exceptionHandler(), thus treating
thrown errors as regular rejections.

Does this PR introduce a breaking change?
Yes.

Please check if the PR fulfills these requirements

The commit message follows our guidelines: https://github.com/angular/angular.js/blob/master/CONTRIBUTING.md#commit-message-format
Tests for the changes have been added (for bug fixes / features)
~~Docs have been added / updated (for bug fixes / features)~~

Other information:
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 #3174
Fixes #14745

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

gkalpak · 2016-10-04T08:48:23Z

Should we document that the $exceptionHandler is called on 1.5.x or earlier (as suggested in #3174 (comment))?
Sounds like low level stuff.

petebacondarwin

A few minor things but generally LGTM

petebacondarwin · 2016-10-04T12:51:29Z

src/ng/compile.js

@@ -3052,8 +3052,8 @@ function $CompileProvider($provide, $$sanitizeUriProvider) {

      $compileNode.empty();

-      $templateRequest(templateUrl)
-        .then(function(content) {
+      $templateRequest(templateUrl).


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

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

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

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.

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

petebacondarwin · 2016-10-04T12:59:57Z

test/ng/compileSpec.js

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


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

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

petebacondarwin · 2016-10-04T13:02:37Z

test/ng/templateRequestSpec.js


-    expect(function() {
-      $rootScope.$digest();
+      $templateRequest('tpl.html').catch(noop);


Perhaps this noop should be a spy; and we should check that it gets called?

petebacondarwin · 2016-10-04T13:02:52Z

test/ng/templateRequestSpec.js

@@ -115,19 +115,17 @@ describe('$templateRequest', function() {
  }));

  it('should throw an error when the template is not found',


This description is no longer accurate.

dcherman · 2016-10-04T13:55:30Z

src/ng/compile.js

+            $exceptionHandler(error);
+          }
+        }).
+        catch(noop);


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.

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

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

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 angular#3174 Fixes angular#14745 Closes angular#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.

gkalpak added needs: review type: refactor needs: breaking change component: $q severity: inconvenient labels Oct 4, 2016

gkalpak added this to the 1.6.0-rc.1 milestone Oct 4, 2016

googlebot added the cla: yes label Oct 4, 2016

gkalpak mentioned this pull request Oct 4, 2016

feat($httpBackend): JSONP requests now require trusted resource #15161

Closed

3 tasks

fixup! fix($q): treat thrown errors as regular rejections

58cc8a5

gkalpak force-pushed the fix-q-not-call-exceptionHandler-for-thrown-errors branch from a1bf542 to 58cc8a5 Compare October 4, 2016 12:20

gkalpak mentioned this pull request Oct 4, 2016

Chained promises broken when testing due to $exceptionHandler interference #3174

Closed

petebacondarwin suggested changes Oct 4, 2016

View reviewed changes

dcherman reviewed Oct 4, 2016

View reviewed changes

fixup! fix($q): treat thrown errors as regular rejections

0c4266d

petebacondarwin approved these changes Oct 5, 2016

View reviewed changes

gkalpak force-pushed the fix-q-not-call-exceptionHandler-for-thrown-errors branch from e3dcd47 to 0c4266d Compare October 5, 2016 11:42

gkalpak closed this in e13eeab Oct 5, 2016

gkalpak deleted the fix-q-not-call-exceptionHandler-for-thrown-errors branch October 5, 2016 11:53

gkalpak mentioned this pull request Oct 5, 2016

test(*): introduce the toEqualMinErr() custom Jasmine matcher #15216

Closed

3 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

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

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

gkalpak commented Oct 4, 2016

gkalpak commented Oct 4, 2016

petebacondarwin left a comment

petebacondarwin Oct 4, 2016

gkalpak Oct 4, 2016

petebacondarwin Oct 4, 2016

gkalpak Oct 4, 2016 •

edited

Loading

petebacondarwin Oct 5, 2016

petebacondarwin Oct 4, 2016

gkalpak Oct 4, 2016

petebacondarwin Oct 4, 2016

petebacondarwin Oct 4, 2016

dcherman Oct 4, 2016

gkalpak Oct 4, 2016

dcherman Oct 4, 2016

		@@ -115,19 +115,17 @@ describe('$templateRequest', function() {
		}));

		it('should throw an error when the template is not found',

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

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

Conversation

gkalpak commented Oct 4, 2016

gkalpak commented Oct 4, 2016

petebacondarwin left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

gkalpak Oct 4, 2016 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

gkalpak Oct 4, 2016 •

edited

Loading