paulmillr / es6-shim

ECMAScript 6 compatibility shims for legacy JS engines
http://paulmillr.com
MIT License
3.11k stars 389 forks source link

Unhandled Promise errors #311

Open timoxley opened 9 years ago

timoxley commented 9 years ago

Currently, if there's no .catch errors will vanish mysteriously. Native promises in chrome 41.0.2249.0 (current canary) will print a message about unhandled promise errors:

image

but if the es6-shim is loaded, it clobbers the native Promise because it doesn't seem to support subclassing, and replaces it with an insidious version with total error suppression:

image

Not sure what the best approach is to solve this.

timoxley commented 9 years ago

Not even sure if chrome implementation is valid, i.e. are you supposed to be able to attach catch handlers asynchronously?

cscott commented 9 years ago

Yes, you are supposed to be able to attach catch handlers asynchronously. But I think Chrome is supposed to remove the message from console if that happens, or something like that?

It would be nice to chain through native promises. The most straightforward way to do that would be to subclass them.... but of course that's what Chrome isn't supporting yet. Boo.

ljharb commented 9 years ago

I wonder if it would be worth it, in the case where there's a native Promise that doesn't support subclassing but otherwise works, to use an alternative Promise shim that is a subclassable proxy wrapper around the native one? If that worked, it should preserve this behavior.

At any rate, the Chrome behavior of console warning when you forget .catch isn't part of the spec, and is something the browser is choosing to do, so it's really not the shim's responsibility to support or maintain that functionality.

Yaffle commented 9 years ago

@ljharb , Did you try to output an error with console.log or console.error ? I tried, but it is printed without "stack", so it is not easy to jump to a source code. Although, I think, the console API should allow this, because it is useful when you have "onReject", but you do not handle all kinds of errors inside.

cscott commented 9 years ago

@ljharb what's the status of proxy support in browsers?

My initial thought was that maybe our Promise implementation could attach a "native" promise to the end of its promise chain, and then remove it as soon as then/catch was called (since there would presumably be a new native promise chained after that). This ought to preserve the browser's "uncaught exception" behavior, although perhaps it wouldn't capture the stack. It would also have a performance impact, although perhaps that would be slight.

ljharb commented 9 years ago

I don't think anything has proxies yet. That's definitely a use case for it though.

Yaffle commented 9 years ago

@ljharb I cannot understand this issue, but

  var x = Promise.reject(new Error("!"));
  setTimeout(function () {
    x.then(undefined, function (e) {
      console.log(e);
    });
  }, 0);

this causes a error message in the log in Chrome. And if replace setTimeout with Promise.resolve(undefined) - nothing will be printed to the log. So to simulate same behavior you could use setTimeout, right?

cscott commented 9 years ago

@Yaffle yes, many utility libraries for Promises (such as http://github.com/cscott/prfun) add a Promise.prototype.done method which uses setTimeout in exactly this way.

There's also a web spec in-process for triggering the "uncaught promise exception" handler in the same way that there is an "uncaught exception" handler in the web stack for synchronous code. I don't know if Chrome has exposed this to users yet, though. If they have, we could hook it.

AlastairTaft commented 9 years ago

The try catch around line 1867 is currently eating all my Promise errors (in the promiseReactionJob method). All my promise errors are caught and not thrown so it fails silently. I'm going to have to run with a flag to not include the es6-shim when developing in chrome. That also makes me wonder that the shim shouldn't be overriding native chrome promise support.

ljharb commented 9 years ago

@AlastairTaft I'm not sure what you mean - that's what native Promises do by spec. All Promises swallow all errors, and the only way to catch them is with a .catch.

The additional browser behavior of notifying the console when you have an unhandled rejection is not part of the spec, it's bonus behavior some promises provide.

AlastairTaft commented 9 years ago

@ljharb If it's by design then no worries. I'll provide some code though to explain my point a bit better just encase it is a problem.

So I'm doing something like this in my code

var self = this
// Get the content
Promise.all([
  api.client.getProfileP(),
  api.client.getInfoSummariesP()
])
.then(function(args){
  var profile = args[0]
    , infoSummaries = args[1]

  self.loadContent_(profile, infoSummaries)
})
.catch(function(err){
  throw err
})

When I hit a run time error in the self.loadContent_(profile, infoSummaries) method the throw err is swallowed and nothing is output to the console.

ljharb commented 9 years ago

Correct. You can never throw an error outside of a Promise - that's how they work everywhere. You can do setTimeout(function () { throw err; }, 0) but that's basically your only option.

Handle the error in the right place: inside the promise :-)

AlastairTaft commented 9 years ago

Fair enough :)

cscott commented 9 years ago

The prfun library (and others) have a Promise #done method that can also be helpful in making your thrown errors visible.

ertant commented 8 years ago

At least for debug versions I have changed the rejectPromise (line 1913) function like below, otherwise finding an error almost impossible.

var rejectPromise = function (promise, reason) {
   .....
   if(!reactions.length) {
      console.warn('Uncaught (in promise)', reason, reason.stack);
   }
   triggerPromiseReactions(reactions, reason);
};