🐞[BUG]: Angular ErrorHandler not called anymore (regression)

[jbjhjm]

Affected Package

@ngxs/store

Is this a regression?

Yes, used to work in 3.7.4
(likely introduced in 3.7.6)
97% sure its caused by this change: https://github.com/ngxs/store/pull/1927/files

Description

Errors thrown in Action handlers are not propagated to Angular ErrorHandler anymore.
This is breaking several of our unit tests and also hides errors that else would be propagated to ng ErrorHandler, followed by our Error Management system connecting to that.

With #1927 I found what is likely the source of that change.
As markwhitfield annotated here (without any answer to that unfortunately):
https://github.com/ngxs/store/pull/1927/files#r1018904814
this does change behavior:
As soon as subscribing in any way, ngxs disables error logging.
Even if the only subscriber listens for e.g. completion.

The original feature request asked for a solution to stop doubled error logging. #1691
This solution unfortunately makes it 0 error logging for us and likely others :/

Also it is a breaking change which should be documented and not released in a minor version.
I get why the new approach is better for some users...
but many will go blind, not knowing about this change, implementing stuff, not being notified about thrown errors anymore.
I only learned about this because we had some unit tests based on that exact behavior.
Which made me start searching what has changed and then I found out about this PR.

As a compromise, I ask to make this configurable by a root config flag like it was done for suppressErrors.
By adding a flag disableErrorPropagationWhenSubscribed or similar, this can be an opt-in feature,
not breaking any behavior for existing implementations.
And it allows any devs plagued by doubled error logging to switch this feature on at any time.

🔬 Minimal Reproduction

here's a simple reproduction with a jest test.

import { ErrorHandler, Injectable } from '@angular/core';
import { TestBed } from '@angular/core/testing';
import { Action, NgxsModule, State, Store } from '@ngxs/store';

// eslint-disable-next-line @typescript-eslint/no-empty-interface
interface TestModel {}

class ThrowingAction {
	static readonly type = '[ErrorHandlingIssueState] Throw an Error';
}

@Injectable()
@State<TestModel>({
	name:'errorhandlingreproduction',
	defaults:{}
})
export class ErrorHandlingIssueState {
	@Action(ThrowingAction)
	errorThrowingHandler() {
		throw new Error('Cannot handle something!')
	}
}


describe('reproduction',()=>{
	let store:Store;
	let errorHandler:ErrorHandler;

	beforeEach(() => {
		TestBed.configureTestingModule({
			imports: [
				NgxsModule.forRoot([ErrorHandlingIssueState],{developmentMode:true,selectorOptions: { suppressErrors: false }}),
			],
			providers: [
				ErrorHandler,
			]
		});
		store = TestBed.inject(Store);
		errorHandler = TestBed.inject(ErrorHandler);
		jest.restoreAllMocks()
	});

	it('expect error to be sent to ng error handler', () => {
		/**
		 * this will now fail because due to the recent error handling change,
		 * as soon as anything subscribes, ngxs ignores errors. We are blind now. :(
		 */
		errorHandler.handleError = jest.fn();
		store.dispatch(new ThrowingAction()).subscribe(()=>{
			console.error('oops! if this is being called, the test failed because the action unexpectedly completed without throwing!')
		})
		expect(errorHandler.handleError).toHaveBeenCalled()
	});

})

Environment

Libs:
- @angular/core version: 15.1.0
- @ngxs/store version: 3.7.6

---

Update:

On further debugging, I found it is worse than I thought.
When NOT subscribing, error reporting wont work either.
Not sure what happens, but it seems that inside ngxsErrorHandler,
the code that checks for any subscribers is not executed at all.
Which lets me think of a hot/cold Observable issue first.

it('this will fail too', async () => {
	errorHandler.handleError = jest.fn();
	store.dispatch(new ThrowingAction());
	expect(errorHandler.handleError).toHaveBeenCalled()
});

I found an additional minor behavior change:
Previously, the error was reported to ErrorHandler in a synchronous way.
Due to the new setup, tests expecting ErrorHandler to be notified immediately, seem to fail too.
So even with a fix that disables discarding of errors in case there are any subscribers,
the test described in reproduction will fail, cause it expects synchronous error reporting.
This will work however:

	it('should work asynchronously', async () => {
		/**
		 * this will now fail because due to the recent error handling change,
		 * as soon as anything subscribes, ngxs ignores errors. We are blind now. :(
		 */
		errorHandler.handleError = jest.fn();
		store.dispatch(new ThrowingAction());
		// expect(errorHandler.handleError).toHaveBeenCalled()
		await new Promise((res)=>{
			setTimeout(res,10)
		}).then(()=>{
			expect(errorHandler.handleError).toHaveBeenCalled()
		})
	});

[markwhitfeld]

@jbjhjm Thank you so much for the well-researched and described error report.
I agree that this is a regression. I missed the fact that my comment had not been addressed.
I'll chat to @arturovt about the way forward to resolve this as a patch fix.

[jbjhjm]

thanks for the fast response @markwhitfeld !

Update:

