cujojs / when

A solid, fast Promises/A+ and when() implementation, plus other async goodies.
Other
3.44k stars 396 forks source link

Emit unhandled rejection events in Node #419

Closed benjamingr closed 9 years ago

benjamingr commented 9 years ago

Hey, I've written the following proposal here: https://gist.github.com/benjamingr/0237932cee84712951a2

I'd love your feedback.

briancavalier commented 9 years ago

Seems like a pretty nice idea. I'd be willing to implement it if we can work out all the details across libs.

One issue might be that libs that already report unhandled rejections out of the box may want to try to detect if there are actually any listeners registered for the possiblyUnhandledRejection event. That way, the out of the box experience is still good without having to register the handler. Then, if there is at least one handler, use process.emit. That's detectable right now via process.listeners(...).length, but I dunno if it's kosher to depend on that.

Of course, that means if some 3rd party lib adds a listener that only does remote error reporting, or does something silly without actually logging the error, then errors will go silent ... hrm. Not sure how to deal with that except just to say "don't do that".

Any thoughts on those things?

I prefer the word "potentially" to "possibly"--to me, "potentially" has a stronger relationship to time than "possibly". But, that's splitting hairs and shouldn't get in the way.

briancavalier commented 9 years ago

Interestingly, it is probably possible to do something similar with window and custom DOM events in browsers, ie via document.createEvent and window.dispatchEvent. There's no way to detect the presence of listeners, of course.

benjamingr commented 9 years ago

We were just discussing the default log handler in the promises IRC channel. My first intuition is to log unless a handler was attached to the specific library instance to stop since logging is very valuable and double logging (to the console and to a server for example) is much better than no logging at all. I agree it's an interesting topic and we should investigate how to handle it better.

Petka made some chart: https://gist.github.com/petkaantonov/3fd7a00134056eb3b32d#file-md-md I'm not sure I agree with it or understand it very well though :)


As for window - that idea is also what domenic suggested but I think this is generally less of a problem in browsers (multiple promise libraries are less common) and we should defer making that decision until WhatWG has chosen between their alternatives for this for native promises.

briancavalier commented 9 years ago

Yeah, the chart makes sense, at least as far as I understand how Bluebird works. I was thinking a bit differently for when.js though, since all of the unhandled rejection stuff is implemented externally. I was thinking the logger that's currently installed by default would do what it does now, unless a process event handler has been added, in which case it would process.emit instead. Like you say, tho, It may make sense to do both, in which case, you may get double logging. If you opt to install your own logger, though, whatever it does is what you would get.

Either way, though, it'd be great to put together an npm package that listened to these new process events and did some reasonable default logging.

Re: window, multiple promise impls definitely do happen. For example, es6-module-loader and RaveJS both embed when.js's ES6 Promise shim, so you're starting out with one promise library already. Add any reasonably complex app on top of those, and you can end up with at least a second promise library pretty quickly. I agree though that the multiversion problem probably happens far less often in browser apps. Now that RaveJS supports recursive node_modules, though, it certainly can happen.

sompylasar commented 9 years ago

As for browser-side, consider webpack/browserify apps, too. They could have several promise libraries with almost the same probability as node apps.

benjamingr commented 9 years ago

Ah, that's an interesting case about browserify and es6-module-loader with multiple promise implementations. I wonder if we should include it in this here or wait for a standard to emerge (whatwg are debating it, I'm aware we might be able to make a decision and 'skew' the standard with what's happening de-facto but I think we might want to avoid that)

benjamingr commented 9 years ago

@briancavalier ping.

By the way - bluebird already rolled with this - RSVP and es6promise will roll out with this this weekend.

Bluebird fires window events now but I think this issue and that one are separate.

briancavalier commented 9 years ago

@benjamingr Sorry, have had other things on my plate recently. I def see the value to users in these most popular impls all releasing this feature at or around the same time. I should be able to have something ready to release by this weekend.

briancavalier commented 9 years ago

@benjamingr This just landed in #422. It'll be released shortly in 3.7.0. Thanks again for leading the charge on this.

benjamingr commented 9 years ago

Awesome thanks!