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

Where to put the unhandled error event hook? Looking for ideas/thoughts #3004

Closed
benlesh opened this issue Oct 26, 2017 · 12 comments
Closed

Comments

@benlesh
Copy link
Member

benlesh commented Oct 26, 2017

For version 6, we need to remove the error rethrowing behavior for Observables. Basically this means that if an error propagates to the end of an Observable chain, it will no longer be rethrown, rather, it will be reported to a global error handler. There are a variety of reasons for this that have already been discussed, so I don't want to discuss those here, but the thing we need to know is, where should we put this error handler?

Prior Art: Promises

Promises allow handling unhandled rejections via window.addEventListener('unhandledrejection', handler) in a browser or process.on('unhandledRejection', handler) in Node.

Caveats

  • It's entirely likely that this solution might change with whatever the spec does.
  • initially, we might just have it log until we can figure out where the eventing should exist.
@kwonoj
Copy link
Member

kwonoj commented Oct 26, 2017

It's entirely likely that this solution might change with whatever the spec does.

Does spec have any glimpse of preferences where it would be?

@benlesh
Copy link
Member Author

benlesh commented Oct 26, 2017

@kwonoj they want to use HostReportErrors, which AFAIK, is an engine implementation specific thing. In other words window.onerror or the like. Perhaps @domenic can provide some insight there?

Either way, I'm not sure that's what a library should be doing. But maybe I'm wrong.

@benlesh
Copy link
Member Author

benlesh commented Oct 26, 2017

cc/ @jhusain @mattpodwysocki

@domenic
Copy link

domenic commented Oct 26, 2017

I think for observables, which don't have the "can be handled later" problem that promises have, you just want to deliver it to the global scope. That basically means setTimeout(() => { throw err; }, 0). In browsers that will fire an error event on the appropriate window (which window is pretty complicated...) and in Node it will cause crashes.

That's what HostReportErrors means in the spec, as well.

@benlesh
Copy link
Member Author

benlesh commented Oct 26, 2017

That's what HostReportErrors means in the spec, as well.

TIL. Thanks @domenic... I'm in favor of doing this.

I'll leave this issue open to discuss whether or not anyone wants to do something RxJS-specific, just to get ideas out there.

@avatsaev
Copy link

I’m just afraid this will be AngularJS’ $rootScope.on hell all over again

@acutmore
Copy link
Contributor

More of an FYI than a serious suggestion:

let err;
const btn = document.createElement(‘button’);
btn.onClick = () => { throw err; };

export function reportError(e) {
  err = e;
  btn.click();
}

Gets reported just like if throwing inside a setTimeout but syncronous. So appears in log at the correct place.

@xtianjohns
Copy link

Hey @benlesh, just want to clarify in light of @domenic's comments: is this still valid:

it will no longer be rethrown, rather, it will be reported to a global error handler.

Or is this: setTimeout(() => { throw err; }, 0); OK? This probably has more to do with the "reasons" for this change, which I know you didn't want to get into. Just wanna know if there's any more to discuss if current rethrow implementation doesn't need to change in any way.

@sparty02
Copy link

sparty02 commented Oct 27, 2017

Prior art: https://github.com/ReactiveX/RxJava/wiki/Plugins. It could hang off of the Rx global namespace as a hook. Does RxJS have any notion of "hooks" right now? Should it?

@chaosmonster
Copy link

Are we only talking about the unhandledrejection event or are we also looking at the rejectionhandled event.
In my opinion we should do both, if we decide to do the first one. Just for completeness sake.

@benlesh
Copy link
Member Author

benlesh commented Oct 30, 2017

@xtianjohns Yes... To disambiguate this... there's rethrowing synchronously (which is the current behavior), and there's rethrowing asynchronously (on a different stack)... which will cause global error reporting to things like window.onerror, process.on('error', fn) etc.

We want to move to the later.

@benlesh
Copy link
Member Author

benlesh commented Oct 30, 2017

@acutmore .. I'm not sure we can rely on that approach as some huge amount of RxJS use is in Node.

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

No branches or pull requests

8 participants