From b7d21e209a3ef77264d41e13bcfbbdc767ced66e Mon Sep 17 00:00:00 2001 From: Andrew Scott Date: Tue, 14 Oct 2025 17:10:46 -0700 Subject: [PATCH] refactor(router): compress synchronous end to router navigation to single operator The end of the Router navigation is a block of synchronous logic that can be compressed into a single operator rather than splitting it across several, making it harder to step through. The only benefit from the split is automatic unsubscribe/cancellation, which we can replicate with an additional 'shouldContinue' check before proceeding. --- .../router/bundle.golden_symbols.json | 1 - packages/router/src/navigation_transition.ts | 75 ++++++++++--------- .../router/src/operators/activate_routes.ts | 22 ------ 3 files changed, 40 insertions(+), 58 deletions(-) diff --git a/packages/core/test/bundling/router/bundle.golden_symbols.json b/packages/core/test/bundling/router/bundle.golden_symbols.json index 965f6e60df2..0e532672425 100644 --- a/packages/core/test/bundling/router/bundle.golden_symbols.json +++ b/packages/core/test/bundling/router/bundle.golden_symbols.json @@ -385,7 +385,6 @@ "_stripOrigin", "_wasLastNodeCreated", "abortSignalToObservable", - "activateRoutes", "activeConsumer", "addAfterRenderSequencesForView", "addEmptyPathsToChildrenIfNeeded", diff --git a/packages/router/src/navigation_transition.ts b/packages/router/src/navigation_transition.ts index 58392d7409d..077e67b85e5 100644 --- a/packages/router/src/navigation_transition.ts +++ b/packages/router/src/navigation_transition.ts @@ -58,7 +58,7 @@ import { isRedirectingNavigationCancelingError, redirectingNavigationError, } from './navigation_canceling_error'; -import {activateRoutes} from './operators/activate_routes'; +import {ActivateRoutes} from './operators/activate_routes'; import {checkGuards} from './operators/check_guards'; import {recognize} from './operators/recognize'; import {resolveData} from './operators/resolve_data'; @@ -725,6 +725,7 @@ export class NavigationTransitions { switchTap(() => this.afterPreactivation()), + // TODO(atscott): Move this into the last block below. switchMap(() => { const {currentSnapshot, targetSnapshot} = overallTransitionState; const viewTransitionStarted = this.createViewTransition?.( @@ -740,36 +741,57 @@ export class NavigationTransitions { : of(overallTransitionState); }), + // Ensure that if some observable used to drive the transition doesn't + // complete, the navigation still finalizes This should never happen, but + // this is done as a safety measure to avoid surfacing this error (#49567). + take(1), + map((t: NavigationTransition) => { const targetRouterState = createRouterState( router.routeReuseStrategy, t.targetSnapshot!, t.currentRouterState, ); - this.currentTransition = overallTransitionState = {...t, targetRouterState}; + this.currentTransition = overallTransitionState = t = {...t, targetRouterState}; this.currentNavigation.update((nav) => { nav!.targetRouterState = targetRouterState; return nav; }); - return overallTransitionState; - }), - tap(() => { this.events.next(new BeforeActivateRoutes()); + if (!shouldContinueNavigation()) { + return; + } + + new ActivateRoutes( + router.routeReuseStrategy, + overallTransitionState.targetRouterState!, + overallTransitionState.currentRouterState, + (evt: Event) => this.events.next(evt), + this.inputBindingEnabled, + ).activate(this.rootContexts); + + if (!shouldContinueNavigation()) { + return; + } + + completedOrAborted = true; + this.currentNavigation.update((nav) => { + (nav as Writable).abort = noop; + return nav; + }); + this.lastSuccessfulNavigation.set(untracked(this.currentNavigation)); + this.events.next( + new NavigationEnd( + t.id, + this.urlSerializer.serialize(t.extractedUrl), + this.urlSerializer.serialize(t.urlAfterRedirects!), + ), + ); + this.titleStrategy?.updateTitle(t.targetRouterState!.snapshot); + t.resolve(true); }), - activateRoutes( - this.rootContexts, - router.routeReuseStrategy, - (evt: Event) => this.events.next(evt), - this.inputBindingEnabled, - ), - - // Ensure that if some observable used to drive the transition doesn't - // complete, the navigation still finalizes This should never happen, but - // this is done as a safety measure to avoid surfacing this error (#49567). - take(1), - takeUntil( abortSignalToObservable(abortController.signal).pipe( // Ignore aborts if we are already completed, canceled, or are in the activation stage (we have targetRouterState) @@ -785,23 +807,6 @@ export class NavigationTransitions { ), tap({ - next: (t: NavigationTransition) => { - completedOrAborted = true; - this.currentNavigation.update((nav) => { - (nav as Writable).abort = noop; - return nav; - }); - this.lastSuccessfulNavigation.set(untracked(this.currentNavigation)); - this.events.next( - new NavigationEnd( - t.id, - this.urlSerializer.serialize(t.extractedUrl), - this.urlSerializer.serialize(t.urlAfterRedirects!), - ), - ); - this.titleStrategy?.updateTitle(t.targetRouterState!.snapshot); - t.resolve(true); - }, complete: () => { completedOrAborted = true; }, @@ -849,6 +854,7 @@ export class NavigationTransitions { } }), catchError((e) => { + completedOrAborted = true; // If the application is already destroyed, the catch block should not // execute anything in practice because other resources have already // been released and destroyed. @@ -857,7 +863,6 @@ export class NavigationTransitions { return EMPTY; } - completedOrAborted = true; /* This error type is issued during Redirect, and is handled as a * cancellation rather than an error. */ if (isNavigationCancelingError(e)) { diff --git a/packages/router/src/operators/activate_routes.ts b/packages/router/src/operators/activate_routes.ts index ff46e0975ab..4f4fa548d79 100644 --- a/packages/router/src/operators/activate_routes.ts +++ b/packages/router/src/operators/activate_routes.ts @@ -6,35 +6,13 @@ * found in the LICENSE file at https://angular.dev/license */ -import {MonoTypeOperatorFunction} from 'rxjs'; -import {map} from 'rxjs/operators'; - import {ActivationEnd, ChildActivationEnd, Event} from '../events'; -import type {NavigationTransition} from '../navigation_transition'; import type {DetachedRouteHandleInternal, RouteReuseStrategy} from '../route_reuse_strategy'; import type {ChildrenOutletContexts} from '../router_outlet_context'; import {ActivatedRoute, advanceActivatedRoute, RouterState} from '../router_state'; import {nodeChildrenAsMap, TreeNode} from '../utils/tree'; let warnedAboutUnsupportedInputBinding = false; - -export const activateRoutes = ( - rootContexts: ChildrenOutletContexts, - routeReuseStrategy: RouteReuseStrategy, - forwardEvent: (evt: Event) => void, - inputBindingEnabled: boolean, -): MonoTypeOperatorFunction => - map((t) => { - new ActivateRoutes( - routeReuseStrategy, - t.targetRouterState!, - t.currentRouterState, - forwardEvent, - inputBindingEnabled, - ).activate(rootContexts); - return t; - }); - export class ActivateRoutes { constructor( private routeReuseStrategy: RouteReuseStrategy,