feat($q): report promises with non rejection callback #13662

lgalfaso · 2016-01-02T15:41:07Z

Rejected promises that do not have a callback to handle the rejection report this to $exceptionHandler so they can be logged to the console.

BREAKING CHANGE

Tests that depend on specific order or number of messages in $exceptionHandler
will need to handle rejected promises report.

Closes: #13653
Closes: #7992

jbedard · 2016-01-07T08:41:19Z

src/ngResource/resource.js

@@ -702,12 +702,13 @@ angular.module('ngResource', ['ng']).
              return $q.reject(response);
            });

-            promise.finally(function() {
+            promise = promise.finally(function(response) {


The .finally callback doesn't take any arguments and the result is ignored unless it's a promise (and even when it's a promise the result of that promise is ignored). I think this arg and the return can be dropped.

Fact: If a resource call is canceled the internal promise is rejected

The previous code did the following:

internalPromise.finally(...) this creates a promise that I will call X
internalPromise.then(...) this second promise is returned. I will call this promise Y

If internalPromise is rejected, then it will reject X and Y. The user can handle the rejection of Y as this is what they receive, but they can do nothing about X. This leaves us with 2 options

1/ Change the code to

promise.finally(...).then(noop, noop); promise = promise.then(...);

This will handle any rejections from X

2/ What is in this patch

promise = promise.finally(...).then(...);

The second option has the benefit of not creating another promise

That all makes sense... but response is undefined 100% of the time because the .finally callback doesn't take any arguments. You can drop the response arg and return statement...

lgalfaso · 2016-03-20T13:29:58Z

@gkalpak added an option to disable this behavior. PTAL.

gkalpak · 2016-03-21T11:03:54Z

src/ng/q.js

+   * @kind function
+   *
+   * @description
+   * Retrieves or overrides on wheter to generate an error when a rejected promise is not handled.


on wheter --> whether (?)

gkalpak · 2016-03-21T11:57:02Z

I left some comments. Some additional thoughts:

If we have this enabled by default, shouldn't we also document the extra errors as a BC.
E.g. if my otherwise perfect production application started logging warnings in my clients console, I would like a big, bold heads-up, so I can take appropriate meassures.
Same as with $timeout/$interval, an explicitly cancelled $resource request (using $cancelRequest) should not report an error, imo.
To avoid surprises, we should be more explicit - both in the docs and in tests - about cases that we are handling internally (e.g. $timeout/$interval#cancel(), $resource#$cancelRequest()?).

lgalfaso · 2016-03-21T21:39:55Z

Updated the PR with the changes. Made the change to $resource#$cancelRequest() so it follows the same logic that is present for $timeout/$interval#cancel().

That said, I do not think that there is a need to add any special documentation for this case.

gkalpak · 2016-03-22T10:21:20Z

src/ng/q.js

+
+  /**
+   * @ngdoc method
+   * @name $qProvider#errorOnUnhandledRejections


I wonder where this method shows up in the docs (considering there are no docs for $qProvider).
I think there should "standalone" docs for $qProvider (even if just containing this method), but it can be tackled in a follow-up PR.

gkalpak · 2016-03-22T11:06:24Z

I left a couple of minor comments, but it LGTM otherwise.

I'm not sure about the default value of the flag. I guess true is OK although it might scare people at first when they migrate to 1.6.x (I don't think anyone is handling all their rejections in tests for example).
Maybe we could even backport it to 1.5.x with the flag set to false by default (but it's not necessary).

lgalfaso · 2016-03-22T20:02:15Z

Added a page for qProvider, fixed the documentation issue and changed the remaining .then(noop, noop) to catch(noop)

lgalfaso · 2016-03-22T20:03:24Z

About backporting this to 1.5. I am fine as long as the default value is false. This ways, people can check this before 1.6 is out.

Rejected promises that do not have a callback to handle the rejection report this to $exceptionHandler so they can be logged to the console. BREAKING CHANGE Unhandled rejected promises will be logged to $exceptionHandler. Tests that depend on specific order or number of messages in $exceptionHandler will need to handle rejected promises report. Closes: angular#13653 Closes: angular#7992

…me promise

lgalfaso · 2016-03-27T13:08:34Z

landed into master as c9dffde and 71cf28c

Angular 1.5 has a bug that causes angular's `$exceptionHandler` to be called for rejected `$q` promises even if they have a rejection handler. This bug caused duplicate error messages in Etcher. A consequence of upgrading to Angular 1.6 is that `$q` promises without a rejection handler will throw `Possibly unhandled rejection` errors. To avoid these errors, this commit moves code responsible for opening a tooltip from the template to the controller and handles the rejection. Other packages upgraded: - angular-moment to v1.0.1 - angular-ui-router to v0.4.2 - angular-mocks to v1.6.1 Change-type: patch Changelog-Entry: Fix duplicate error messages Fixes: #1082 See: angular/angular.js#7992 See: angular/angular.js#13662

amalitsky · 2018-01-09T02:25:12Z

src/ng/q.js

+      if (!toCheck.pur) {
+        toCheck.pur = true;
+        var errorMessage = 'Possibly unhandled rejection: ' + toDebugString(toCheck.value);
+        exceptionHandler(errorMessage);


Out of curiosity, why errorMessage isn't wrapped by new Error() so that stack trace is available?

Good question 🤔

@amalitsky This code path is only used if the value thrown in a promise callback was not an error. We could construct an error object here but it wouldn't be of much value as its stack would end up here and not in the place from where the error comes.

To be useful, only errors need to be thrown in promise callbacks and not other values. Which is a good idea in general.

@mgol, wouldn't path also have a reference to the promise invocation?

lgalfaso added component: $q branch: 1.6.x labels Jan 2, 2016

lgalfaso added this to the 1.6.x milestone Jan 2, 2016

googlebot added the cla: yes label Jan 2, 2016

lgalfaso mentioned this pull request Jan 2, 2016

$q: Rejected Promises with non rejection callback should report the rejection reason in the console. #13653

Closed

lgalfaso force-pushed the q-issue-13653 branch from 8c2419d to e6c1449 Compare January 2, 2016 16:05

jbedard reviewed Jan 7, 2016
View reviewed changes

Narretz force-pushed the master branch from 70049ae to b77e14b Compare January 16, 2016 21:59

lgalfaso force-pushed the q-issue-13653 branch from 9f7ed38 to c9e1eb7 Compare March 20, 2016 13:29

gkalpak reviewed Mar 21, 2016
View reviewed changes

lgalfaso force-pushed the q-issue-13653 branch 2 times, most recently from e812b71 to d18ff2e Compare March 21, 2016 21:36

lgalfaso force-pushed the q-issue-13653 branch 2 times, most recently from a85c87f to f03988c Compare March 21, 2016 21:42

gkalpak reviewed Mar 22, 2016
View reviewed changes

lgalfaso force-pushed the q-issue-13653 branch from f03988c to c59a6d1 Compare March 22, 2016 20:02

lgalfaso force-pushed the q-issue-13653 branch from c59a6d1 to 7374c53 Compare March 22, 2016 20:38

refactor(ngResource): handle success and error callbacks using the sa…

ac84e47

…me promise

lgalfaso force-pushed the q-issue-13653 branch from 7374c53 to ac84e47 Compare March 22, 2016 21:03

lgalfaso closed this Mar 27, 2016

lgalfaso mentioned this pull request May 6, 2016

Handler for unhandled rejections #14569

Closed

gkalpak mentioned this pull request May 13, 2016

Rejected promises don't get handled by $exceptionHandler #11754

Closed

stefan-mihaila mentioned this pull request Feb 4, 2017

Duplicated error messages balena-io/etcher#1082

Closed

stefan-mihaila mentioned this pull request Feb 7, 2017

upgrade: angular to v1.6.1; angular-ui-bootstrap to v2.5.0 balena-io/etcher#1083

Merged

amalitsky reviewed Jan 9, 2018

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat($q): report promises with non rejection callback #13662

feat($q): report promises with non rejection callback #13662

lgalfaso commented Jan 2, 2016

jbedard Jan 7, 2016

lgalfaso Jan 7, 2016

jbedard Jan 8, 2016

lgalfaso commented Mar 20, 2016

gkalpak Mar 21, 2016

gkalpak commented Mar 21, 2016

lgalfaso commented Mar 21, 2016

gkalpak Mar 22, 2016

gkalpak commented Mar 22, 2016

lgalfaso commented Mar 22, 2016

lgalfaso commented Mar 22, 2016

lgalfaso commented Mar 27, 2016

amalitsky Jan 9, 2018

gkalpak Jan 25, 2018

mgol Jun 22, 2018

amalitsky Jun 22, 2018

feat($q): report promises with non rejection callback #13662

feat($q): report promises with non rejection callback #13662

Conversation

lgalfaso commented Jan 2, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

lgalfaso commented Mar 20, 2016

Choose a reason for hiding this comment

gkalpak commented Mar 21, 2016

lgalfaso commented Mar 21, 2016

Choose a reason for hiding this comment

gkalpak commented Mar 22, 2016

lgalfaso commented Mar 22, 2016

lgalfaso commented Mar 22, 2016

lgalfaso commented Mar 27, 2016

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