diff --git a/packages/store/src/internal/action-handler-factory.ts b/packages/store/src/internal/action-handler-factory.ts index 279f36232..ad6f4f513 100644 --- a/packages/store/src/internal/action-handler-factory.ts +++ b/packages/store/src/internal/action-handler-factory.ts @@ -51,12 +51,10 @@ export class InternalActionHandlerFactory { let result = handlerFn(stateContext, action); - // We need to use `isPromise` instead of checking whether - // `result instanceof Promise`. In zone.js patched environments, `global.Promise` - // is the `ZoneAwarePromise`. Some APIs, which are likely not patched by zone.js - // for certain reasons, might not work with `instanceof`. For instance, the dynamic - // import returns a native promise (not a `ZoneAwarePromise`), causing this check to - // be falsy. + // Use `isPromise` here, not `result instanceof Promise`. With zone.js + // loaded, `global.Promise` is `ZoneAwarePromise`, but a few APIs hand back + // a native promise instead - e.g. dynamic `import()` - and `instanceof` + // would miss those. if (ɵisPromise(result)) { result = from(result); } @@ -79,9 +77,8 @@ export class InternalActionHandlerFactory { takeUntil( new Observable(subscriber => { return canceled.subscribe(() => { - // Note that we shouldn't use `catchError` to catch abort errors - // because the observable is canceled before the error is thrown, - // so we don't need to handle it. + // No `catchError` needed for abort errors here - we cancel the + // observable before the error is ever thrown. abortController.abort(); subscriber.next(); }); @@ -100,12 +97,10 @@ export class InternalActionHandlerFactory { } result = result.pipe( - // Note that we use the `finalize` operator only when the action handler - // explicitly returns an observable (or a promise) to wait for. This means - // the action handler is written in a "fire & wait" style. If the handler’s - // result is unsubscribed (either because the observable has completed or - // it was unsubscribed by `takeUntil` due to a new action being dispatched), - // we prevent writing to the state context. + // We only reach this `finalize` when the handler returned an + // observable or promise to wait on ("fire & wait" style). Once that + // result is done - completed, or cut off by `takeUntil` when a new + // action comes in - block any further writes to the state context. finalize(() => { if (typeof ngDevMode !== 'undefined' && ngDevMode) { function noopAndWarn() { diff --git a/packages/store/src/internal/action-results.ts b/packages/store/src/internal/action-results.ts index d605d51a7..4b98c9a8f 100644 --- a/packages/store/src/internal/action-results.ts +++ b/packages/store/src/internal/action-results.ts @@ -4,18 +4,16 @@ import { Subject } from 'rxjs'; import type { ActionContext } from '../actions-stream'; /** - * Internal Action result stream that is emitted when an action is completed. - * This is used as a method of returning the action result to the dispatcher - * for the observable returned by the dispatch(...) call. - * The dispatcher then asynchronously pushes the result from this stream onto the main action stream as a result. + * Internal stream of action results - one per completed action. The dispatcher + * listens here to build the observable that `dispatch(...)` hands back, and then + * forwards each result onto the main action stream. */ @Injectable({ providedIn: 'root' }) export class InternalDispatchedActionResults extends Subject { constructor() { super(); - // Complete the subject once the root injector is destroyed to ensure - // there are no active subscribers that would receive events or perform - // any actions after the application is destroyed. + // When the root injector is destroyed, complete the subject so nothing is + // left subscribed and reacting to events after the app is gone. inject(DestroyRef).onDestroy(() => { this.complete(); this.unsubscribe(); diff --git a/packages/store/src/internal/dispatcher.ts b/packages/store/src/internal/dispatcher.ts index d6218a5d7..1edc3e2de 100644 --- a/packages/store/src/internal/dispatcher.ts +++ b/packages/store/src/internal/dispatcher.ts @@ -15,14 +15,14 @@ import { InternalDispatchedActionResults } from './action-results'; import { ActionStatus, InternalActions } from '../actions-stream'; import { InternalNgxsExecutionStrategy } from '../execution/execution-strategy'; -// RxJS reads operator config at construction time and never keeps the object, -// so these can be shared across every dispatch instead of rebuilt each call. +// RxJS copies these settings when it builds the operator and never looks at the +// object again, so every dispatch can share the same one instead of making a new one. const DISPATCH_SHARE_REPLAY = { bufferSize: 1, refCount: true }; const ACTION_RESULT_SHARE = { - // A fresh subject per dispatch, but the same factory each time. + // New subject per dispatch, same factory every time. connector: () => new ReplaySubject<ɵPlainObject>(1), - // A dispatch result happens once; keep replaying it and never re-run. + // An action result happens once - keep replaying it, never re-run the source. resetOnError: false, resetOnComplete: false, resetOnRefCountZero: false @@ -58,11 +58,11 @@ export class InternalDispatcher { if (Array.isArray(actionOrActions)) { if (actionOrActions.length === 0) return of(undefined); - // Note: a canceled action's stream completes without emitting, and - // `forkJoin` completes without emitting if any source does. So if any - // action in the batch is canceled, subscribers get `complete` but no - // `next`, even if the other actions succeeded. This is documented on - // `Store#dispatch`; kept as-is for consistency with single-action cancel. + // Heads up: a canceled action completes without emitting, and `forkJoin` + // does the same if any of its inputs do. So cancel one action in a batch + // and the caller gets `complete` with no `next`, even if the rest worked + // fine. It's the documented behavior on `Store#dispatch` and matches a + // single canceled dispatch, so we leave it alone. return forkJoin(actionOrActions.map(action => this.dispatchSingle(action))).pipe( map(() => undefined) ); @@ -71,8 +71,8 @@ export class InternalDispatcher { } } - // Emits the resulting state (not `void`); `dispatchByEvents` narrows it back - // to `Observable` for the public `dispatch()` contract. + // This one actually emits the new state, not `void` - `dispatchByEvents` maps + // it back down to `void` for the public `dispatch()` signature. private dispatchSingle(action: any): Observable { if (typeof ngDevMode !== 'undefined' && ngDevMode) { const type: string | undefined = getActionTypeFromInstance(action); @@ -85,21 +85,20 @@ export class InternalDispatcher { } if (this._destroyRef.destroyed) { - // Injector was already destroyed → no-op. + // Injector's already destroyed, nothing to do. return EMPTY; } const plugins = this._pluginManager.plugins; if (plugins.length === 0) { - // Fast path for the common case of no registered plugins: `_runAction` - // already returns a shared, replaying stream, so hand it back as-is. - // Skips the `runPluginChain` closures and a second `shareReplay` layer - // over the same value. + // Most apps register no plugins, so go straight to `_runAction`. It + // already gives back a shared, replaying stream, so there's nothing left + // to wrap - no `runPluginChain` closures, no second `shareReplay`. // - // No injection context here: an `@Action` handler is not one (only plugin - // functions get one, see `PluginManager`), and with no plugins there is - // nothing that needs it. + // Nothing sets up an injection context here, on purpose: `@Action` + // handlers don't run in one (only plugin functions do - see + // `PluginManager`), and with no plugins nothing needs one anyway. return this._runAction(action); } @@ -120,15 +119,16 @@ export class InternalDispatcher { action ); - // A plugin can wrap `_runAction`'s result in its own un-shared `.pipe(...)`, - // so multicast the chain output for the eager subscriber and the caller. + // A plugin might tack its own `.pipe(...)` onto `_runAction`'s result, and + // that isn't shared, so `shareReplay` the chain here - the eager subscriber + // and the caller both need to see the same single run. return dispatched$.pipe(shareReplay(DISPATCH_SHARE_REPLAY)); } private _runAction(action: any): Observable<ɵPlainObject> { - // Wait for this action's result on `_actionResults` and turn it into what - // `dispatch()` should emit. Also push the result status back onto the main - // action stream so `Actions` listeners (`ofActionSuccessful`, etc.) see it. + // Wait on `_actionResults` for this action to finish, then turn that into + // what `dispatch()` emits. We also re-broadcast the result on the main + // action stream so `Actions` listeners like `ofActionSuccessful` pick it up. const result$ = new Observable<ɵPlainObject>(subscriber => this._actionResults.subscribe({ next: ctx => { @@ -140,7 +140,7 @@ export class InternalDispatcher { switch (ctx.status) { case ActionStatus.Successful: - // Emit the state, as plugins use it. + // Hand back the current state - plugins read it. subscriber.next(this._stateStream.getValue()); subscriber.complete(); break; @@ -155,16 +155,17 @@ export class InternalDispatcher { complete: () => !subscriber.closed && subscriber.complete() }) ).pipe( - // Share the single result with every subscriber (the eager one below, the - // plugin chain, `forkJoin`, and the caller). See `ACTION_RESULT_SHARE`: - // the reset flags are off so the result - a value or an error - keeps - // replaying to late subscribers instead of re-running the source. + // One result, several subscribers (the keep-alive one below, the plugin + // chain, `forkJoin`, the caller), so share it. `ACTION_RESULT_SHARE` turns + // every reset flag off, so whatever came out - a value or an error - gets + // replayed to anyone who subscribes late instead of running again. share(ACTION_RESULT_SHARE) ); - // Subscribe now, before `Dispatched` is sent, so a synchronous handler's - // result isn't missed. Consumers get any error from the shared result above; - // this subscription just keeps the source alive, so ignore errors here. + // Subscribe before firing `Dispatched` below: a synchronous handler can + // finish right away and we'd miss the result otherwise. This subscription is + // only here to keep the stream running - the caller gets errors from the + // shared result above, so ignore them here. result$.subscribe(IGNORE_ERRORS); this._actions.next({ action, status: ActionStatus.Dispatched }); @@ -174,14 +175,14 @@ export class InternalDispatcher { } /** - * Runs the plugin functions as middleware around `handler`, left to right: each - * plugin gets `(state, action, next)` and calls `next(state, action)` to pass - * control on. Once the plugins are done, `handler(state, action)` runs. + * Runs the plugins as middleware around `handler`, left to right. Each plugin + * gets `(state, action, next)` and calls `next(state, action)` to hand off to + * the next one. After the last plugin, `handler(state, action)` runs. * - * This runner adds no injection context of its own: functional plugins that - * need one are wrapped when they are registered (see `PluginManager`), so with - * only class plugins (or none) `handler` - the action-handler invocation - runs - * without one. + * This function doesn't set up an injection context. Functional plugins that + * need `inject()` get wrapped in one at registration time (see `PluginManager`), + * so when every plugin is class-based (or there are none) the action handler + * runs without one. */ function runPluginChain( destroyRef: DestroyRef, @@ -192,7 +193,7 @@ function runPluginChain( ): Observable { const runFrom = (index: number, currentState: any, currentAction: any): any => { if (destroyRef.destroyed) { - // The injector is gone (maybe a plugin destroyed it) — do nothing. + // Injector's already gone (a plugin might have torn it down), so bail. return EMPTY; } diff --git a/packages/store/src/internal/fallback-subscriber.ts b/packages/store/src/internal/fallback-subscriber.ts index 3d94204d4..f81233c3f 100644 --- a/packages/store/src/internal/fallback-subscriber.ts +++ b/packages/store/src/internal/fallback-subscriber.ts @@ -8,11 +8,10 @@ export function fallbackSubscriber(ngZone: NgZone) { let subscription: Subscription | null = source.subscribe({ error: error => { ngZone.runOutsideAngular(() => { - // This is necessary to schedule a microtask to ensure that synchronous - // errors are not reported before the real subscriber arrives. If an error - // is thrown synchronously in any action, it will be reported to the error - // handler regardless. Since RxJS reports unhandled errors asynchronously, - // implementing a microtask ensures that we are also safe in this scenario. + // Defer to a microtask so a synchronous error doesn't fire before the + // real subscriber has attached. If an action throws synchronously the + // error still reaches the error handler either way; RxJS reports + // unhandled errors a tick later, so waiting one tick keeps us in sync. queueMicrotask(() => { if (subscription) { executeUnhandledCallback(error); @@ -23,7 +22,7 @@ export function fallbackSubscriber(ngZone: NgZone) { }); return new Observable(subscriber => { - // Now that there is a real subscriber, we can unsubscribe our pro-active subscription + // A real subscriber turned up, so drop the eager stand-in we started with. subscription?.unsubscribe(); subscription = null; diff --git a/packages/store/src/internal/state-factory.ts b/packages/store/src/internal/state-factory.ts index 13165e3dd..aae79fd65 100644 --- a/packages/store/src/internal/state-factory.ts +++ b/packages/store/src/internal/state-factory.ts @@ -225,16 +225,19 @@ export class StateFactory { function callback() { ngxsUnhandledErrorHandler.handleError(error, { action }); } - // An arrow function here would share the `[[Context]]` of the enclosing `error =>` - // arrow function, which captures `this` (StateFactory), causing the error object to - // retain the entire instance. A named function declaration creates its own context and - // V8 only captures variables it actually references — `ngxsUnhandledErrorHandler`, - // `error`, `action` — so `this` is never captured. `.bind(null)` goes one step further: - // a JSBoundFunction has no `[[Context]]` slot at all, fully severing the retention chain. - // This matters because the callback is stored as a value in the - // `ɵɵunhandledRxjsErrorCallbacks` WeakMap, keyed by the error object. The error may be - // held by third-party code (e.g. error trackers, logging) for an indeterminate duration, - // and we have no control over when that key is released. + // Keep this a named function, not an arrow. An arrow shares the + // enclosing `error =>` scope, which closes over `this` (the + // StateFactory), so the error object would end up retaining the + // whole instance. A named function only closes over what it + // actually uses (`ngxsUnhandledErrorHandler`, `error`, `action`), + // never `this`. `.bind(null)` goes one further: a bound function + // has no scope slot at all, so the retention chain is fully cut. + // + // This matters because the callback is stored in the + // `ɵɵunhandledRxjsErrorCallbacks` WeakMap keyed by the error, and + // the error can be held by third-party code (error trackers, + // logging) for as long as it likes - we don't control when it's + // released. const handleableError = assignUnhandledCallback(error, callback.bind(null)); subscriber.next({ action, diff --git a/packages/store/src/internal/unhandled-rxjs-error-callback.ts b/packages/store/src/internal/unhandled-rxjs-error-callback.ts index 592c61d88..2a4e8c86e 100644 --- a/packages/store/src/internal/unhandled-rxjs-error-callback.ts +++ b/packages/store/src/internal/unhandled-rxjs-error-callback.ts @@ -33,9 +33,8 @@ export function executeUnhandledCallback(error: any) { } export function assignUnhandledCallback(error: any, callback: VoidFunction) { - // 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')`. + // The error can be anything - `throwError('My Error')` throws a plain string, + // for one. WeakMap keys have to be objects, so only handle it when it is one. if (error && typeof error === 'object') { let hasBeenCalled = false; ɵɵunhandledRxjsErrorCallbacks.set(error, () => { diff --git a/packages/store/src/ngxs-unhandled-error-handler.ts b/packages/store/src/ngxs-unhandled-error-handler.ts index bcaa78aa3..46d76186a 100644 --- a/packages/store/src/ngxs-unhandled-error-handler.ts +++ b/packages/store/src/ngxs-unhandled-error-handler.ts @@ -10,17 +10,14 @@ export class NgxsUnhandledErrorHandler { private _errorHandler = inject(ErrorHandler); /** - * The `_unhandledErrorContext` is left unused internally since we do not - * require it for internal operations. However, developers who wish to provide - * their own custom error handler may utilize this context information. + * We don't use `_unhandledErrorContext` ourselves - it's here for custom + * error handlers that want the extra context. */ handleError(error: any, _unhandledErrorContext: NgxsUnhandledErrorContext): void { - // In order to avoid duplicate error handling, it is necessary to leave - // the Angular zone to ensure that errors are not caught twice. The `handleError` - // method may contain a `throw error` statement, which is used to re-throw the error. - // If the error is re-thrown within the Angular zone, it will be caught again by the - // Angular zone. By default, `@angular/core` leaves the Angular zone when invoking - // `handleError` (see `_callAndReportToErrorHandler`). + // Run outside the Angular zone so a re-thrown error isn't caught twice. + // `handleError` often ends with `throw error` to re-throw; do that inside + // the zone and the zone catches it again. `@angular/core` already leaves the + // zone before calling `handleError` (see `_callAndReportToErrorHandler`). this._ngZone.runOutsideAngular(() => this._errorHandler.handleError(error)); } } diff --git a/packages/store/src/plugin-manager.ts b/packages/store/src/plugin-manager.ts index c430ca436..dab6bb2c8 100644 --- a/packages/store/src/plugin-manager.ts +++ b/packages/store/src/plugin-manager.ts @@ -33,19 +33,21 @@ export class PluginManager { const handlers: NgxsPlugin[] = this._pluginHandlers || []; const injector = this._injector; return handlers.map((plugin: NgxsPlugin) => { - // Class plugins inject their dependencies through the constructor, so they - // need no injection context at dispatch time. + // Class plugins get their deps from the constructor, so they don't need + // an injection context when they run. if (plugin.handle) { return plugin.handle.bind(plugin) as NgxsPluginFn; } - // Functional plugins can call `inject()` in their body while assembling - // the chain, so run them inside an injection context. Scoping it here - - // rather than around the whole chain - means that with only class plugins - // (or none) the action handler no longer runs in an injection context; - // `inject()` there belongs in a state's field initializers or constructor. - // (A functional plugin that calls `next()` synchronously still leaks its - // context into the handler - the chain runs inside this frame.) + // Functional plugins can call `inject()` in their body, so run them in an + // injection context. Doing it here, per plugin, instead of around the + // whole chain means that with only class plugins (or none) the action + // handler no longer runs in one - `inject()` in a handler belongs in the + // state's field initializers or constructor instead. + // + // Caveat: a functional plugin that calls `next()` synchronously still + // leaks its context into the handler, since the rest of the chain runs + // inside this frame. const pluginFn = plugin as unknown as NgxsPluginFn; return ((state, action, next) => runInInjectionContext(injector, () => pluginFn(state, action, next))) as NgxsPluginFn; diff --git a/packages/store/src/standalone-features/plugin.ts b/packages/store/src/standalone-features/plugin.ts index 17f8824f2..374558db0 100644 --- a/packages/store/src/standalone-features/plugin.ts +++ b/packages/store/src/standalone-features/plugin.ts @@ -28,9 +28,9 @@ export function withNgxsPlugin(plugin: Type | NgxsPluginFn): Environ ɵisPluginClass(plugin) ? { provide: NGXS_PLUGINS, useClass: plugin, multi: true } : { provide: NGXS_PLUGINS, useValue: plugin, multi: true }, - // We should inject the `PluginManager` to retrieve `NGXS_PLUGINS` and - // register those plugins. The plugin can be added from inside the child - // route, so the plugin manager should be re-injected. + // Force `PluginManager` to be created so it reads `NGXS_PLUGINS` and + // registers these plugins. Plugins can also come from a child route, so + // re-inject it there. provideEnvironmentInitializer(() => inject(PluginManager)) ]); } diff --git a/packages/store/src/utils/dispatch.ts b/packages/store/src/utils/dispatch.ts index ba0292655..42891e9e6 100644 --- a/packages/store/src/utils/dispatch.ts +++ b/packages/store/src/utils/dispatch.ts @@ -4,35 +4,35 @@ import { Observable } from 'rxjs'; import { Store } from '../store'; import { ActionDef } from '../actions/symbols'; -// Extends Observable so callers can subscribe to emission updates (e.g. progress, intermediate states), -// while implementing PromiseLike so the JS engine treats it as a thenable when `await` is used — -// without this dual nature, callers would have to choose upfront between async/await and reactive patterns. +// It's an Observable so you can subscribe for progress / intermediate states, +// and a PromiseLike so `await dispatch(...)` works too. Being both means callers +// don't have to pick one style up front. export class AsyncReturnType extends Observable implements PromiseLike { constructor(private dispatchResult$: Observable) { super(subscriber => dispatchResult$.subscribe(subscriber)); } - // Called automatically by the JS engine when `await dispatch(...)` is used. - // The PromiseLike contract requires full generics on TResult1/TResult2 to support - // promise chaining (e.g. `await dispatch(...).then(x => transform(x))`). + // The engine calls this on `await dispatch(...)`. The TResult1/TResult2 + // generics are what let you keep chaining, e.g. + // `await dispatch(...).then(x => transform(x))`. then( onfulfilled?: ((value: void) => TResult1 | PromiseLike) | null, onrejected?: ((reason: any) => TResult2 | PromiseLike) | null ): PromiseLike { return new Promise((resolve, reject) => { this.dispatchResult$.subscribe({ - // Propagate observable errors into the promise rejection path so - // `try/catch` around `await dispatch(...)` works as expected. + // Send observable errors down the reject path so `try/catch` around + // `await dispatch(...)` catches them. error: reject, - // Resolve on complete rather than on next emission — dispatch returns void, - // so the caller cares about the action finishing, not any intermediate values. + // Resolve on complete, not on the first emission: dispatch is void, so + // what matters is the action finishing, not any values along the way. complete: resolve }); }).then( - // Bridge void → undefined because PromiseLike resolves with no value, - // but `onfulfilled` still needs to be invoked to continue the chain correctly. + // `PromiseLike` resolves with nothing, so pass `undefined` through + // to `onfulfilled` to keep the chain going. onfulfilled ? () => onfulfilled(undefined) : undefined, - // Normalize null to undefined since Promise.then doesn't accept null for rejection handler. + // `Promise.then` won't take `null` for the reject handler, so map it to undefined. onrejected ?? undefined ); }