Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Possibly unhandled error should be thrown #70

Open
ysmood opened this issue Jan 3, 2015 · 44 comments
Open

Possibly unhandled error should be thrown #70

ysmood opened this issue Jan 3, 2015 · 44 comments
Milestone

Comments

@ysmood
Copy link

ysmood commented Jan 3, 2015

Node v0.10, non-harmony. Such as:

var Promise = require('bluebird');
new Promise(function () { throw('err') });
// Unhandled error will be thrown.
var Promise = require('es6-promise').Promise;
new Promise(function () { throw('err') });
// Nothing will happen.
@stefanpenner
Copy link
Owner

Upstream RSVP supports this, as this isn't part of the spec it is up to @jakearchibald if he wants it or not.

Let me know, and i'll add it.

@ysmood
Copy link
Author

ysmood commented Jan 3, 2015

This should be part of the shim for old browser. Or it could be really error-prone in practical usage.
For the sake of the Promise spec, we could add an option to turn it on or off.

@stefanpenner
Copy link
Owner

I strangely suspect @jakearchibald will be fine with this.

@adius
Copy link

adius commented Jan 6, 2015

Unhandled errors should definitely be thrown!

@benjamingr
Copy link

I've written the following proposal about this here https://gist.github.com/benjamingr/0237932cee84712951a2 I'd love your feedback. Here's the related RSVP issue: tildeio/rsvp.js#355

@mziwisky
Copy link

+1, throw me them errors, please -- spent an hour tracking down what turned out to be a missing require call because the ReferenceError that was thrown during the success callback got swallowed up and hidden from my prying eyes. makes me very sad.

at the least, es6-promise could expose a sort of catchall error-handling API as RSVP seems to do here: https://github.com/tildeio/rsvp.js#error-handling

@jakearchibald
Copy link
Collaborator

Chrome currently logs unhandled errors. Firefox does it on GC.

Happy to do the same as Chrome here. It should be a log rather than a throw, we'd be spec-breaking if it appeared in window.onerror.

@ysmood
Copy link
Author

ysmood commented Jan 15, 2015

@jakearchibald node.js still don't throw anything.

@jakearchibald
Copy link
Collaborator

Yep. Wait, maybe you misunderstood my comment, "Chrome currently logs unhandled errors. Firefox does it on GC." was refering to native promises

@benjdlambert
Copy link

IMO its a totally different type of error, its not a synchronous error, its not like you can wrap a try-catch block round it.

It should be something that should be pushed into the .catch handler, as the promise spec states, maybe logged similar to how bluebird doing it.

@benjamingr
Copy link

@ysmood io is merging a patch for the events (my proposal) in the next version. I think you should implement this

@ollym
Copy link

ollym commented Jun 25, 2015

Is this going to be done? A console.error(err) is all we need.
Really difficult to debug without it.

@ysmood
Copy link
Author

ysmood commented Jun 25, 2015

@ollym I wrote another Promsie lib, it's smaller, faster, and more error friendly. For more details goto: https://github.com/ysmood/yaku

@stefanpenner
Copy link
Owner

@ysmood good job! Im curious if you have any enumeration/denodify benchmark examples to go with your existing benchmarks.

@ysmood
Copy link
Author

ysmood commented Jun 26, 2015

@stefanpenner I'm not sure what the enumeration/denodify benchmark means. Does it mean non-promise callback version benchmark?

@stefanpenner
Copy link
Owner

benchmarks involving Promise.all, or benchmarks involving assimilation of node callback style API.

@spikebrehm
Copy link

Is there something we project consumers can do to help move this along? This is a bit painful.

@ysmood
Copy link
Author

ysmood commented Jul 11, 2015

@spikebrehm You can replace promise-es6 with Yaku. Their api are the same, and yaku fixed the problem.

@OliverJAsh
Copy link

when does this according to @domenic’s spec: https://github.com/cujojs/when/blob/master/docs/debug-api.md#global-rejection-events

