docs: Add documentation and TODOs for cleanup (#43391)

* Document `currentUrlTree`, `browserUrlTree`, and `rawUrlTree`
* Add a couple `TODO` comments to investigate changes based on understanding

PR Close #43391

packages/router/src/router.ts

@@ -387,8 +387,42 @@ export const subsetMatchOptions: IsActiveMatchOptions = {
  */
 @Injectable()
 export class Router {
+  /**
+   * Represents the activated `UrlTree` that the `Router` is configured to handle (through
+   * `UrlHandlingStrategy`). That is, after we find the route config tree that we're going to
+   * activate, run guards, and are just about to activate the route, we set the currentUrlTree.
+   *
+   * This should match the `browserUrlTree` when a navigation succeeds. If the
+   * `UrlHandlingStrategy.shouldProcessUrl` is `false`, only the `browserUrlTree` is updated.
+   */
   private currentUrlTree: UrlTree;
+  /**
+   * Meant to represent the entire browser url after a successful navigation. In the life of a
+   * navigation transition:
+   * 1. The rawUrl represents the full URL that's being navigated to
+   * 2. We apply redirects, which might only apply to _part_ of the URL (due to
+   * `UrlHandlingStrategy`).
+   * 3. Right before activation (because we assume activation will succeed), we update the
+   * rawUrlTree to be a combination of the urlAfterRedirects (again, this might only apply to part
+   * of the initial url) and the rawUrl of the transition (which was the original navigation url in
+   * its full form).
+   */
   private rawUrlTree: UrlTree;
+  /**
+   * Meant to represent the part of the browser url that the `Router` is set up to handle (via the
+   * `UrlHandlingStrategy`). This value is updated immediately after the browser url is updated (or
+   * the browser url update is skipped via `skipLocationChange`). With that, note that
+   * `browserUrlTree` _may not_ reflect the actual browser URL for two reasons:
+   *
+   * 1. `UrlHandlingStrategy` only handles part of the URL
+   * 2. `skipLocationChange` does not update the browser url.
+   *
+   * So to reiterate, `browserUrlTree` only represents the Router's internal understanding of the
+   * current route, either before guards with `urlUpdateStrategy === 'eager'` or right before
+   * activation with `'deferred'`.
+   *
+   * This should match the `currentUrlTree` when the navigation succeeds.
+   */
   private browserUrlTree: UrlTree;
   private readonly transitions: BehaviorSubject<NavigationTransition>;
   private navigations: Observable<NavigationTransition>;
@@ -618,13 +652,22 @@ export class Router {
                      switchMap(t => {
                        const urlTransition = !this.navigated ||
                            t.extractedUrl.toString() !== this.browserUrlTree.toString();
+                       /* || this.browserUrlTree.toString() !== this.currentUrlTree.toString() */
+                       // TODO(atscott): Run TGP to see if the above change can be made. There are
+                       // situations where a navigation is canceled _after_ browserUrlTree is
+                       // updated. For example, urlUpdateStrategy === 'eager': if a new
+                       // navigation happens (i.e. in a guard), this would cause the router to
+                       // be in an invalid state of tracking.
                        const processCurrentUrl =
                            (this.onSameUrlNavigation === 'reload' ? true : urlTransition) &&
                            this.urlHandlingStrategy.shouldProcessUrl(t.rawUrl);
 
                        // If the source of the navigation is from a browser event, the URL is
                        // already updated. We already need to sync the internal state.
                        if (isBrowserTriggeredNavigation(t.source)) {
+                         // TODO(atscott): this should be `t.extractedUrl`. The `browserUrlTree`
+                         // should only be the part of the URL that is handled by the router. In
+                         // addition, this should only be done if we process the current url.
                          this.browserUrlTree = t.rawUrl;
                        }
 
@@ -669,6 +712,13 @@ export class Router {
                                if (this.urlUpdateStrategy === 'eager') {
                                  if (!t.extras.skipLocationChange) {
                                    this.setBrowserUrl(t.urlAfterRedirects, t);
+                                   // TODO(atscott): The above line is incorrect. It sets the url to
+                                   // only the part that is handled by the router. It should merge
+                                   // that with the rawUrl so the url includes segments not handled
+                                   // by the router:
+                                   //  const rawUrl = this.urlHandlingStrategy.merge(
+                                   //      t.urlAfterRedirects, t.rawUrl);
+                                   //  this.setBrowserUrl(rawUrl, t);
                                  }
                                  this.browserUrlTree = t.urlAfterRedirects;
                                }
@@ -840,7 +890,7 @@ export class Router {
                      tap((t: NavigationTransition) => {
                        this.currentUrlTree = t.urlAfterRedirects;
                        this.rawUrlTree =
-                           this.urlHandlingStrategy.merge(this.currentUrlTree, t.rawUrl);
+                           this.urlHandlingStrategy.merge(t.urlAfterRedirects, t.rawUrl);
 
                        (this as {routerState: RouterState}).routerState = t.targetRouterState!;
 
@@ -992,6 +1042,13 @@ export class Router {
 
   private getTransition(): NavigationTransition {
     const transition = this.transitions.value;
+    // TODO(atscott): This comment doesn't make it clear why this value needs to be set. In the case
+    // described below (where we don't handle previous or current url), the `browserUrlTree` is set
+    // to the `urlAfterRedirects` value. However, these values *are already the same* because of the
+    // line below. So it seems that we should be able to remove the line below and the line where
+    // `browserUrlTree` is updated when we aren't handling any part of the navigation url.
+    // Run TGP to confirm that this can be done.
+
     // This value needs to be set. Other values such as extractedUrl are set on initial navigation
     // but the urlAfterRedirects may not get set if we aren't processing the new URL *and* not
     // processing the previous URL.