Revert "refactor(router): Add support for precommitHandler in Navigation integration"

This reverts commit 522fa71.

Author: atscott

packages/core/test/bundling/router/bundle.golden_symbols.json

@@ -29,7 +29,6 @@
       "BROWSER_MODULE_PROVIDERS",
       "BaseRouteReuseStrategy",
       "BeforeActivateRoutes",
-      "BeforeRoutesRecognized",
       "BehaviorSubject",
       "BrowserDomAdapter",
       "BrowserPlatformLocation",

packages/router/src/events.ts

@@ -630,20 +630,15 @@ export class Scroll {
 }
 
 export class BeforeActivateRoutes {}
-export class BeforeRoutesRecognized {}
 export class RedirectRequest {
   constructor(
     readonly url: UrlTree,
     readonly navigationBehaviorOptions: NavigationBehaviorOptions | undefined,
   ) {}
 }
-export type PrivateRouterEvents = BeforeActivateRoutes | RedirectRequest | BeforeRoutesRecognized;
+export type PrivateRouterEvents = BeforeActivateRoutes | RedirectRequest;
 export function isPublicRouterEvent(e: Event | PrivateRouterEvents): e is Event {
-  return (
-    !(e instanceof BeforeActivateRoutes) &&
-    !(e instanceof RedirectRequest) &&
-    !(e instanceof BeforeRoutesRecognized)
-  );
+  return !(e instanceof BeforeActivateRoutes) && !(e instanceof RedirectRequest);
 }
 
 /**

packages/router/src/navigation_transition.ts

@@ -27,7 +27,6 @@ import {createRouterState} from './create_router_state';
 import {INPUT_BINDER} from './directives/router_outlet';
 import {
   BeforeActivateRoutes,
-  BeforeRoutesRecognized,
   Event,
   GuardsCheckEnd,
   GuardsCheckStart,
@@ -297,11 +296,6 @@ export interface Navigation {
    * This function is a no-op if the navigation is beyond the point where it can be aborted.
    */
   readonly abort: () => void;
-
-  /** @internal */
-  routesRecognizeHandler: {deferredHandle?: Promise<void>};
-  /** @internal */
-  beforeActivateHandler: {deferredHandle?: Promise<void>};
 }
 
 const noop = () => {};
@@ -325,9 +319,6 @@ export interface NavigationTransition {
   targetRouterState: RouterState | null;
   guards: Checks;
   guardsResult: GuardResult | null;
-
-  routesRecognizeHandler: {deferredHandle?: Promise<void>};
-  beforeActivateHandler: {deferredHandle?: Promise<void>};
 }
 
 export const NAVIGATION_ERROR_HANDLER = new InjectionToken<
@@ -427,9 +418,6 @@ export class NavigationTransitions {
         guards: {canActivateChecks: [], canDeactivateChecks: []},
         guardsResult: null,
         id,
-
-        routesRecognizeHandler: {},
-        beforeActivateHandler: {},
       });
     });
   }
@@ -484,9 +472,6 @@ export class NavigationTransitions {
                     previousNavigation: null,
                   },
               abort: () => abortController.abort(),
-
-              routesRecognizeHandler: t.routesRecognizeHandler,
-              beforeActivateHandler: t.beforeActivateHandler,
             });
             const urlTransition =
               !router.navigated || this.isUpdatingInternalState() || this.isUpdatedBrowserUrl();
@@ -549,16 +534,7 @@ export class NavigationTransitions {
                     nav!.finalUrl = t.urlAfterRedirects;
                     return nav;
                   });
-                  this.events.next(new BeforeRoutesRecognized());
-                }),
 
