Skip to content

fix(@angular/ssr): support getPrerenderParams for wildcard routes #30072

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

alan-agius4
Copy link
Collaborator

@alan-agius4 alan-agius4 commented Apr 9, 2025

Handle getPrerenderParams return values when used with wildcard route paths.
Supports returning an array of path segments (e.g., ['category', '123']) for ** routes.

This enables more flexible prerendering configurations in server routes.

Example:

{
  path: '/product/**',
  renderMode: RenderMode.Prerender,
  async getPrerenderParams() {
    return [['category', '1'], ['category', '2']];
  }
}

@alan-agius4 alan-agius4 added action: review The PR is still awaiting reviews from at least one requested reviewer target: major This PR is targeted for the next major release labels Apr 9, 2025
@alan-agius4 alan-agius4 requested a review from dgp1130 April 9, 2025 09:10
@alan-agius4 alan-agius4 added target: patch This PR is targeted for the next patch release and removed target: major This PR is targeted for the next major release labels Apr 9, 2025
@alan-agius4 alan-agius4 force-pushed the ssr-wildcard branch 4 times, most recently from e583b67 to ff6d1b4 Compare April 9, 2025 10:04
@@ -63,7 +63,7 @@ export interface ServerRoutePrerender extends Omit<ServerRouteCommon, 'status'>
// @public
export interface ServerRoutePrerenderWithParams extends Omit<ServerRoutePrerender, 'fallback'> {
fallback?: PrerenderFallback;
getPrerenderParams: () => Promise<Record<string, string>[]>;
getPrerenderParams: () => Promise<(string[] | Record<string, string>)[]>;
Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Another option would be to add a different option something like getPrerenderPath or getPrerenderSegments

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

IMHO, reusing getPrerenderParams is reasonable. I think the intent behind this function is still the same whether it's filling in :foo or **. Though I'm a little worried that supporting the two in an xor fashion might be confusing for developers. If we can find a reasonable way to support :foo/**, then using a single function LGTM.

Copy link
Collaborator

@dgp1130 dgp1130 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What about a path like '/product/:first/**/:second'? Not sure if we allow segments after ** (reading this PR, seems like no?), but presumably we at least allow them before? Is this feature limited to either /:first xor /**?

I feel like we could support both with something like:

{
  path: '/product/:first/**/:second',
  renderMode: RenderMode.Prerender,
  async getPrerenderParams() {
    return [
      {'first': 'foo', __spread__: ['category', '1'], 'second': 'bar'},
      {'first': 'bar', __spread__: ['category', '2'], 'second': 'foo'},
    ];
  }
}

Not sure exactly the right way of identifying the spread array though. Maybe a Symbol imported from @angular/ssr?

Is there a better way of mixing ** and :foo syntax in the same path? Is :foo/** a rare enough pattern that we just don't care at this stage?

return value;
});
const routeWithResolvedParams = isParamsArray
? currentRoutePath.replace('**', params.join('/'))
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Consider: What if I render ['foo/bar']?

I remember we had a discussion on this previously, do we need to escape / characters returned by getPrerenderParams? Or did we decide not to do that?

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No we are not escaping any character. foo/bar will be rendered it's a valid URL.

@@ -63,7 +63,7 @@ export interface ServerRoutePrerender extends Omit<ServerRouteCommon, 'status'>
// @public
export interface ServerRoutePrerenderWithParams extends Omit<ServerRoutePrerender, 'fallback'> {
fallback?: PrerenderFallback;
getPrerenderParams: () => Promise<Record<string, string>[]>;
getPrerenderParams: () => Promise<(string[] | Record<string, string>)[]>;
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

IMHO, reusing getPrerenderParams is reasonable. I think the intent behind this function is still the same whether it's filling in :foo or **. Though I'm a little worried that supporting the two in an xor fashion might be confusing for developers. If we can find a reasonable way to support :foo/**, then using a single function LGTM.

@alan-agius4
Copy link
Collaborator Author

alan-agius4 commented Apr 10, 2025

What about a path like '/product/:first/**/:second'? Not sure if we allow segments after ** (reading this PR, seems like no?), but presumably we at least allow them before? Is this feature limited to either /:first xor /**?

I feel like we could support both with something like:

{
  path: '/product/:first/**/:second',
  renderMode: RenderMode.Prerender,
  async getPrerenderParams() {
    return [
      {'first': 'foo', __spread__: ['category', '1'], 'second': 'bar'},
      {'first': 'bar', __spread__: ['category', '2'], 'second': 'foo'},
    ];
  }
}

Not sure exactly the right way of identifying the spread array though. Maybe a Symbol imported from @angular/ssr?

Is there a better way of mixing ** and :foo syntax in the same path? Is :foo/** a rare enough pattern that we just don't care at this stage?

/product/:first/**/:second is not a valid Angular router route. Also, we did not allow this before.

Edit
Thinking a bit more, /product/:first/** would be a valid route, but in that case, the route can also be defined as as /product/**.

@dgp1130
Copy link
Collaborator

dgp1130 commented Apr 11, 2025

Thinking a bit more, /product/:first/** would be a valid route, but in that case, the route can also be defined as as /product/**.

What about /product/:first/owner/**? I don't think we can generally assume you can always collapse the :params into **?

@alan-agius4
Copy link
Collaborator Author

alan-agius4 commented Apr 11, 2025

Thinking a bit more, /product/:first/** would be a valid route, but in that case, the route can also be defined as as /product/**.

What about /product/:first/owner/**? I don't think we can generally assume you can always collapse the :params into **?

Yes, this is not handled at the moment, maybe the simplest would be to allow something like instead of changing the signature of the function

{
  path: '/product/:first/**',
  renderMode: RenderMode.Prerender,
  async getPrerenderParams() {
    return [
      {'first': 'foo', '**': 'category/1', 'second': 'bar'},
    ];
  }
}

Handle `getPrerenderParams` return values when used with wildcard route paths.
Supports returning an array of path segments (e.g., `['category', '123']`) for `**` routes.

This enables more flexible prerendering configurations in server routes.

Example:
```ts
{
  path: '/product/**',
  renderMode: RenderMode.Prerender,
  async getPrerenderParams() {
    return [['category', '1'], ['category', '2']];
  }
}
```

Closes angular#30035
@dgp1130
Copy link
Collaborator

dgp1130 commented Apr 11, 2025

Would it be worth getting some wider feedback from the team (Andrew or Jan in particular)? Maybe a quick one-pager would be enough to get some quick thoughts on the approach.

I'm open to {'**': 'foo/bar'}, but also can see that being a little awkward. Presumably /foo/:**/bar is not allowed as a path so there's no ambiguity (if not, it probably should be banned)?

@jkrems
Copy link
Contributor

jkrems commented Apr 14, 2025

I like ** as a key because it's easy to remember. More generally, I'm in favor of an API that is consistently either a Record or an array, especially because of the mix case. Replacing /prod/:id/** with /prod/** removes the route param as a readable property on the activated route (or from component inputs if enabled), so it's not just an "free" change.

Afaict there's no pre-existing name for the value captured from the wildcard.

@alan-agius4
Copy link
Collaborator Author

@atscott, any thoughts?

@dgp1130
Copy link
Collaborator

dgp1130 commented Apr 14, 2025

@AndrewKushnir might also have some quick thoughts here.

@AndrewKushnir
Copy link

@AndrewKushnir might also have some quick thoughts here.

Thinking a bit more, /product/:first/** would be a valid route, but in that case, the route can also be defined as as /product/**.

I'm wondering what are the use-cases for this feature? What would be "hiding" behind the ** symbol: would we only have static paths or dynamic segments (like :first). My thinking is that allowing dynamic segments might be confusing, since in the getPrerenderParams() function we'd need to return a set of values for keys that are not present in the URL, so as a developer I'd need to reconcile this with corresponding routes in the main routing file (also it'd be a challenge to keep things in sync).

Potentially, we can apply the following rules:

  • ** can only be present at the end of the URL pattern
  • ** can only be capturing static segments (i.e. a pattern like products/** is not valid for products/:id), so there is always a direct mapping between dynamic segments and corresponding values returned from the getPrerenderParams() function

However, I'm not sure if that would be too limiting for the use-cases that this feature is improving.

@alan-agius4
Copy link
Collaborator Author

I'm wondering what are the use-cases for this feature?

One example is when catch-all routes need to be prerendered. For instance, see this issue: #30035. In such cases, parameterized routes can't be used effectively because the nesting depth is arbitrary.

As for the rules you mentioned, those sound good to me, and I don't see them as restrictive.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
action: review The PR is still awaiting reviews from at least one requested reviewer area: @angular/ssr target: patch This PR is targeted for the next patch release
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants