fix(@angular/ssr): handle nested redirects not explicitly defined in router config #28906
Conversation
alan-agius4
added
action: review
alan-agius4
force-pushed
the
redirects-nested
branch
4 times, most recently
from
698059e
to
d8e81e6
Compare
There was a problem hiding this comment.
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
force-pushed
the
redirects-nested
branch
from
d8e81e6
to
478f588
Compare
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. |
alan-agius4
force-pushed
the
redirects-nested
branch
from
478f588
to
926873a
Compare
alan-agius4
force-pushed
the
redirects-nested
branch
4 times, most recently
from
113d042
to
9ce8e3d
Compare
alan-agius4
force-pushed
the
redirects-nested
branch
2 times, most recently
from
e9f4568
to
7cec59d
Compare
…router config This commit ensures proper handling of nested redirects that are implicitly structured but not explicitly defined in the router configuration. Closes angular#28903
alan-agius4
force-pushed
the
redirects-nested
branch
from
7cec59d
to
3096858
Compare
There was a problem hiding this comment.
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.
| */ | ||
| export function addTrailingSlash(url: string): string { | ||
| // Check if the URL already end with a slash | ||
| return url[url.length - 1] === '/' ? url : `${url}/`; |
There was a problem hiding this comment.
Bug: I think this fails if url === ''?
There was a problem hiding this comment.
| return new URL(urlTemplate, baseUrl); | ||
| } | ||
|
|
||
| if (urlTemplate[0] !== '/') { |
There was a problem hiding this comment.
Consider: Should this check go first? Otherwise constructUrlWithPlaceholders('foo', new URL('http://example.com/bar')) would be valid?
| return new URL(urlTemplate, baseUrl); | ||
| } | ||
|
|
||
| if (urlTemplate[0] !== '/') { |
There was a problem hiding this comment.
Consider: urlTemplate.startsWith('/')
There was a problem hiding this comment.
[0] is considerably faster when checking for a single character
| * console.log(staticUrl.toString()); // Outputs: 'http://example.com/static/path' | ||
| * ``` | ||
| */ | ||
| export function constructUrlWithPlaceholders(urlTemplate: string, baseUrl: URL): URL { |
There was a problem hiding this comment.
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?
| * @example | ||
| * ```typescript | ||
| * const resolvedUrl = constructUrlWithPlaceholders('/:id/details', new URL('http://example.com/user/123')); | ||
| * console.log(resolvedUrl.toString()); // Outputs: 'http://example.com/123/details' |
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
The example is wrong, I’ll update it
| * console.log(staticUrl.toString()); // Outputs: 'http://example.com/static/path' | ||
| * ``` | ||
| */ | ||
| export function constructUrlWithPlaceholders(urlTemplate: string, baseUrl: URL): URL { |
There was a problem hiding this comment.
Consider: I suspect this function is coupling two different problems together which might be unrelated:
- Resolve a path from a particular URL.
- Carry over URL parameters.
I wonder if there's an alternative design which might be able to decouple them. What if we:
- Calculated redirects statically by looking at the route list and generating absolute templates.
We could take this and generate a static list of absolute routes (
const serverRoutes = [ { path: ':id', children: [ { path: 'foo' }, { path: 'bar', redirectTo: 'foo' }, // Assuming this redirects to `/:id/foo`, which I might be wrong about. ], }, ];
['/:id/foo', '/:id/bar']), I imagine we already have something close to this if not this exact thing in theRouteTree. - 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.
- Then at runtime, we would receive
/1234/bar, match that as/:id/barand extract{ id: '1234' }. - Resolve the redirect to
/:id/foo. - 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?
There was a problem hiding this comment.
4 and 5 are already done separately in constructUrlWithPlaceholders
| @@ -546,6 +563,14 @@ export async function extractRoutesAndCreateRouteTree( | |||
| metadata.redirectTo = joinUrlParts(baseHref, metadata.redirectTo); | |||
| } | |||
|
|
|||
| // Remove undefined fields | |||
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
This commit ensures proper handling of nested redirects that are implicitly structured but not explicitly defined in the router configuration.
Closes #28903