On further debugging, I found it is worse than I thought.
When NOT subscribing, error reporting wont work either.
Not sure what happens, but it seems that inside ngxsErrorHandler,
the code that checks for any subscribers is not executed at all.
Which lets me think of a hot/cold Observable issue first.

it('this will fail too', async () => {
	errorHandler.handleError = jest.fn();
	store.dispatch(new ThrowingAction());
	expect(errorHandler.handleError).toHaveBeenCalled()
});

I found an additional minor behavior change:
Previously, the error was reported to ErrorHandler in a synchronous way.
Due to the new setup, tests expecting ErrorHandler to be notified immediately, seem to fail too.
So even with a fix that disables discarding of errors in case there are any subscribers,
the test described in reproduction will fail, cause it expects synchronous error reporting.
This will work however:

	it('should work asynchronously', async () => {
		/**
		 * this will now fail because due to the recent error handling change,
		 * as soon as anything subscribes, ngxs ignores errors. We are blind now. :(
		 */
		errorHandler.handleError = jest.fn();
		store.dispatch(new ThrowingAction());
		
		// expect(errorHandler.handleError).toHaveBeenCalled()
		await new Promise((res)=>{
			setTimeout(res,10)
		}).then(()=>{
			expect(errorHandler.handleError).toHaveBeenCalled()
		})
	});

[markwhitfeld]

Just a few questions:

• Are you observing this issue in your app (as opposed to tests)?
• Do you have Zone.js enabled?
• Are you running your tests with Zone.js enabled? Usually it is not enabled for tests.

Everything should work as expected if you are operating within a zone.js context.
If you are outside the context then rxjs does not call the angular error handler for you.
We need to know this for more context for the repro.

PS. The Angular ErrorHandler is in place to catch things that are exceptional, and not part of normal program flow.
It would really not be advisable to have any logic that relies on this error handler being called synchronously or not.
I don't see this particular aspect (sync vs async) as a regression, because an application should really not rely on this sort of sequential coupling.

[jbjhjm]

Right now, I am only aware of this issue inside of tests, however I have not tested behavior inside Angular app at all yet.
I'll check this in detail but it is likely I won't have time for it before next week. Will let you know as soon as I have reliable info.

[jbjhjm]

Hi @markwhitfeld , here's the information you were requesting!

Your assumptions were pretty much correct.

• Found the bug in our jest unit tests which do not run zone.js
• Tested the error handling behavior within a zonejs-angular app, and errors are delegated to ErrorHandler correctly!

About your P.S.: Agreed. I'm not describing regular program flow here, just things that I found during debugging and things that we sometimes rely on e.g. when unit testing correct erroring behavior. In such a case a now-async handling will make tests fail that previously worked based on the assumption of synchronous error delegation.
I agree that this is a very minor change. On the other hand I'm not sure if there's a good reason not to delegate errors immediately/in sync to error handler?

[arturovt]

@jbjhjm I was the author of those changes and definitely this seems to be a regression. But this is a "stochastic" regression. This means the actual behavior we have right now is valid at runtime (and as you also confirmed), but the breaking change exists which leads to a regression.

The root issue is that the runtime behavior differs from the unit testing behavior. This is caused by zone.js. It's NOT behaving in the same way as it behaves at runtime. At runtime Angular forks the zone and provides the onHandleError hook which intercepts all of the failed tasks and calls the ErrorHandler.handleError. This isn't happening in unit tests unfortunately.

[jbjhjm]

@arturovt sounds logical.
I agree that the current solution is working fine at runtime.
But I also believe the new implementation itself is a bit intransparent:

Right now, error reporting "mode" will be switched off as soon as ANY subscriber is being added.
Even if it is just a completion callback - ngxs does not know what kind of subscriber was added.
So it is kinda blind in its decision on enabling/disabling error reporting.
Such a mechanism isn't intuitive IMO and might lead to various issues or irritations.

Assuming for most users the previous error handling solution was perfectly fine, I believe a hybrid, configurable approach is most appropriate. As far as I understand the source code, a simple configurable flag toggling the check for existing subscribers on or off would restore the known "classic" behavior.

[arturovt]

It's not transparent.

All of those "hacks" are related to NGXS execution strategy which handles actions outside of the Angular zone by default, meaning that failed tasks are not caught by Angular's zone. This is why we had to subscribe manually to the dispatch result and call error handler manually. We haven't come up to the most transparent approach for now.

[arturovt]

@markwhitfeld I don't think we have to revert the implementation we pair-programmed. We could add a config flag (e.g., useLegacyErrorHandler, which is true by default) and add the ability to transitively switch to the new error handler behavior. We may remove the config flag in v4.

[jbjhjm]

just updated our repo to use ngxs 3.8.0. I found that one reported detail is incorrect:
AFAIK, when using jest-preset-angular, it sets up zone.js accordingly.
So as a result, my test scenarios were running with zone.
Though errors still weren't propagated to zone error handler.

I thought of updating test implementations...
But the risk of any ngxs-related observable hiding errors as soon as there's a subscription would require many changes and is vulnerable as it is easily forgotten to add active error handling everywhere.

Don't see a well working solution despite configurable error handling or a specific error-handling mode for test environments.
Will have to continue using my hacked-in fix and hope to see an official solution by you guys in the future! 😀