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

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

Closed
xieranmaya opened this issue Dec 30, 2015 · 6 comments

Comments

@xieranmaya
Copy link
Contributor

Eg: If I write the code below:

var promise2 = Promise.resolve(8).then(function(value){
  alter(value)
})

Notice the alter(should be alert) function which is not exist and will cause an error which will be caught and adopted by the promise returned by the last then which is the promise2.

If you run this code in console, you'll see something like Uncaught (in promise) ReferenceError: alter is not defined(…) in console.
The primitive Promise in ES6 will report rejected reason in console when the promise has no rejection handler.

image

But if you change the Promise to $q in the above code snippet, you won't see the error reported in console. Which is bacsuse $q did not do this.

Sometimes if the promise returned by the last then method in promise chain and something went wrong in the last then method, the promise will got rejected, if the promise implementation do not report the error in the console, the developers would never know the error unless he/she dive deep in the code...

On the contrary, Q's done method can report the error(but if you don't chain a done method in the promise chain, it won't), and Bluebird will always report the error(and some useful warnings) in console if a rejected promise has no rejection handler.

Q(notice the done method):
image

Bluebird(with warnings):
image

$q(do not report error on rejected promise):
image

$q(always report):
image

Eh...as I wrote this, I found that since angular 1.4.x, it starts to report the error or exception caught in the promise but even if the exception will be processed in the subsequent promise chain, it still report the error which is not a desired behavior:
image

What is thought to be the desired behavior is that it only report rejected promises' errors when there is no rejection handler registered on the promise(that is, never called then(*,handler) on it), which is like Q or Bluebird or ES6 primitive Promise.

@Narretz
Copy link
Contributor

Narretz commented Dec 31, 2015

I guess this logging comes at a price, because it needs try ... catch blocks around everything that needs to be logged, no? Since $q should be lightweight, I don't think it's necessarily something we want. But I'd like other contributors to chime in on this.

@lgalfaso
Copy link
Contributor

I think there are two sides here:

  1. Reporting exceptions thrown by one of the callbacks (that currently is done always)
  2. Reporting rejections on promises when there are no handlers for it to be reported (that is never done)

I find the current behavior of (1) to be the correct one, an exception was thrown and it should be reported (maybe should be configurable?). The current behavior of (2) is open to discussion; If this is implemented, then when a promise is rejected we may over-trigger as we are not sure if the user will call then later. E.g. this warning will trigger every single time the user calls $q.reject or when a promise is resolved with a rejected promise

@xieranmaya
Copy link
Contributor Author

@lgalfaso I agree with you, and we can deal with (2) to our expectation too
the below code using primitive Promise will not report the Promise.reject(5)'s error, like it somehow "knows" the catch function will process it's rejection, thus I think report or not depends on implementation

Promise.resolve(0).then(function(){
  return Promise.reject(5)
}).catch(alert)

But the below will report the rejection

Promise.resolve(0).then(function(){
  return Promise.reject(5)
})

I have implemented the Promise once, and as I know, the promise's handler function(s) will always run in async mode, which means we can report the error in the reject function in new Promise((resolve,reject)=>{}) when the reject function found that the promise have no rejection handler.
and in the below code

Promise.resolve(0).then(function(){
  var promise2
  return promise2 = Promise.reject(5)
}).catch(alert)

the catch method registered a handler on the promise2 which returned by Promise.reject(5) under the hood thus promise2 it "knows" not to report the error because it has a rejection handler.

I think $q can do the same thing to achieve the goal.

@xieranmaya
Copy link
Contributor Author

And by the way, Bluebird behaves the same way:
image

@lgalfaso
Copy link
Contributor

lgalfaso commented Jan 2, 2016

I think it is possible have something that behaves like this. I think it will take some extra work as we would need to do some changes to $timeout, $interval and ng-resources so they do not generate an unnecessary error. Let me put together a PR and the implementation specifics can be discussed there

lgalfaso added a commit to lgalfaso/angular.js that referenced this issue Jan 2, 2016
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: angular#13653
Closes: angular#7992
@lgalfaso
Copy link
Contributor

lgalfaso commented Jan 2, 2016

Created #13662 that implements this. The change is somehow larger than what I was expecting it to be, but should be good to continue the discussion over there.

lgalfaso added a commit to lgalfaso/angular.js that referenced this issue Jan 2, 2016
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: angular#13653
Closes: angular#7992
lgalfaso added a commit to lgalfaso/angular.js that referenced this issue Mar 20, 2016
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: angular#13653
Closes: angular#7992
lgalfaso added a commit to lgalfaso/angular.js that referenced this issue Mar 21, 2016
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: angular#13653
Closes: angular#7992
lgalfaso added a commit to lgalfaso/angular.js that referenced this issue Mar 21, 2016
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
lgalfaso added a commit to lgalfaso/angular.js that referenced this issue Mar 22, 2016
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
lgalfaso added a commit to lgalfaso/angular.js that referenced this issue Mar 22, 2016
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
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

No branches or pull requests

3 participants