-
Notifications
You must be signed in to change notification settings - Fork 12k
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
base: main
Are you sure you want to change the base?
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -382,6 +382,7 @@ async function* handleSSGRoute( | |
|
||
const { route: currentRoutePath, fallback, ...meta } = metadata; | ||
const getPrerenderParams = 'getPrerenderParams' in meta ? meta.getPrerenderParams : undefined; | ||
const isCatchAllRoute = currentRoutePath.endsWith('**'); | ||
|
||
if ('getPrerenderParams' in meta) { | ||
delete meta['getPrerenderParams']; | ||
|
@@ -391,7 +392,10 @@ async function* handleSSGRoute( | |
meta.redirectTo = resolveRedirectTo(currentRoutePath, redirectTo); | ||
} | ||
|
||
if (!URL_PARAMETER_REGEXP.test(currentRoutePath)) { | ||
if ( | ||
(isCatchAllRoute && !getPrerenderParams) || | ||
(!isCatchAllRoute && !URL_PARAMETER_REGEXP.test(currentRoutePath)) | ||
) { | ||
// Route has no parameters | ||
yield { | ||
...meta, | ||
|
@@ -415,7 +419,9 @@ async function* handleSSGRoute( | |
|
||
if (serverConfigRouteTree) { | ||
// Automatically resolve dynamic parameters for nested routes. | ||
const catchAllRoutePath = joinUrlParts(currentRoutePath, '**'); | ||
const catchAllRoutePath = isCatchAllRoute | ||
? currentRoutePath | ||
: joinUrlParts(currentRoutePath, '**'); | ||
const match = serverConfigRouteTree.match(catchAllRoutePath); | ||
if (match && match.renderMode === RenderMode.Prerender && !('getPrerenderParams' in match)) { | ||
serverConfigRouteTree.insert(catchAllRoutePath, { | ||
|
@@ -429,20 +435,38 @@ async function* handleSSGRoute( | |
const parameters = await runInInjectionContext(parentInjector, () => getPrerenderParams()); | ||
try { | ||
for (const params of parameters) { | ||
const routeWithResolvedParams = currentRoutePath.replace(URL_PARAMETER_REGEXP, (match) => { | ||
const parameterName = match.slice(1); | ||
const value = params[parameterName]; | ||
if (typeof value !== 'string') { | ||
const isParamsArray = Array.isArray(params); | ||
|
||
if (isParamsArray) { | ||
if (!isCatchAllRoute) { | ||
throw new Error( | ||
`The 'getPrerenderParams' function defined for the '${stripLeadingSlash(currentRoutePath)}' route ` + | ||
`returned a non-string value for parameter '${parameterName}'. ` + | ||
`Please make sure the 'getPrerenderParams' function returns values for all parameters ` + | ||
'specified in this route.', | ||
`The 'getPrerenderParams' function for the '${stripLeadingSlash(currentRoutePath)}' ` + | ||
`route returned an array '${JSON.stringify(params)}', which is not valid for catch-all routes.`, | ||
); | ||
} | ||
} else if (isCatchAllRoute) { | ||
throw new Error( | ||
`The 'getPrerenderParams' function for the '${stripLeadingSlash(currentRoutePath)}' ` + | ||
`route returned an object '${JSON.stringify(params)}', which is not valid for parameterized routes.`, | ||
); | ||
} | ||
|
||
return value; | ||
}); | ||
const routeWithResolvedParams = isParamsArray | ||
? currentRoutePath.replace('**', params.join('/')) | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Consider: What if I render I remember we had a discussion on this previously, do we need to escape There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. No we are not escaping any character. |
||
: currentRoutePath.replace(URL_PARAMETER_REGEXP, (match) => { | ||
const parameterName = match.slice(1); | ||
const value = params[parameterName]; | ||
if (typeof value !== 'string') { | ||
throw new Error( | ||
`The 'getPrerenderParams' function defined for the '${stripLeadingSlash(currentRoutePath)}' route ` + | ||
`returned a non-string value for parameter '${parameterName}'. ` + | ||
`Please make sure the 'getPrerenderParams' function returns values for all parameters ` + | ||
'specified in this route.', | ||
); | ||
} | ||
|
||
return value; | ||
}); | ||
|
||
yield { | ||
...meta, | ||
|
@@ -530,9 +554,9 @@ function buildServerConfigRouteTree({ routes, appShellRoute }: ServerRoutesConfi | |
continue; | ||
} | ||
|
||
if (path.includes('*') && 'getPrerenderParams' in metadata) { | ||
if ('getPrerenderParams' in metadata && (path.includes('/*/') || path.endsWith('/*'))) { | ||
errors.push( | ||
`Invalid '${path}' route configuration: 'getPrerenderParams' cannot be used with a '*' or '**' route.`, | ||
`Invalid '${path}' route configuration: 'getPrerenderParams' cannot be used with a '*' route.`, | ||
); | ||
continue; | ||
} | ||
|
There was a problem hiding this comment.
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
orgetPrerenderSegments
There was a problem hiding this comment.
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.