Skip to content

fix(@angular/ssr): resolve parameters in redirect targets - #34321

Open
bschaeublin wants to merge 2 commits into
angular:mainfrom
bschaeublin:fix/ssr-redirect-target-params
Open

bschaeublin wants to merge 2 commits into
angular:mainfrom
bschaeublin:fix/ssr-redirect-target-params

Conversation

@bschaeublin

Copy link
Copy Markdown
Contributor

PR Checklist

Please check to confirm your PR fulfills the following requirements:

PR Type

What kind of change does this PR introduce?

  • Bugfix
  • Feature
  • Code style update (formatting, local variables)
  • Refactoring (no functional changes, no api changes)
  • Build related changes
  • CI related changes
  • Documentation content changes
  • Other... Please describe:

What is the current behavior?

Follow-up to #34301 (comment).

Parameters in a redirectTo target are sent unchanged in the server's Location header, while the router replaces them:

Route Request Server Location Router navigates to
user/:id → profile/:id /user/7 /profile/:id /profile/7
search/:term → /find?q=:x /search/cats?x=dogs /find?q=:x /find?q=dogs

What is the new behavior?

The server resolves both, one commit each:

  1. Route parameters: during route extraction, :name in the redirect path becomes *<n>, the position of that parameter in the request path, which buildPathWithParams resolves at request time. Prerendered routes get the value from getPrerenderParams.
  2. Query placeholders of absolute redirects: q=:x takes the values of x from the request query.

Does this PR introduce a breaking change?

  • Yes
  • No

Other information

Without x in the request, the server redirects to /find. The router would go to /find?q=undefined.

Route parameters in the path of a `redirectTo` target, such as `:id` in `profile/:id`, were
sent unchanged in the `Location` header by the server, while the router replaces them with the
matching parameters of the current URL. The server now does the same.
…argets

Query parameters of an absolute `redirectTo` target whose value is a placeholder, such as
`q=:term`, were sent unchanged in the `Location` header by the server, while the router replaces
them with the values of the matching query parameters of the current URL. The server now does
the same.

Query parameters whose source parameter is missing in the current URL are left out, which
matches the `undefined` value that the router assigns to them.

@gemini-code-assist gemini-code-assist Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Code Review

This pull request enhances Angular SSR's redirect handling by resolving route and query parameter placeholders in redirect targets, aligning its behavior with the Angular router. It introduces buildQueryWithParams to resolve query placeholders, supports positional placeholders (*) in buildPathWithParams, and adds corresponding unit tests. The review feedback highlights a potential runtime error in buildPathWithParams when positional placeholder indices are out of bounds, suggesting a fallback to empty strings. Additionally, it recommends using .startsWith(':') instead of direct index access [0] in both replaceRedirectParams and buildQueryWithParams to safely handle empty strings.

Comment on lines 210 to 214
const resolvedParts = toPathParts.map((part, index) =>
toPathParts[index] === '*' ? fromPathParts[index] : part,
part === '*'
? fromPathParts[index]
: part.replace(POSITIONAL_PLACEHOLDER_REGEXP, (_, position) => fromPathParts[position]),
);

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

high

If fromPath has fewer segments than toPath, or if the positional placeholder index is out of bounds, fromPathParts[index] or fromPathParts[position] will be undefined. This will cause joinUrlParts to throw a TypeError when accessing part.length on the undefined value, or result in the string 'undefined' being inserted into the path. Adding a fallback to an empty string prevents these runtime errors.

Suggested change
const resolvedParts = toPathParts.map((part, index) =>
toPathParts[index] === '*' ? fromPathParts[index] : part,
part === '*'
? fromPathParts[index]
: part.replace(POSITIONAL_PLACEHOLDER_REGEXP, (_, position) => fromPathParts[position]),
);
const resolvedParts = toPathParts.map((part, index) =>
part === '*'
? (fromPathParts[index] ?? '')
: part.replace(POSITIONAL_PLACEHOLDER_REGEXP, (_, position) => fromPathParts[position] ?? ''),
);

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Not reachable: each */*<n> placeholder is derived from the position of a parameter in the matched route, and the route tree only matches when the request has a segment at each of those positions.

Comment thread packages/angular/ssr/src/routes/ng-routes.ts
Comment thread packages/angular/ssr/src/utils/url.ts
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant