Skip to content
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

AngularNodeAppEngine has different unknown path routing behavior than CommonEngine #29539

Closed
1 task done
griest024 opened this issue Jan 29, 2025 · 6 comments · Fixed by #29543
Closed
1 task done

AngularNodeAppEngine has different unknown path routing behavior than CommonEngine #29539

griest024 opened this issue Jan 29, 2025 · 6 comments · Fixed by #29543
Assignees
Labels
area: @angular/ssr freq1: low Only reported by a handful of users who observe it rarely severity3: broken type: bug/fix

Comments

@griest024
Copy link

Command

serve

Is this a regression?

  • Yes, this behavior used to work in the previous version

The previous version in which this bug was not present was

18

Description

When the routing configuration contains a nested wildcard route and a redirect target and an non-nested wildcard route after the nested route that redirects to the nested route (see below), CommonEngine will use the nested route while AngularNodeAppEngine will use the non-nested route.

@Component({selector: 'test', template: 'test'})
class TestComponent {}

@Component({selector: 'not-found', template: '404 not found'})
class NotFoundComponent {}

export const routes: Routes = [
  {
    path: '',
    children: [
      // AngularNodeAppEngine will end up following the redirect and rendering this route
      {
        path: 'not-found',
        component: NotFoundComponent
      },
      // CommonEngine will just simply render this route
      {
        path: '**',
        component: TestComponent
      },
    ]
  },
  // AngularNodeAppEngine chooses this route
  {
    path: '**',
    redirectTo: 'not-found'
  },
]

Is this intended behavior? It seems that the vite dev server follows the behavior of AngularNodeAppEngine and is blocking us from upgrading to v19.

Something odd is that AngularNodeAppEngine will render / as test but /asdfasdf as 404 not found. CommonEngine renders test for both of these paths.

Some debugging found that

routesResults.push(result);
is likely to blame. It flattens the route tree in a way that the nested wildcard route gets overridden by the non-nested one during the insert into the final route tree
routeTree.insert(fullRoute, metadata);

Minimal Reproduction

AngularNodeAppEngine - https://github.com/griest024/ng19-server-routing
CommonEngine - https://github.com/griest024/ng19-server-routing/tree/common-engine

  1. run npx ng b && npm run serve:ssr:19-dev-server-routes
  2. navigate to /asdfasdfasdf or any other route that is not / or /not-found
  3. See that the rendered component is different.

Exception or Error


Your Environment

