fix(router): 'createUrlTreeFromSnapshot' with empty paths and named outlets (#48734)

atscott · AndrewKushnir · commit a6b10f6e597e · 2023-01-18T10:46:44.000-08:00
The details of this commit are pretty thoroughly described in the tests and code comments. In short, it is sometimes ambiguous where to apply commands in a `SegmentGroup` tree that is created from an `ActivatedRoute` when dealing with empty paths. This adjusts the strategy to tolerate more ambiguity while still allowing developers to be explicit. This is a fix-forward for b/265215141 PR Close #48734
diff --git a/packages/router/src/create_url_tree.ts b/packages/router/src/create_url_tree.ts
@@ -388,6 +388,32 @@ function updateSegmentGroupChildren(
   } else {
     const outlets = getOutlets(commands);
     const children: {[key: string]: UrlSegmentGroup} = {};
+    // If the set of commands does not apply anything to the primary outlet and the child segment is
+    // an empty path primary segment on its own, we want to skip applying the commands at this
+    // level. Imagine the following config:
+    //
+    // {path: '', children: [{path: '**', outlet: 'popup'}]}.
+    //
+    // Navigation to /(popup:a) will activate the child outlet correctly Given a follow-up
+    // navigation with commands
+    // ['/', {outlets: {'popup': 'b'}}], we _would not_ want to apply the outlet commands to the
+    // root segment because that would result in
+    // //(popup:a)(popup:b) since the outlet command got applied one level above where it appears in
+    // the `ActivatedRoute` rather than updating the existing one.
+    //
+    // Because empty paths do not appear in the URL segments and the fact that the segments used in
+    // the output `UrlTree` are squashed to eliminate these empty paths where possible
+    // https://github.com/angular/angular/blob/13f10de40e25c6900ca55bd83b36bd533dacfa9e/packages/router/src/url_tree.ts#L755
+    // it can be hard to determine what is the right thing to do when applying commands to a
+    // `UrlSegmentGroup` that is created from an "unsquashed"/expanded `ActivatedRoute` tree.
+    // This code effectively "squashes" empty path primary routes when they have no siblings on
+    // the same level of the tree.
+    if (!outlets[PRIMARY_OUTLET] && segmentGroup.children[PRIMARY_OUTLET] &&
+        segmentGroup.numberOfChildren === 1 &&
+        segmentGroup.children[PRIMARY_OUTLET].segments.length === 0) {
+      return updateSegmentGroupChildren(
+          segmentGroup.children[PRIMARY_OUTLET], startIndex, commands);
+    }
 
     forEach(outlets, (commands, outlet) => {
       if (typeof commands === 'string') {
diff --git a/packages/router/test/create_url_tree.spec.ts b/packages/router/test/create_url_tree.spec.ts
@@ -115,31 +115,30 @@ describe('createUrlTree', async () => {
     expect(serializer.serialize(t)).toEqual('/%2Fone/two%2Fthree');
   });
 
-  it('should preserve secondary segments', async () => {
-    const p = serializer.parse('/a/11/b(right:c)');
-    const t = await createRoot(p, ['/a', 11, 'd']);
-    expect(serializer.serialize(t)).toEqual('/a/11/d(right:c)');
-  });
-
-  it('should support updating secondary segments (absolute)', async () => {
-    const p = serializer.parse('/a(right:b)');
-    const t = await createRoot(p, ['/', {outlets: {right: ['c']}}]);
-    expect(serializer.serialize(t)).toEqual('/a(right:c)');
-  });
+  describe('named outlets', async () => {
+    it('should preserve secondary segments', async () => {
+      const p = serializer.parse('/a/11/b(right:c)');
+      const t = await createRoot(p, ['/a', 11, 'd']);
+      expect(serializer.serialize(t)).toEqual('/a/11/d(right:c)');
+    });
 
-  it('should support updating secondary segments', async () => {
-    const p = serializer.parse('/a(right:b)');
-    const t = await createRoot(p, [{outlets: {right: ['c', 11, 'd']}}]);
-    expect(serializer.serialize(t)).toEqual('/a(right:c/11/d)');
-  });
+    it('should support updating secondary segments (absolute)', async () => {
+      const p = serializer.parse('/a(right:b)');
+      const t = await createRoot(p, ['/', {outlets: {right: ['c']}}]);
+      expect(serializer.serialize(t)).toEqual('/a(right:c)');
+    });
 
-  it('should support updating secondary segments (nested case)', async () => {
-    const p = serializer.parse('/a/(b//right:c)');
-    const t = await createRoot(p, ['a', {outlets: {right: ['d', 11, 'e']}}]);
-    expect(serializer.serialize(t)).toEqual('/a/(b//right:d/11/e)');
-  });
+    it('should support updating secondary segments', async () => {
+      const p = serializer.parse('/a(right:b)');
+      const t = await createRoot(p, [{outlets: {right: ['c', 11, 'd']}}]);
+      expect(serializer.serialize(t)).toEqual('/a(right:c/11/d)');
+    });
 
-  describe('', async () => {
+    it('should support updating secondary segments (nested case)', async () => {
+      const p = serializer.parse('/a/(b//right:c)');
+      const t = await createRoot(p, ['a', {outlets: {right: ['d', 11, 'e']}}]);
+      expect(serializer.serialize(t)).toEqual('/a/(b//right:d/11/e)');
+    });
     it('should support removing secondary outlet with prefix', async () => {
       const p = serializer.parse('/parent/(child//secondary:popup)');
       const t = await createRoot(p, ['parent', {outlets: {secondary: null}}]);
@@ -206,6 +205,106 @@ describe('createUrlTree', async () => {
       const t = await createRoot(p, ['parent', {outlets: {primary: 'child', secondary: null}}]);
       expect(serializer.serialize(t)).toEqual('/parent/child(rootSecondary:rootPopup)');
     });
+
+    describe('absolute navigations', () => {
+      it('with and pathless root', async () => {
+        router.resetConfig([
+          {
+            path: '',
+            children: [
+              {path: '**', outlet: 'left', component: class {}},
+            ],
+          },
+        ]);
+        await router.navigateByUrl('(left:search)');
+        expect(router.url).toEqual('/(left:search)');
+        expect(router.createUrlTree(['/', {outlets: {'left': ['projects', '123']}}]).toString())
+            .toEqual('/(left:projects/123)');
+      });
+      it('empty path parent and sibling with a path', async () => {
+        router.resetConfig([
+          {
+            path: '',
+            children: [
+              {path: 'x', component: class {}},
+              {path: '**', outlet: 'left', component: class {}},
+            ],
+          },
+        ]);
+        await router.navigateByUrl('/x(left:search)');
+        expect(router.url).toEqual('/x(left:search)');
+        expect(router.createUrlTree(['/', {outlets: {'left': ['projects', '123']}}]).toString())
+            .toEqual('/x(left:projects/123)');
+        // TODO(atscott): router.createUrlTree uses the "legacy" strategy based on the current
+        // UrlTree to generate new URLs. Once that changes, this can be `router.createUrlTree`
+        // again.
+        expect(createUrlTreeFromSnapshot(
+                   router.routerState.root.snapshot,
+                   [
+                     '/', {
+                       outlets: {
+                         'primary': [{
+                           outlets: {
+                             'left': ['projects', '123'],
+                           }
+                         }]
+                       }
+                     }
+                   ])
+                   .toString())
+            .toEqual('/x(left:projects/123)');
+      });
+
+      it('empty path parent and sibling', async () => {
+        router.resetConfig([
+          {
+            path: '',
+            children: [
+              {path: '', component: class {}},
+              {path: '**', outlet: 'left', component: class {}},
+              {path: '**', outlet: 'right', component: class {}},
+            ],
+          },
+        ]);
+        await router.navigateByUrl('/(left:search//right:define)');
+        expect(router.url).toEqual('/(left:search//right:define)');
+        expect(router.createUrlTree(['/', {outlets: {'left': ['projects', '123']}}]).toString())
+            .toEqual('/(left:projects/123//right:define)');
+      });
+      it('two pathless parents', async () => {
+        router.resetConfig([
+          {
+            path: '',
+            children: [{
+              path: '',
+              children: [
+                {path: '**', outlet: 'left', component: class {}},
+              ]
+            }],
+          },
+        ]);
+        await router.navigateByUrl('(left:search)');
+        expect(router.url).toEqual('/(left:search)');
+        expect(router.createUrlTree(['/', {outlets: {'left': ['projects', '123']}}]).toString())
+            .toEqual('/(left:projects/123)');
+      });
+
+      it('maintains structure when primary outlet is not pathless', async () => {
+        router.resetConfig([
+          {
+            path: 'a',
+            children: [
+              {path: '**', outlet: 'left', component: class {}},
+            ],
+          },
+          {path: '**', outlet: 'left', component: class {}},
+        ]);
+        await router.navigateByUrl('/a/(left:search)');
+        expect(router.url).toEqual('/a/(left:search)');
+        expect(router.createUrlTree(['/', {outlets: {'left': ['projects', '123']}}]).toString())
+            .toEqual('/a/(left:search)(left:projects/123)');
+      });
+    });
   });
 
   it('can navigate to nested route where commands is string', async () => {
