refactor(router): Add warning for relativeLinkResolution: 'legacy' (#…

…45523) This change adds code to compute the corrected value for a link, regardless of the `relativeLinkResolution` value. Then, if the `relativeLinkResolution` is set to `legacy` and differs from the correct value, a warning is printed to the console in dev mode. This change is meant to assist in notifying developers that they have code which relies on the deprecated, broken behavior so they can fix and update the code before the `relativeLinkResolution` option is fully removed. PR Close #45523
angular · Apr 13, 2022 · 47e7a28 · 47e7a28
1 parent 598b759
commit 47e7a28
Show file tree

Hide file tree

Showing 7 changed files with 89 additions and 19 deletions.
diff --git a/aio/content/guide/deprecations.md b/aio/content/guide/deprecations.md
@@ -317,6 +317,8 @@ The `relativeLinkResolution` option is deprecated and being removed.
 In version 11, the default behavior was changed to the correct one.
 After `relativeLinkResolution` is removed, the correct behavior is always used without an option to use the broken behavior.
 
+A dev mode warning was added in v14 to warn if a created `UrlTree` relies on the `relativeLinkResolution: 'legacy'` option.
+
 <a id="loadChildren"></a>
 
 ### loadChildren string syntax

diff --git a/packages/router/src/create_url_tree.ts b/packages/router/src/create_url_tree.ts
@@ -24,13 +24,34 @@ export function createUrlTree(
     return tree(urlTree.root, urlTree.root, new UrlSegmentGroup([], {}), queryParams, fragment);
   }
 
-  const startingPosition = findStartingPosition(nav, urlTree, route);
+  function createTreeUsingPathIndex(lastPathIndex: number) {
+    const startingPosition =
+        findStartingPosition(nav, urlTree, route.snapshot?._urlSegment, lastPathIndex);
+
+    const segmentGroup = startingPosition.processChildren ?
+        updateSegmentGroupChildren(
+            startingPosition.segmentGroup, startingPosition.index, nav.commands) :
+        updateSegmentGroup(startingPosition.segmentGroup, startingPosition.index, nav.commands);
+    return tree(urlTree.root, startingPosition.segmentGroup, segmentGroup, queryParams, fragment);
+  }
+  // Note: The types should disallow `snapshot` from being `undefined` but due to test mocks, this
+  // may be the case. Since we try to access it at an earlier point before the refactor to add the
+  // warning for `relativeLinkResolution: 'legacy'`, this may cause failures in tests where it
+  // didn't before.
+  const result = createTreeUsingPathIndex(route.snapshot?._lastPathIndex);
+
+  // Check if application is relying on `relativeLinkResolution: 'legacy'`
+  if (typeof ngDevMode === 'undefined' || !!ngDevMode) {
+    const correctedResult = createTreeUsingPathIndex(route.snapshot?._correctedLastPathIndex);
+    if (correctedResult.toString() !== result.toString()) {
+      console.warn(
+          `relativeLinkResolution: 'legacy' is deprecated and will be removed in a future version of Angular. The link to ${
+              result.toString()} will change to ${
+              correctedResult.toString()} if the code is not updated before then.`);
+    }
+  }
 
-  const segmentGroup = startingPosition.processChildren ?
-      updateSegmentGroupChildren(
-          startingPosition.segmentGroup, startingPosition.index, nav.commands) :
-      updateSegmentGroup(startingPosition.segmentGroup, startingPosition.index, nav.commands);
-  return tree(urlTree.root, startingPosition.segmentGroup, segmentGroup, queryParams, fragment);
+  return result;
 }
 
 function isMatrixParams(command: any): boolean {
@@ -151,13 +172,14 @@ class Position {
   }
 }
 
