refactor(SafeSubscriber): simplify methods

[benlesh]

refactor(SafeSubscriber): simplify methods

• Simplifies the call patterns in SafeSubscriber to not be quite so insane.
• Note: for some reason one mocha test is passing, but still printing out the error it is catching in expect(fn).to.throw() in the console. Annoying, but I stepped through the RxJS code and verified the behavior was correct.

fix(bindNodeCallback): now emits errors that happen after callback

• No longer logs to console warn
• AsyncSubject overhead no longer required
• Inlined the deprecated scheduled path

fix(bindCallback): now emits errors that happen after callback

• No longer logs to console warn
• AsyncSubject overhead no longer required
• Inlined the scheduled path as is it deprecated

refactor(Observable): stop deprecated error handling from executing unnecessary code

• Adds a few comments
• Moves canReportError to the only place it is used

Related #5646

[cartant]

These changes LGTM, but the problem with the error being logged - in the otherwise passing test - is due to this code:

      if (config.useDeprecatedSynchronousErrorHandling) {
        sink.syncErrorThrown = true;
        sink.syncErrorValue = err;
      }
      if (canReportError(sink)) {
        sink.error(err);
      } else {
        console.warn(err);
      }

I think this should be something like:

      if (canReportError(sink)) {
        sink.error(err);
      } else if (config.useDeprecatedSynchronousErrorHandling) {
        sink.syncErrorThrown = true;
        sink.syncErrorValue = err;
      } else {
        console.warn(err);
      }

I don't think errors should be reported via the error callback and thrown. And I don't think they should be written to the console and thrown either.

Also, I'm presuming that the binding of the subscriber's context to the standalone next callback - if useDeprecatedNextContext is enabled - is going to be done in another PR.

[benlesh]

Yeah. I didn't get to that. I hate that code, too. But I'm concerned as to why that's being hit, I'll revisit that in this PR i guess. I've been thinking about ways to make that cleaner.

[benlesh]

Okay. A bit of yak-shaving, but overall I think it's worth it. I've updated my comment.

Basically, when looking for other usages of canReportError, I noticed that bindCallback and bindNodeCallback had incorrect behavior, where it should have emitted the error from a function that errors after the callback. If it's synchronous, it will next and then error, but the error should always be emitted.

That allowed me to move canReportError (which could be expensive, but I'll address that later) to Observable.ts.

In Observable.ts I noticed that we were hitting unnecessary code paths in the deprecated error handling case, and fixed that with a simple conditional.

I have plans in an upcoming PR to have a point to register a handler for these "unreportable errors" or whatever we want to call them. Once that is in place, we can do something where we only hit canReportError if there's a registered handler, and we'd never call console.warn again. (I hate that).

[cartant]

LGTM. New bindCallback implementations are really nice. I don't think the canReportError comment is accurate, though.

[cartant]

issue: This comment is not quite correct, AFAICT. Execution can only arrive here if an error is thrown - not emitted as a notification via the observer's error method. Specifically, the canReportError business was added because implementation errors - within the RxJS core - were being swallowed when unintentional errors where thrown after complete or error notifications had been made. And the swallowed errors meant the problems were hard-to-find.

IMO, we should edit the comment to reflect this. I feel strongly that - with the upcoming major version change - we should take the opportunity to simplify this and make it more general (in a separate PR). I am strongly of the opinion that any call to an observer's error method should effect a call to hostReportError - or some developer-configurable hook - rather than do what's done now if the subscriber is stopped: nothing (see here and here). I am of the opinion that there is no greater sin than swallowing errors - especially within a library - and if there is any hill upon which I would be prepared to die, this would be it.

[benlesh]

@cartant. I have a follow-up PR here that adds a new configurable unhandled error hook, and removes this console.warn:

#5681

[cartant]

❤ I like the new bindCallback implementations. Nice!

[benlesh]

I've updated the comment and merged. (I was too impatient to wait for CI after all I did was update a comment, so if it breaks, it's my fault I guess. LOL)