Events:

- 'unhandledRejection': fired when an unhandled rejection is detected
- 'rejectionHandled': fired when rejection previously reported via an 'unhandledRejection' event becomes handled

The default event handler should log.

@sirpengi
Copy link

I'm using v2.2.0 (installed via bower from github.com/components) and I got around this issue with the following shim:

Promise.prototype._onerror = function(err) {
    console.assert(false, err);
}

I don't see why this won't also work on the latest

@ysmood
Copy link
Author

ysmood commented Jul 23, 2015

I think I should close this issue, because I solved the problem perfectly with Yaku, and I can't find any reason to keep using es6-promise any more.

@ysmood ysmood closed this as completed Jul 23, 2015
@usmonster
Copy link

@ysmood in the interest of the greater community that does continue to use and rely on this polyfill, perhaps reopen the issue until it is actually resolved?

@jakearchibald, any comment on the intent to implement this (i.e. merge it down from RSVP)?

Thanks!

@ysmood
Copy link
Author

ysmood commented Jul 24, 2015

@usmonster This issue has lasted half year now. There no API difference between most ES6 promise shim libs. You simply change one line of the package.json of your project, everything will go well. Why do you waste time to wait?

@frosas
Copy link

frosas commented Jul 25, 2015

@ysmood it's not only about waiting for @jakearchibald to address it, but to give visibility of the issue to anyone considering to use this library.

@ysmood
Copy link
Author

ysmood commented Jul 25, 2015

@frosas You are right. If I am the author of ES6-promise, I will write a list to compare different promise libs on the README, and let people aware of the key disadvantages of each lib, include es6-promise itself. We should give people the chance to choose, not hidden the truth.

@ysmood ysmood reopened this Jul 25, 2015
@niondir
Copy link

niondir commented Dec 15, 2015

+1
Chrome also throws the error when not catched. Please do so in es6-promise.

@domenic
Copy link
Contributor

domenic commented Dec 15, 2015

Chrome does not throw an error.

@benjamingr
Copy link

Right, I think what @niondir wants is to add the behavior you specified for Chrome 49 with window.addEventListener("unhandledrejection" - I definitely would like to see es6-promise do this.

@mmoss
Copy link

mmoss commented Dec 23, 2015

What I'm currently seeing.. It definitely appears that Chrome does throw an error...

Chrome Native Promise without Rejection Handler:
nativepromiserethrownwithoutrejectionhandler

Using es6-promise without a Rejection Handler in Chrome:
No indication that something's gone awry

promiseshimwithoutrejectionhandlerswallowserror

@niondir
Copy link

niondir commented Dec 23, 2015

I also do not know why this polyfill takes effect in browsers where promises are implemented already. An alternative solution coud be to just not define the Polyfill where Promise already exists.

Especially because ES6 modules do not allow to conditionally load a module anymore.

@benjamingr
Copy link

A lot of browsers implement the spec incorrectly and do not allow, for example, to subclass promises.

This behavior can be easily amended by implementing the unhandled rejection spec (both the browser and Node) and console.erroring on unhandled rejection like in Chrome.

@mmoss
Copy link

mmoss commented Dec 26, 2015

@benjamingr are you suggesting a simple monkey patch to implement the unhandled rejection spec when using the es6-promise library, or a fix to the lib itself?

bgrozev pushed a commit to jitsi/lib-jitsi-meet that referenced this issue Jan 28, 2016
…t anyway

es6-promise.polyfill() is broken for Chrome atm and replaces the native Promise and swallows unhandled errors.
stefanpenner/es6-promise#70
@ggarek
Copy link

ggarek commented Jun 6, 2016

The means to catch unhandled rejections is a part of ECMAScript 2017 Draft spec (as HostPromiseRejectionTracker). Looking forward to see it in this library!

@stefanpenner
Copy link
Owner

stefanpenner commented Jun 6, 2016

The means to catch unhandled rejections is a part of ECMAScript 2017 Draft spec (as HostPromiseRejectionTracker). Looking forward to see it in this library!

Ya, still trying to find time ;) It will land, I was a big proponent of the feature in the first place.

