fix(@angular/ssr): handle nested redirects not explicitly defined in router config

[alan-agius4]

This commit ensures proper handling of nested redirects that are implicitly structured but not explicitly defined in the router configuration.

Closes #28903

[dgp1130]

Question: Is the intent here to make it so redirectTo: '/foo' is relative to the base URL while redirectTo: 'foo' or redirectTo: './foo' is relative to the current route?

I do wonder if redirectTo: 'foo' might be confusing and ambiguous for some devs, but it seems like this aligns with the app router at least?

[alan-agius4]

Question: Is the intent here to make it so redirectTo: '/foo' is relative to the base URL while redirectTo: 'foo' or redirectTo: './foo' is relative to the current route?
I do wonder if redirectTo: 'foo' might be confusing and ambiguous for some devs, but it seems like this aligns with the app router at least?

The Angular router offers various ways to define redirects, and the goal here is to support these options. I agree that it can be confusing and ambiguous—personally, I also found it difficult to fully grasp how redirects work. The fact that an empty string is interpreted as a trailing slash.

Example of Route Configuration:

const routes: Routes = [
  {
    path: '',
    pathMatch: 'full',
    redirectTo: 'some',  // Redirects to '/some'
  },
  {
    path: ':param',
    children: [
      {
        path: '', // This is treated as a /
        pathMatch: 'full',
        redirectTo: 'thing',  // Redirects to './thing' (relative to :param/) IE: :param/thing
      },
      {
        path: 'else',  // This is treated as /else
        pathMatch: 'full',
        redirectTo: 'thing',  // Redirects to './thing' (relative to :param/else) IE: :param/else/thing
      },
      {
        path: 'thing',
        component: DummyComponent,
      },
    ],
  },
];

I updated the logic to actually resolve these during extraction.

change logic

[dgp1130]

Oof, that router logic seems quite confusing. I recommend writing down some of these rough edges in a doc somewhere and we can potentially have a conversation with Andrew Scott when he gets back about finding opportunities for simplifying some of this stuff.

So far I've generally insisted to align with app router to maintain consistency, reduce scope by not changing the app router, and remove barriers to adoption. But longer term, it would be great to smooth over some of these rough edges in both routers now that we have more ownership over the SSR side of the problem.

[dgp1130]

Consider: Should this check go first? Otherwise constructUrlWithPlaceholders('foo', new URL('http://example.com/bar')) would be valid?

[dgp1130]

Consider: I got confused way longer than I should have thinking that baseUrl was a representation of baseHref. Maybe the naming could be more clear about the intent here.

Is this function only used for redirects? Maybe we should embed that into the naming like:

function buildRedirectUrl(toPath: string, fromUrl: URL): URL;

WDYT?

[dgp1130]

Question: I'm not following this example, what kind of router config is this coming from? How do we map :id to 123? The implementation makes it seem like we're zipping the two segment arrays and lining up segments at the same location, but that would match :id with user, not 123. Is there an error in this example or am I just misunderstanding something?

[alan-agius4]

The example is wrong, I’ll update it

[dgp1130]

Consider: I suspect this function is coupling two different problems together which might be unrelated:

1. Resolve a path from a particular URL.
2. Carry over URL parameters.

I wonder if there's an alternative design which might be able to decouple them. What if we:

1. Calculated redirects statically by looking at the route list and generating absolute templates.

   const serverRoutes = [
     {
       path: ':id',
       children: [
         { path: 'foo' },
         { path: 'bar', redirectTo: 'foo' }, // Assuming this redirects to `/:id/foo`, which I might be wrong about.
       ],
     },
   ];

   We could take this and generate a static list of absolute routes (['/:id/foo', '/:id/bar']), I imagine we already have something close to this if not this exact thing in the RouteTree.
2. Still statically, identify redirects between paths ({['/:id/bar']: '/:id/foo'}).
   • Whether this is done at build time or server initialization is up to you, point is we haven't see any real URLs yet.
3. Then at runtime, we would receive /1234/bar, match that as /:id/bar and extract { id: '1234' }.
4. Resolve the redirect to /:id/foo.
5. Apply the same params to get /1234/foo.

I think we already do 1. - 3. this way, but just spelling them out for clarity. My main point is separating 4. and 5. into distinct steps and allow us to decouple path resolution from applying parameters. WDYT?

[alan-agius4]

4 and 5 are already done separately in constructUrlWithPlaceholders

[dgp1130]

I think my point is that we've put 4. and 5. under a distinct abstraction (constructUrlWithPlaceholders), coupling them together. I was thinking it might make more sense to inline these two steps in src/app.ts, but renaming that function to something more meaningful might be fine (buildRedirectUrl, resolveRedirect, etc.) since that would better define this abstraction.

The other piece is that we are copying URL parameters from one string to another (again, coupling the idea of applying parameters to resolving redirects). I'm thinking that it might be clearer to implement step 5. specifically as applying parameters from an independent object. Whether that's literally:

const params = extractParams(baseUrl);
const redirectUrl = applyParams(redirectUrlTemplate, params);

in constructUrlWithPlaceholders or if we can reuse parameters extracted previously and just apply them to the redirect URL.

This probably isn't super important, my intuition is that the intent would be easier to follow if we apply a given set of parameters to a URL template instead of trying to copy over specific segments from one URL to another. Just something to consider, it might not actually work out better in practice so I'm fine to defer to you in the end.

[dgp1130]

Question: What exactly is the goal with deleting these fields? Do we not want to leak them to users? A more descriptive comment might be helpful here.

[alan-agius4]

To avoid having to update a bunch of unit tests whenever we add a new field or make some changes.

I encountered this also in the preload PR.

[dgp1130]

Nit: Looks unused?

[dgp1130]

I think my point is that we've put 4. and 5. under a distinct abstraction (constructUrlWithPlaceholders), coupling them together. I was thinking it might make more sense to inline these two steps in src/app.ts, but renaming that function to something more meaningful might be fine (buildRedirectUrl, resolveRedirect, etc.) since that would better define this abstraction.

The other piece is that we are copying URL parameters from one string to another (again, coupling the idea of applying parameters to resolving redirects). I'm thinking that it might be clearer to implement step 5. specifically as applying parameters from an independent object. Whether that's literally:

const params = extractParams(baseUrl);
const redirectUrl = applyParams(redirectUrlTemplate, params);

in constructUrlWithPlaceholders or if we can reuse parameters extracted previously and just apply them to the redirect URL.

This probably isn't super important, my intuition is that the intent would be easier to follow if we apply a given set of parameters to a URL template instead of trying to copy over specific segments from one URL to another. Just something to consider, it might not actually work out better in practice so I'm fine to defer to you in the end.