feat(store): implement NGXS unhandled error handler #2137

arturovt · 2024-03-21T16:57:38Z

No description provided.

nx-cloud · 2024-03-21T19:29:50Z

☁️ Nx Cloud Report

CI is running/has finished running commands for commit 19c4b4f. As they complete they will appear below. Click to see the status, the terminal output, and the build insights.

📂 See all runs for this CI Pipeline Execution

✅ Successfully ran 4 targets

Sent with 💌 from NxCloud.

bundlemon · 2024-03-21T20:23:42Z

BundleMon (Integration Projects)

Files updated (2)

Status	Path	Size	Limits
✅	Main bundles(Gzip) _{hello-world-ng16/dist-integration/main.(hash) .js}	67.03KB (+89B +0.13%)	+1%
✅	Main bundles(Gzip) _{hello-world-ng17/dist-integration/main.(hash) .js}	68.49KB (+63B +0.09%)	+1%

Total files change +152B +0.11%

Final result: ✅

View report in BundleMon website ➡️

Current branch size history | Target branch size history

markwhitfeld

Awesome work here!
A few comments.
And just wondering if the original issue is represented in a test?

markwhitfeld · 2024-03-22T18:45:37Z

packages/store/src/internal/dispatcher.ts

+import { leaveNgxs } from '../operators/leave-ngxs';
+import { executeUnhandledCallback } from './unhandled-rxjs-error-callback';
+
+function fallbackErrorHandler<T>(ngZone: NgZone) {


A very important aspect of this operator is the fact that it subscribes. We could call it fallbackSubscriber. WDYT? It should also move to another file.

markwhitfeld · 2024-03-22T18:50:28Z

packages/store/src/internal/dispatcher.ts

+    return new Observable<T>(subscriber => {
+      realSubscriber = true;
+      // Now that there is a real subscriber, we can unsubscribe our pro-active subscription
+      subscription.unsubscribe();


This might be called multiple times if there are multiple real subscribers. I think it is just a no-op, but haven't tested it.
We could set it to null after unsubscribing (and change this line to use ?.).
Having a falsy subscription could also be the indicator of whether or not we have a real subscriber.

markwhitfeld · 2024-03-22T19:01:14Z

packages/store/src/internal/unhandled-rxjs-error-callback.ts

+  // Since the error can be essentially anything, we must ensure that we only
+  // handle objects, as weak maps do not allow any other key type besides objects.
+  // The error can also be a string if thrown in the following manner: `throwError('My Error')`.
+  if (error !== null && typeof error === 'object') {


Do we need a strategy for handling non-object errors?

I don't think so. Here's why:

the ecma 2022 standard specifies the Error class which is intended to represent errors

while other types are technically allowed in throw statements, such as throw some_string, it's widely regarded as bad practice. This is because it doesn't include stack trace information, making debugging more difficult and less informative

markwhitfeld · 2024-03-22T19:02:14Z

packages/store/src/ngxs-error-handler.ts

+
+  handleError(_error: any, _context: NgxsErrorContext): void {
+    // Retrieve lazily to avoid cyclic dependency exception.
+    this._errorHandler = this._errorHandler || this._injector.get(ErrorHandler);


Given the other changes to load this lazily in the StateFactory, we should move this back to the constructor.
This would also be closer to what a user may implement.

markwhitfeld · 2024-03-22T19:05:18Z

packages/store/tests/action.spec.ts

-          'Action 1 - dispatch complete',
          'action1 obs - unsubscribe',
+          'Action 1 - dispatch complete',


Is this change because we unsubscribe before subscribing the real subscriber.
Would it go back to what it was if we unsubscribed after adding the real subscriber (in the fallback operator) ?

This change is related to RxJS upgrade, seems like the sequence of some stuff has changed internally

codeclimate · 2024-03-23T23:28:48Z

Code Climate has analyzed commit 19c4b4f and detected 1 issue on this pull request.

Here's the issue category breakdown:

Category	Count
Complexity	1

The test coverage on the diff in this pull request is 91.7% (50% is the threshold).

This pull request will bring the total coverage in the repository to 95.2% (0.0% change).

View more on Code Climate.

arturovt · 2024-03-23T23:41:49Z

And just wondering if the original issue is represented in a test?

The original issue was that the error handler was not being called on each error. We've decided not to support this anymore because it's strange to log errors even if they're handled. For instance, if someone is using Rollbar and providing their custom ErrorHandler which logs errors to Rollbar, even handled errors would be logged to Rollbar, which is completely undesirable.

arturovt force-pushed the feat/unhandled branch from a1142ce to 21a1641 Compare March 21, 2024 17:19

arturovt force-pushed the feat/unhandled branch from 21a1641 to 4dabcae Compare March 21, 2024 20:18

markwhitfeld force-pushed the feat/unhandled branch 2 times, most recently from 2687750 to 37d6200 Compare March 22, 2024 01:23

arturovt force-pushed the feat/unhandled branch from 8922e9e to a1a872d Compare March 22, 2024 06:57

markwhitfeld reviewed Mar 22, 2024

View reviewed changes

arturovt and others added 18 commits March 24, 2024 00:50

feat(store): implement NGXS unhandled error handler

697131d

chore: we now require RXJS 7 (for error handling)

c20c215

feat: export NgxsErrorHandler

dcff2bd

refactor: move the unhandled error handler logic to the StateFactory

a3f2f6d

fix: rather use a WeakMap to avoid object mutation

92045f9

refactor: rename error callback file

4e9cf6f

chore: small tweaks

7725111

fix: issue with Injector destroyed

221babd

fix: must lazy inject ErrorHandler

00e1bb7

fix: dispatch observable should subscribe in correct zone

6496923

fix: tweak lazy inject ErrorHandler

9582cba

fix: internal subscribe should handle errors

9f7b5ca

fix: attempt proactive dispatch subscription

b9c0e9d

fix: attempt earlier proactive dispatch subscription

09cbf51

fix: add error fallback to dispatch observable

ef801e6

fix: report error in a microtask

cb20e0e

fix: check whether error is an object

8f870c4

fix: change to theta since we already use for private stuff

6657abb

arturovt force-pushed the feat/unhandled branch from a1a872d to 6657abb Compare March 23, 2024 22:50

arturovt added 2 commits March 24, 2024 01:04

fix: address comments

58b2a09

docs: update docs

19c4b4f

arturovt marked this pull request as ready for review March 23, 2024 23:30

arturovt requested review from kuncevic, Carniatto, poloagustin, joaqcid and splincode as code owners March 23, 2024 23:30

arturovt merged commit 3d36414 into master Mar 23, 2024
13 checks passed

arturovt deleted the feat/unhandled branch March 23, 2024 23:46

markwhitfeld added this to the v.18.0.0 milestone Jun 24, 2024

markwhitfeld added the released label Jun 24, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat(store): implement NGXS unhandled error handler #2137

feat(store): implement NGXS unhandled error handler #2137

arturovt commented Mar 21, 2024

nx-cloud bot commented Mar 21, 2024 •

edited

Loading

bundlemon bot commented Mar 21, 2024 •

edited

Loading

markwhitfeld left a comment

markwhitfeld Mar 22, 2024

markwhitfeld Mar 22, 2024 •

edited

Loading

markwhitfeld Mar 22, 2024

arturovt Mar 23, 2024

markwhitfeld Mar 22, 2024

markwhitfeld Mar 22, 2024

arturovt Mar 23, 2024

codeclimate bot commented Mar 23, 2024

arturovt commented Mar 23, 2024

feat(store): implement NGXS unhandled error handler #2137

feat(store): implement NGXS unhandled error handler #2137

Conversation

arturovt commented Mar 21, 2024

nx-cloud bot commented Mar 21, 2024 • edited Loading

☁️ Nx Cloud Report

bundlemon bot commented Mar 21, 2024 • edited Loading

BundleMon (Integration Projects)

markwhitfeld left a comment

Choose a reason for hiding this comment

markwhitfeld Mar 22, 2024

Choose a reason for hiding this comment

markwhitfeld Mar 22, 2024 • edited Loading

Choose a reason for hiding this comment

markwhitfeld Mar 22, 2024

Choose a reason for hiding this comment

arturovt Mar 23, 2024

Choose a reason for hiding this comment

markwhitfeld Mar 22, 2024

Choose a reason for hiding this comment

markwhitfeld Mar 22, 2024

Choose a reason for hiding this comment

arturovt Mar 23, 2024

Choose a reason for hiding this comment

codeclimate bot commented Mar 23, 2024

arturovt commented Mar 23, 2024

nx-cloud bot commented Mar 21, 2024 •

edited

Loading

bundlemon bot commented Mar 21, 2024 •

edited

Loading

markwhitfeld Mar 22, 2024 •

edited

Loading