feat(Observable): unhandled errors are now reported to HostReportErrors #3062
Conversation
Generated by 🚫 dangerJS |
---3.9KB-green.svg?style=flat-square){kind=icon}
-+0.1KB-red.svg?style=flat-square){kind=icon}
|
Will check this soon. |
src/Subscriber.ts
Outdated
| this.unsubscribe(); | ||
| this._hostReportError(err); |
There was a problem hiding this comment.
not sure if this need to be aligned - cases like error / complete we do report error first then unsubscribe, for next we do unsubscribe first then report. Nitpick maybe?
There was a problem hiding this comment.
The error is reported asynchronously, so in this case it doesn't matter. However, I think it's probably supposed to be start reporting error then unsubscribe. (if the error reporting was synchronous, which it isn't any more)
src/operators/tap.ts
Outdated
| super(destination); | ||
| this._tapError = error || noop; | ||
| this._tapComplete = complete || noop; | ||
| this._tapNext = noop; |
There was a problem hiding this comment.
83 already assigned into noop, do we need double init?
|
Except few nits I think change looks ok, probably biggest thing we'd like to get opinion is this breaking change itself I guess. |
The opinions have already been expressed here It's part of the spec, and frankly I really am not interested in this discussion again, as you can read the last one was long and heated. We want to align with the proposal. More specifically, @mattpodwysocki has already weighed in here. The reason we waited so long is it's a particular nasty refactor and it's a breaking change. |
BREAKING CHANGE: Unhandled errors are no longer caught and rethrown, rather they are caught and scheduled to be thrown, which causes them to be reported to window.onerror or process.on('error'), depending on the environment. Consequently, teardown after a synchronous, unhandled, error will no longer occur, as the teardown would not exist, and producer interference cannot occur
|
I feel it's hard for me to imagine what is this change going to cause (for example in Angular apps). I guess it shouldn't change basically anything for me because I don't rely on rethrowing errors but I'm not completely sure. Maybe this change should be described in MIGRATION.md with some before/after example. |
|
This thread has been automatically locked since there has not been any recent activity after it was closed. Please open a new issue for related bugs. |
BREAKING CHANGE: Unhandled errors are no longer caught and rethrown, rather they are caught and scheduled to be thrown, which causes them to be reported to window.onerror or process.on('error'), depending on the environment. Consequently, teardown after a synchronous, unhandled, error will no longer occur, as the teardown would not exist, and producer interference cannot occur