Repository navigation
refactor(@angular/ssr): parse route paths and redirect URLs into structured segments and query params - #34502
alan-agius4 wants to merge 2 commits into
Conversation
…ctured segments and query params
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request refactors URL and route parsing in Angular SSR by introducing structured types (PathSegment, QueryParam, ParsedUrl) and robust parsing/serialization functions to handle static, parameter, and wildcard segments. It also updates route redirection logic and switches positional wildcards to 0-based indexing. Feedback on these changes suggests optimizing performance by parsing path segments outside of the resolveParam callback to avoid repeated execution, and improving robustness by wrapping the serialization of the shared mutable serializationTree singleton in a try...finally block to ensure its state is always reset.
alan-agius4
added
the
action: review
alan-agius4
added
the
target: minor
…to structured segments and query params
dgp1130
left a comment
dgp1130
left a comment
There was a problem hiding this comment.
Thanks for looking into this! Hopefully makes manipulating these paths a little cleaner.
| * - `static`: A literal query parameter value (single string or repeated `string[]`). | ||
| * - `param`: A named query parameter placeholder (`?q=:term` -> `name: 'q'`, `param: 'term'`). | ||
| */ | ||
| export type QueryParam = |
There was a problem hiding this comment.
Consider: Would it be simpler to treat this like a Map<Name, Value> rather than Array<{name: string, value: string}? Does the order matter for query parameters?
| * - `param`: A named query parameter placeholder (`?q=:term` -> `name: 'q'`, `param: 'term'`). | ||
| */ | ||
| export type QueryParam = | ||
| | { readonly type: 'static'; readonly name: string; readonly value: string | string[] } |
There was a problem hiding this comment.
Question: What does the string[] case represent? Are you allowed to do ?q=foo&q=bar?
| readonly segments: readonly PathSegment[]; | ||
| readonly queryParams: readonly QueryParam[]; | ||
| readonly fragment: string | null; | ||
| readonly rawSuffix: string; |
There was a problem hiding this comment.
Question: Why do we need this? IIUC, it seems like this contains the ?foo or #bar text, which feels like it's just ignoring the parsing behavior we're intending to use here?
|
|
||
| describe('parseUrl and serializePathSegments', () => { | ||
| it('should parse path segments and query parameters into discriminated unions', () => { | ||
| const parsed = parseUrl('/user/:id/*/details/*0/**?q=:term&q2=:a&q2=:b&static=1#section'); |
There was a problem hiding this comment.
TIL: *0 is a thing we accept.
| if (POSITIONAL_WILDCARD_REGEX.test(segment)) { | ||
| return { type: 'wildcard', index: Number(segment.slice(1)) }; | ||
| } | ||
| } else if (segment[0] === ':' && segment.length > 1) { |
There was a problem hiding this comment.
Consider: I think this will parse /:/ as a static segment with the text :. Is that the behavior we want?
| */ | ||
| export function serializePathSegments(segments: readonly PathSegment[]): string { | ||
| let result = ''; | ||
| for (const segment of segments) { |
There was a problem hiding this comment.
Suggestion: Might be simpler with segments.map(serializePathSegment).join('/')?
| /** | ||
| * Extracts raw non-empty segment strings from a URL pathname without allocating empty strings. | ||
| */ | ||
| function extractPathSegments(path: string): string[] { |
There was a problem hiding this comment.
Question: How is this different from parsePathSegment above? It looks pretty similar.
| const segments = parsePathSegments(path); | ||
| const hasDynamicSegments = segments.some((segment) => segment.type !== 'static'); | ||
|
|
||
| if (rawSuffix[0] === '?' && rawSuffix.includes('=:')) { |
There was a problem hiding this comment.
Bug: What about /?foo=myfavoriteemojiistheonethatlookslikeguilefromstreetfighter=:isntthatsocool
| const path = suffixIndex === -1 ? url : url.slice(0, suffixIndex); | ||
| const rawSuffix = suffixIndex === -1 ? '' : url.slice(suffixIndex); | ||
| const segments = parsePathSegments(path); | ||
| const hasDynamicSegments = segments.some((segment) => segment.type !== 'static'); |
There was a problem hiding this comment.
Consider: Can we just use new Url(url, 'localhost') and read path / query parameters / fragment off that object? I know we've had issues with inconsistent parsing behavior before.
| expect(serializePathSegments(parsed.segments)).toBe('/user/:id/*/details/*0/**'); | ||
| }); | ||
|
|
||
| it('should normalize leading, trailing, and consecutive slashes in parsePathSegments', () => { |
There was a problem hiding this comment.
Question: Do we need to care about backslashes \?
alan-agius4
added
action: merge
Follow-up to #34467 to parse route paths and redirect URLs up front into structured
PathSegmentandQueryParamdiscriminated unions (static,param,wildcard), decouplingresolveRedirectToandbuildUrlWithParamsfrom string-splitting offsets while preserving fast paths for static redirects and reusing a singleUrlTreeshell when serializing dynamic query parameters.