-
Notifications
You must be signed in to change notification settings - Fork 11.9k
fix(@angular/ssr): ensure wildcard RenderMode is applied when no Angular routes are defined #28718
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
Conversation
…lar routes are defined This fix addresses a bug where, in the absence of defined Angular routes, the RenderMode was not correctly applied based on the wildcard setting.
@@ -478,7 +478,12 @@ export async function getRoutesFromAngularRouterConfig( | |||
} | |||
} | |||
} else { | |||
routesResults.push({ route: '', renderMode: RenderMode.Prerender }); | |||
const renderMode = serverConfigRouteTree?.match('')?.renderMode ?? RenderMode.Prerender; |
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.
@dgp1130, I'd prefer if in this case it was a hard error both the old and new API. But this would require us to generate an additional component and add route when doing ng-new
.
Is this something that we'll be willing to do?
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.
This is the case where export const SERVER_ROUTES = [];
? You think that should be a hard error?
I can see where you're coming from. This has bugged me in the past because we do the same thing for client-side routes and it means the <router-outlet>
is a somewhat confusing no-op in ng new
and it's not clear how to add a route, which leads to devs using component
instead of loadComponent
and deoptimizing their apps.
Could we generate { path: '/**', renderMode: RenderMode.Prerender }
by default in SSR? Would that avoid the need for a defined route? Or do we need to add a route in the app router to get this to work?
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.
This is the case where export const SERVER_ROUTES = [];? You think that should be a hard error?
I was referring to Angular Router routes.
We do generate { path: '/**', renderMode: RenderMode.Prerender }
already but we do not generate any route in the Angular router.
@@ -478,7 +478,12 @@ export async function getRoutesFromAngularRouterConfig( | |||
} | |||
} | |||
} else { | |||
routesResults.push({ route: '', renderMode: RenderMode.Prerender }); | |||
const renderMode = serverConfigRouteTree?.match('')?.renderMode ?? RenderMode.Prerender; |
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.
This is the case where export const SERVER_ROUTES = [];
? You think that should be a hard error?
I can see where you're coming from. This has bugged me in the past because we do the same thing for client-side routes and it means the <router-outlet>
is a somewhat confusing no-op in ng new
and it's not clear how to add a route, which leads to devs using component
instead of loadComponent
and deoptimizing their apps.
Could we generate { path: '/**', renderMode: RenderMode.Prerender }
by default in SSR? Would that avoid the need for a defined route? Or do we need to add a route in the app router to get this to work?
The changes were merged into the following branches: main, 19.0.x |
This issue has been automatically locked due to inactivity. Read more about our automatic conversation locking policy. This action has been performed automatically by a bot. |
This fix addresses a bug where, in the absence of defined Angular routes, the RenderMode was not correctly applied based on the wildcard setting.