-                switchMap((value) =>
-                  from(
-                    overallTransitionState.routesRecognizeHandler.deferredHandle ?? of(void 0),
-                  ).pipe(map(() => value)),
-                ),
-
-                tap(() => {
                   // Fire RoutesRecognized
                   const routesRecognized = new RoutesRecognized(
                     t.id,
@@ -759,7 +735,7 @@ export class NavigationTransitions {
           // this is done as a safety measure to avoid surfacing this error (#49567).
           take(1),
 
-          switchMap(async (t: NavigationTransition) => {
+          map((t: NavigationTransition) => {
             const targetRouterState = createRouterState(
               router.routeReuseStrategy,
               t.targetSnapshot!,
@@ -775,12 +751,7 @@ export class NavigationTransitions {
             if (!shouldContinueNavigation()) {
               return;
             }
-            if (overallTransitionState.beforeActivateHandler.deferredHandle) {
-              await overallTransitionState.beforeActivateHandler.deferredHandle;
-            }
-            if (!shouldContinueNavigation()) {
-              return;
-            }
+
             new ActivateRoutes(
               router.routeReuseStrategy,
               overallTransitionState.targetRouterState!,

packages/router/src/router.ts

@@ -293,7 +293,6 @@ export class Router {
         this.location.path(true),
         IMPERATIVE_NAVIGATION,
         this.stateManager.restoredState(),
-        {replaceUrl: true},
       );
     }
   }
@@ -308,11 +307,9 @@ export class Router {
     // already patch onPopState, so location change callback will
     // run into ngZone
     this.nonRouterCurrentEntryChangeSubscription ??=
-      this.stateManager.registerNonRouterCurrentEntryChangeListener(
-        (url, state, source, extras) => {
-          this.navigateToSyncWithBrowser(url, source, state, extras);
-        },
-      );
+      this.stateManager.registerNonRouterCurrentEntryChangeListener((url, state, source) => {
+        this.navigateToSyncWithBrowser(url, source, state);
+      });
   }
 
   /**
@@ -326,8 +323,9 @@ export class Router {
     url: string,
     source: NavigationTrigger,
     state: RestoredState | null | undefined,
-    extras: NavigationExtras,
   ) {
+    const extras: NavigationExtras = {replaceUrl: true};
+
     // TODO: restoredState should always include the entire state, regardless
     // of navigationId. This requires a breaking change to update the type on
     // NavigationStart’s restoredState, which currently requires navigationId

packages/router/src/statemanager/navigation_state_manager.ts

@@ -20,14 +20,9 @@ import {
   ɵPRECOMMIT_HANDLER_SUPPORTED as PRECOMMIT_HANDLER_SUPPORTED,
 } from '@angular/common';
 import {StateManager} from './state_manager';
-import {
-  NavigationExtras,
-  RestoredState,
-  Navigation as RouterNavigation,
-} from '../navigation_transition';
+import {RestoredState, Navigation as RouterNavigation} from '../navigation_transition';
 import {
   BeforeActivateRoutes,
-  BeforeRoutesRecognized,
   isRedirectingEvent,
   NavigationCancel,
   NavigationCancellationCode,
@@ -37,6 +32,7 @@ import {
   NavigationStart,
   NavigationTrigger,
   PrivateRouterEvents,
+  RoutesRecognized,
 } from '../events';
 import {Subject, SubscriptionLike} from 'rxjs';
 import {UrlTree} from '../url_tree';
@@ -89,7 +85,6 @@ export class NavigationStateManager extends StateManager {
     /** Function to resolve the intercepted navigation event. */
     resolveHandler?: (v: void) => void;
     navigationEvent?: NavigateEvent;
-    commitUrl?: () => Promise<void>;
   } = {};
 
   /**
@@ -129,18 +124,12 @@ export class NavigationStateManager extends StateManager {
       url: string,
       state: RestoredState | null | undefined,
       trigger: NavigationTrigger,
-      extras: NavigationExtras,
     ) => void,
   ): SubscriptionLike {
     this.activeHistoryEntry = this.navigation.currentEntry!;
     this.nonRouterEntryChangeListener = this.nonRouterCurrentEntryChangeSubject.subscribe(
       ({path, state}) => {
-        listener(
-          path,
-          state,
-          'popstate',
-          !this.precommitHandlerSupported ? {replaceUrl: true} : {},
-        );
+        listener(path, state, 'popstate');
       },
     );
     return this.nonRouterEntryChangeListener;
@@ -161,46 +150,20 @@ export class NavigationStateManager extends StateManager {
     this.currentNavigation = {...this.currentNavigation, routerTransition: transition};
     if (e instanceof NavigationStart) {
       this.updateStateMemento();
-      // If we have precommit handler support, we can create a navigation
-      // immediately and redirect it later.
-      if (this.precommitHandlerSupported) {
-        this.maybeCreateNavigationForTransition(transition);
-      }
     } else if (e instanceof NavigationSkipped) {
       this.finishNavigation();
       this.commitTransition(transition);
-    } else if (e instanceof BeforeRoutesRecognized) {
-      transition.routesRecognizeHandler.deferredHandle = new Promise<void>(async (resolve) => {
-        if (this.urlUpdateStrategy === 'eager') {
-          try {
-            this.maybeCreateNavigationForTransition(transition);
-            await this.currentNavigation.commitUrl?.();
-          } catch {
-            // If commit fails (e.g., precommitHandler rejects), abort.
-            // The AbortSignal will notify
-            return;
-          }
-        }
-        resolve();
-      });
+    } else if (e instanceof RoutesRecognized) {
+      if (this.urlUpdateStrategy === 'eager' && !transition.extras.skipLocationChange) {
+        this.createNavigationForTransition(transition);
+      }
     } else if (e instanceof BeforeActivateRoutes) {
-      transition.beforeActivateHandler.deferredHandle = new Promise<void>(async (resolve) => {
-        // If URL update strategy is 'deferred', commit the URL now (before activation).
-        if (this.urlUpdateStrategy === 'deferred') {
-          try {
-            this.maybeCreateNavigationForTransition(transition);
-            await this.currentNavigation.commitUrl?.();
-          } catch {
-            return;
-          }
-        }
-        // Commit the internal router state.
-        this.commitTransition(transition);
-        resolve();
-      });
+      // Commit the internal router state.
+      this.commitTransition(transition);
+      if (this.urlUpdateStrategy === 'deferred' && !transition.extras.skipLocationChange) {
+        this.createNavigationForTransition(transition);
+      }
     } else if (e instanceof NavigationCancel || e instanceof NavigationError) {
-      // TODO(atscott): We want to keep the navigation event on redirects if
-      // the URL wasn't committed yet rather than cancelling it.
       void this.cancel(transition, e);
     } else if (e instanceof NavigationEnd) {
       const {resolveHandler, removeAbortListener} = this.currentNavigation;
@@ -220,19 +183,17 @@ export class NavigationStateManager extends StateManager {
     }
   }
 
-  private maybeCreateNavigationForTransition(transition: RouterNavigation) {
-    const {navigationEvent, commitUrl} = this.currentNavigation;
+  private createNavigationForTransition(transition: RouterNavigation) {
+    const {navigationEvent} = this.currentNavigation;
+    // If we are currently handling a traversal navigation, we do not need a new navigation for it
+    // because we are strictly restoring a previous state. If we are instead handling a navigation
+    // initiated outside the router, we do need to replace it with a router-triggered navigation
+    // to add the router-specific state.
     if (
-      // Presence of commitUrl function indicates the navigateEvent supports redirect
-      commitUrl ||
-      // If we are currently handling a traversal navigation, we do not need a new navigation for it
-      // because we are strictly restoring a previous state. If we are instead handling a navigation
-      // initiated outside the router, we do need to replace it with a router-triggered navigation
-      // to add the router-specific state.
-      (navigationEvent &&
-        (navigationEvent.navigationType === 'traverse' ||
-          navigationEvent.navigationType === 'reload') &&
-        this.eventAndRouterDestinationsMatch(navigationEvent, transition))
+      navigationEvent &&
+      (navigationEvent.navigationType === 'traverse' ||
+        navigationEvent.navigationType === 'reload') &&
+      this.eventAndRouterDestinationsMatch(navigationEvent, transition)
     ) {
       return;
     }
@@ -297,7 +258,6 @@ export class NavigationStateManager extends StateManager {
    * and resolving the post-commit handler promise. Clears the `currentNavigation` state.
    */
   private finishNavigation() {
-    this.currentNavigation.commitUrl?.();
     this.currentNavigation?.resolveHandler?.();
     this.currentNavigation = {};
   }
@@ -426,66 +386,18 @@ export class NavigationStateManager extends StateManager {
       resolve: resolveHandler,
       reject: rejectHandler,
     } = promiseWithResolvers<void>();
-
-    const {
-      promise: precommitHandlerPromise,
-      resolve: resolvePrecommitHandler,
-      reject: rejectPrecommitHandler,
-    } = promiseWithResolvers<void>();
-    this.currentNavigation.rejectNavigateEvent = () => {
-      event.signal.removeEventListener('abort', abortHandler);
-      rejectPrecommitHandler();
-      rejectHandler();
-    };
     this.currentNavigation.resolveHandler = () => {
       this.currentNavigation.removeAbortListener?.();
       resolveHandler();
     };
+    this.currentNavigation.rejectNavigateEvent = () => {
+      this.currentNavigation.removeAbortListener?.();
+      rejectHandler();
+    };
     // Prevent unhandled promise rejections from internal promises.
     handlerPromise.catch(() => {});
-    precommitHandlerPromise.catch(() => {});
     interceptOptions.handler = () => handlerPromise;
 
-    if (this.deferredCommitSupported(event)) {
-      const redirect = new Promise<
-        (url: string, options: {state: unknown; history?: 'push' | 'replace'}) => void
-      >((resolve) => {
-        // The `precommitHandler` option is not in the standard DOM types yet
-        (interceptOptions as any).precommitHandler = (controller: any) => {
-          resolve(controller.redirect.bind(controller));
-          return precommitHandlerPromise;
-        };
-      });
-      // `commitUrl` is a function that will be called by the router's lifecycle
-      // (e.g., in `BeforeRoutesRecognized` or `BeforeActivateRoutes` depending on `urlUpdateStrategy`)
-      // to actually perform the URL change via the Navigation API.
-      this.currentNavigation.commitUrl = async () => {
-        this.currentNavigation.commitUrl = undefined; // Ensure it's only called once.
-        const transition = this.currentNavigation.routerTransition;
-
-        // If not skipping location change, use the `redirect` function (from `precommitHandler`'s
-        // controller) to perform the URL update with the correct state and history action.
-        if (transition && !transition.extras.skipLocationChange) {
-          const internalPath = this.createBrowserPath(transition);
-          const history =
-            this.location.isCurrentPathEqualTo(internalPath) || !!transition.extras.replaceUrl
-              ? 'replace'
-              : 'push';
-          const state = {
-            ...transition.extras.state,
-            navigationId: transition.id,
-          };
-          // this might be a path or an actual URL depending on the baseHref
-          const pathOrUrl = this.location.prepareExternalUrl(internalPath);
-          (await redirect)(pathOrUrl, {state, history});
-        }
-        resolvePrecommitHandler();
-        // Wait for the Navigation API's own `committed` promise if available (part of transition object)
-        // This ensures we respect the browser's timing for when the commit actually happens.
-        return await this.navigation.transition?.committed;
-      };
-    }
-
     // Intercept the navigation event with the configured options.
     event.intercept(interceptOptions);
 
@@ -527,16 +439,6 @@ export class NavigationStateManager extends StateManager {
     const routerDestination = this.location.prepareExternalUrl(internalPath);
     return new URL(routerDestination, eventDestination.origin).href === eventDestination.href;
   }
-
-  private deferredCommitSupported(event: NavigateEvent): boolean {
-    return (
-      this.precommitHandlerSupported &&
-      // Cannot defer commit if not cancelable by the Navigation API's rules.
-      event.cancelable &&
-      // Deferring a traversal commit is currently problematic or not fully supported.
-      event.navigationType !== 'traverse'
-    );
-  }
 }
 
 /**