-function findStartingPosition(nav: Navigation, tree: UrlTree, route: ActivatedRoute): Position {
+function findStartingPosition(
+    nav: Navigation, tree: UrlTree, segmentGroup: UrlSegmentGroup,
+    lastPathIndex: number): Position {
   if (nav.isAbsolute) {
     return new Position(tree.root, true, 0);
   }
 
-  if (route.snapshot._lastPathIndex === -1) {
-    const segmentGroup = route.snapshot._urlSegment;
+  if (lastPathIndex === -1) {
     // Pathless ActivatedRoute has _lastPathIndex === -1 but should not process children
     // see issue #26224, #13011, #35687
     // However, if the ActivatedRoute is the root we should process children like above.
@@ -166,9 +188,8 @@ function findStartingPosition(nav: Navigation, tree: UrlTree, route: ActivatedRo
   }
 
   const modifier = isMatrixParams(nav.commands[0]) ? 0 : 1;
-  const index = route.snapshot._lastPathIndex + modifier;
-  return createPositionApplyingDoubleDots(
-      route.snapshot._urlSegment, index, nav.numberOfDoubleDots);
+  const index = lastPathIndex + modifier;
+  return createPositionApplyingDoubleDots(segmentGroup, index, nav.numberOfDoubleDots);
 }
 
 function createPositionApplyingDoubleDots(

diff --git a/packages/router/src/recognize.ts b/packages/router/src/recognize.ts
@@ -18,6 +18,8 @@ import {getOutlet, sortByMatchingOutlets} from './utils/config';
 import {isImmediateMatch, match, noLeftoversInUrl, split} from './utils/config_matching';
 import {TreeNode} from './utils/tree';
 
+const NG_DEV_MODE = typeof ngDevMode === 'undefined' || !!ngDevMode;
+
 class NoMatch {}
 
 function newObservableError(e: unknown): Observable<RouterStateSnapshot> {
@@ -159,24 +161,31 @@ export class Recognizer {
 
     if (route.path === '**') {
       const params = segments.length > 0 ? last(segments)!.parameters : {};
+      const pathIndexShift = getPathIndexShift(rawSegment) + segments.length;
       snapshot = new ActivatedRouteSnapshot(
           segments, params, Object.freeze({...this.urlTree.queryParams}), this.urlTree.fragment,
           getData(route), getOutlet(route), route.component!, route,
-          getSourceSegmentGroup(rawSegment), getPathIndexShift(rawSegment) + segments.length,
-          getResolve(route));
+          getSourceSegmentGroup(rawSegment), pathIndexShift, getResolve(route),
+          // NG_DEV_MODE is used to prevent the getCorrectedPathIndexShift function from affecting
+          // production bundle size. This value is intended only to surface a warning to users
+          // depending on `relativeLinkResolution: 'legacy'` in dev mode.
+          (NG_DEV_MODE ? getCorrectedPathIndexShift(rawSegment) + segments.length :
+                         pathIndexShift));
     } else {
       const result = match(rawSegment, route, segments);
       if (!result.matched) {
         return null;
       }
       consumedSegments = result.consumedSegments;
       remainingSegments = result.remainingSegments;
+      const pathIndexShift = getPathIndexShift(rawSegment) + consumedSegments.length;
 
       snapshot = new ActivatedRouteSnapshot(
           consumedSegments, result.parameters, Object.freeze({...this.urlTree.queryParams}),
           this.urlTree.fragment, getData(route), getOutlet(route), route.component!, route,
-          getSourceSegmentGroup(rawSegment),
-          getPathIndexShift(rawSegment) + consumedSegments.length, getResolve(route));
+          getSourceSegmentGroup(rawSegment), pathIndexShift, getResolve(route),
+          (NG_DEV_MODE ? getCorrectedPathIndexShift(rawSegment) + consumedSegments.length :
+                         pathIndexShift));
     }
 
     const childConfig: Route[] = getChildConfig(route);
@@ -303,10 +312,20 @@ function getSourceSegmentGroup(segmentGroup: UrlSegmentGroup): UrlSegmentGroup {
 
 function getPathIndexShift(segmentGroup: UrlSegmentGroup): number {
   let s = segmentGroup;
-  let res = (s._segmentIndexShift ? s._segmentIndexShift : 0);
+  let res = s._segmentIndexShift ?? 0;
+  while (s._sourceSegment) {
+    s = s._sourceSegment;
+    res += s._segmentIndexShift ?? 0;
+  }
+  return res - 1;
+}
+
+function getCorrectedPathIndexShift(segmentGroup: UrlSegmentGroup): number {
+  let s = segmentGroup;
+  let res = s._segmentIndexShiftCorrected ?? s._segmentIndexShift ?? 0;
   while (s._sourceSegment) {
     s = s._sourceSegment;
-    res += (s._segmentIndexShift ? s._segmentIndexShift : 0);
+    res += s._segmentIndexShiftCorrected ?? s._segmentIndexShift ?? 0;
   }
   return res - 1;
 }

diff --git a/packages/router/src/router_state.ts b/packages/router/src/router_state.ts
@@ -284,6 +284,13 @@ export class ActivatedRouteSnapshot {
   _urlSegment: UrlSegmentGroup;
   /** @internal */
   _lastPathIndex: number;
+  /**
+   * @internal
+   *
+   * Used only in dev mode to detect if application relies on `relativeLinkResolution: 'legacy'`
+   * Should be removed in v16.
+   */
+  _correctedLastPathIndex: number;
   /** @internal */
   _resolve: ResolveData;
   /** @internal */
@@ -333,10 +340,11 @@ export class ActivatedRouteSnapshot {
       public outlet: string,
       /** The component of the route */
       public component: Type<any>|string|null, routeConfig: Route|null, urlSegment: UrlSegmentGroup,
-      lastPathIndex: number, resolve: ResolveData) {
+      lastPathIndex: number, resolve: ResolveData, correctedLastPathIndex?: number) {
     this.routeConfig = routeConfig;
     this._urlSegment = urlSegment;
     this._lastPathIndex = lastPathIndex;
+    this._correctedLastPathIndex = correctedLastPathIndex ?? lastPathIndex;
     this._resolve = resolve;
   }
 

diff --git a/packages/router/src/url_tree.ts b/packages/router/src/url_tree.ts
@@ -227,6 +227,13 @@ export class UrlSegmentGroup {
   _sourceSegment?: UrlSegmentGroup;
   /** @internal */
   _segmentIndexShift?: number;
+  /**
+   * @internal
+   *
+   * Used only in dev mode to detect if application relies on `relativeLinkResolution: 'legacy'`
+   * Should be removed in when `relativeLinkResolution` is removed.
+   */
+  _segmentIndexShiftCorrected?: number;
   /** The parent node in the url tree */
   parent: UrlSegmentGroup|null = null;
 

diff --git a/packages/router/src/utils/config_matching.ts b/packages/router/src/utils/config_matching.ts
@@ -111,6 +111,9 @@ function addEmptyPathsToChildrenIfNeeded(
       s._sourceSegment = segmentGroup;
       if (relativeLinkResolution === 'legacy') {
         s._segmentIndexShift = segmentGroup.segments.length;
+        if (typeof ngDevMode === 'undefined' || !!ngDevMode) {
+          s._segmentIndexShiftCorrected = consumedSegments.length;
+        }
       } else {
         s._segmentIndexShift = consumedSegments.length;
       }

diff --git a/packages/router/test/integration.spec.ts b/packages/router/test/integration.spec.ts
@@ -87,6 +87,7 @@ describe('Integration', () => {
 
       it('should not ignore empty paths in legacy mode',
          fakeAsync(inject([Router], (router: Router) => {
+           const warnSpy = spyOn(console, 'warn');
            router.relativeLinkResolution = 'legacy';
 
            const fixture = createRoot(router, RootCmp);
@@ -96,6 +97,10 @@ describe('Integration', () => {
 
            const link = fixture.nativeElement.querySelector('a');
            expect(link.getAttribute('href')).toEqual('/foo/bar/simple');
+           expect(warnSpy.calls.first().args[0])
+               .toContain('/foo/bar/simple will change to /foo/simple');
+           expect(warnSpy.calls.first().args[0])
+               .toContain('relativeLinkResolution: \'legacy\' is deprecated');
          })));
 
       it('should ignore empty paths in corrected mode',
@@ -5888,6 +5893,7 @@ describe('Integration', () => {
 
       it('should not ignore empty path when in legacy mode',
          fakeAsync(inject([Router], (router: Router) => {
+           const warnSpy = spyOn(console, 'warn');
            router.relativeLinkResolution = 'legacy';
 
            const fixture = createRoot(router, RootCmp);
@@ -5899,6 +5905,10 @@ describe('Integration', () => {
 
            const link = fixture.nativeElement.querySelector('a');
            expect(link.getAttribute('href')).toEqual('/lazy/foo/bar/simple');
+           expect(warnSpy.calls.first().args[0])
+               .toContain('/lazy/foo/bar/simple will change to /lazy/foo/simple');
+           expect(warnSpy.calls.first().args[0])
+               .toContain('relativeLinkResolution: \'legacy\' is deprecated');
          })));
 
       it('should ignore empty path when in corrected mode',