@ljharb
Copy link
Contributor

ljharb commented Jun 7, 2016

To clarify, the only thing in the ES spec is implementation-specific hooks, and restrictions around what implementations may do. An example implementation of this hook is in whatwg, but that's not part of ECMAScript itself.

@stefanpenner stefanpenner added this to the v5.0.0 milestone Sep 27, 2016
@nilliams
Copy link

nilliams commented Oct 19, 2016

Hi, would love to use this polyfill, but been avoiding for this reason (that the silent swallowing of all exceptions is completely unacceptable).

Now that Node has a workaround for this ...

When a native Promise incurs a rejection but there is no handler to receive it, a warning will be printed to standard error.

... could this polyfill also buck the (IMO lacking) standard and do something similar?

@stefanpenner
Copy link
Owner

@nilliams it will be part of 5.0, as described by the milestone assignment

@nilliams
Copy link

Okay that is great news thanks!

(And sorry I don't find those sorts of github 'events' that interject the issue thread particularly explicit, my eyes tend to skip over them).

@nilliams
Copy link

I want to be open about the fact I've recommended the isomorphic-fetch module moves away from this as suggested polyfill for this reason, until it's fixed, though it's fairly likely I'm just out of the loop on this: matthew-andrews/isomorphic-fetch#111

@stefanpenner
Copy link
Owner

@nilliams I only have so many cycles, given infinite time this would have been done already ;)

@nilliams
Copy link

nilliams commented Oct 21, 2016

Yes :) I know and appreciate that, and apologies ... I really don't mean to pick on your excellent project. I am just very aware that both the fetch and isomorphic-fetch projects (which have some 13,000 stars between them) are recommending using es6-promise as the default polyfill (which of course is not your fault at all). I just really don't feel it is safe to use yet, and I feel like at the moment I'm running around telling people 'hey don't use that cool fetch thing just yet', and currently this is a reason why.

(There are other reasons, like - last I checked - the promise.prototype.finally polyfill also doesn't work with it).

Personally I'm part of a small team and burned maybe 2 days of effort trying to use isomorphic-fetch, and then ripping it out a week or two later, and I don't want people to face the same pain when frankly they'd be better off with sticking to more old-school ajax libraries (again I appreciate it's not your fault these libs are recommending you, and I know you're only sticking to the spec).

I've just added an even worse example to the other thread, I think that bug is actually a separate issue in your tracker -- I think there's a workaround involving using setTimeout in .catch() but I don't have the notes at hand right now: matthew-andrews/isomorphic-fetch#111 (comment)

@stefanpenner
Copy link
Owner

(There are other reasons, like - last I checked - the promise.prototype.finally polyfill also doesn't work with it).

please see: #232 (comment)
implementation: #233

Just waiting patiently to pull the trigger.

@stefanpenner
Copy link
Owner

I just really don't feel it is safe to use yet, and I feel like at the moment I'm running around telling people 'hey don't use that cool fetch thing just yet', and currently this is a reason why.

This isn't new information, I personally use a variant of this exact library which has this feature ;) I just haven't had spare cycles to port it over. Hopefully soon!

Repository owner locked and limited conversation to collaborators Oct 21, 2016
@stefanpenner
Copy link
Owner

stefanpenner commented Oct 21, 2016

Locking this, because no new information is possible. The constraint has been the same for some time, and that is my own time (or someones contribution to this project).

At some-point, hopefully soon, I will have the spare cycles, until then please be mindful of this.

Note: I do appreciate the need and concerns, and as mentioned above, this is something I intend to support. If someone has invented a way to skip sleep, and work on more OSS, please share ;)

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