_                      _                 ____ _     ___
    / \   _ __   __ _ _   _| | __ _ _ __     / ___| |   |_ _|
   / △ \ | '_ \ / _` | | | | |/ _` | '__|   | |   | |    | |
  / ___ \| | | | (_| | |_| | | (_| | |      | |___| |___ | |
 /_/   \_\_| |_|\__, |\__,_|_|\__,_|_|       \____|_____|___|
                |___/
    

Angular CLI: 19.1.5
Node: 18.20.5
Package Manager: npm 10.8.2
OS: linux x64

Angular: 19.1.3
... animations, common, compiler, compiler-cli, core, forms
... platform-browser, platform-browser-dynamic, platform-server
... router

Package                         Version
---------------------------------------------------------
@angular-devkit/architect       0.1901.5
@angular-devkit/build-angular   19.1.5
@angular-devkit/core            19.1.5
@angular-devkit/schematics      19.1.5
@angular/cli                    19.1.5
@angular/ssr                    19.1.5
@schematics/angular             19.1.5
rxjs                            7.8.1
typescript                      5.6.3
zone.js                         0.15.0

Anything else relevant?

No response

alan-agius4 added a commit to alan-agius4/angular-cli that referenced this issue Jan 30, 2025
…t ones

Ensures that the SSR router gives precedence to the first matching route, addressing the issue where later conflicting routes.

This change prevents the incorrect prioritization of routes and ensures the intended route is matched first, aligning routing behavior.

Closes: angular#29539
@alan-agius4 alan-agius4 added type: bug/fix freq1: low Only reported by a handful of users who observe it rarely severity3: broken area: @angular/ssr labels Jan 30, 2025
@alan-agius4
Copy link
Collaborator

alan-agius4 commented Jan 30, 2025

There seems to be a misalignment here, which we should address by prioritizing the first matched route. The issue appears to be related to your Angular routing configuration. I'm wondering if the Angular router should raise an error or at least a warning when there are conflicting routes like this.

The catch-all route (**) should always be defined at the top level without any parent routes or segments. This means there should only be one ** route at the top level. However, in your case, the parent route is an empty string, and it seems the Angular router is accepting this configuration and prioritizing the first route it encounters. As a result, the second ** route is never reached.

This leads to the following configuration:

[
  {
    path: 'not-found',
    component: NotFoundComponent
  },
  {
    path: '**',
    component: TestComponent
  },
  {
    path: '**',
    redirectTo: 'not-found'
  },
];

There might be a situation where there is more than one ** route if you have something like a canMatch guard involved but should be quite a bit out of the ordinary:

[
  {
    path: 'one',
    component: NotFoundComponent,
  },
  {
    path: '**',
    component: TestComponent,
    canMatch: [() => false]
  },
  {
    path: '**',
    component: NotFoundComponent,
  }
];

@alan-agius4 alan-agius4 self-assigned this Jan 30, 2025
@griest024
Copy link
Author

griest024 commented Jan 30, 2025

@alan-agius4 yes I forgot to note that in our actual routing config, we have multiple nested wildcard routes with canMatchs. I found that canMatch wasn't necessary for reproducing the bug so I didn't include it. Our real config looks something like this:

[
  {
    path: '',
    children: [
      {
        path: 'not-found',
        component: NotFoundComponent
      },
      {
        canMatch: [isSpecialRouteKind1],
        path: '**',
        component: TestComponent
      },
      {
        canMatch: [isSpecialRouteKind2],
        path: '**',
        component: TestComponent
      },
    ]
  },
  {
    path: '**',
    redirectTo: 'not-found'
  },
]

@griest024
Copy link
Author

Oh and AngularNodeAppEngine will fail to load the wildcard route entirely if the redirect-to is replaced with component: NotFoundComponent:

[
  {
    path: '',
    children: [
      {
        path: '**',
        component: TestComponent
      },
    ]
  },
  {
    path: '**',
    component: NotFoundComponent
  },
]

CommonEngine continues to work as expected in this case.

@alan-agius4
Copy link
Collaborator

Just my two cents — when using SSR, I'd be cautious with ** in the app Router. It could unintentionally catch requests for resources like images, JS, and CSS files, leading to the NotFoundComponent page instead of a proper 404.

This happens because the server doesn't differentiate between requests for an angular page and static files.

1 similar comment
@alan-agius4
Copy link
Collaborator

Just my two cents — when using SSR, I'd be cautious with ** in the app Router. It could unintentionally catch requests for resources like images, JS, and CSS files, leading to the NotFoundComponent page instead of a proper 404.

This happens because the server doesn't differentiate between requests for an angular page and static files.

@griest024
Copy link
Author

Just my two cents — when using SSR, I'd be cautious with ** in the app Router. It could unintentionally catch requests for resources like images, JS, and CSS files, leading to the NotFoundComponent page instead of a proper 404.

This happens because the server doesn't differentiate between requests for an angular page and static files.

We handle this in our server.ts. It has a much more selective endpoint than '*' for the angular renderer. Static files are served as expected by express. I omitted much of the code that would make the configuration "make sense" since it is proprietary client code. I appreciate the warning though.

jkrems pushed a commit that referenced this issue Feb 3, 2025
…t ones

Ensures that the SSR router gives precedence to the first matching route, addressing the issue where later conflicting routes.

This change prevents the incorrect prioritization of routes and ensures the intended route is matched first, aligning routing behavior.

Closes: #29539
jkrems pushed a commit that referenced this issue Feb 3, 2025
…t ones

Ensures that the SSR router gives precedence to the first matching route, addressing the issue where later conflicting routes.

This change prevents the incorrect prioritization of routes and ensures the intended route is matched first, aligning routing behavior.

Closes: #29539
(cherry picked from commit 6448f80)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area: @angular/ssr freq1: low Only reported by a handful of users who observe it rarely severity3: broken type: bug/fix
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants