fix(router): lazy loaded modules without RouterModule.forChild() won't cause infinite loop

[martinsik]

PR Checklist

Please check if your PR fulfills the following requirements:

• The commit message follows our guidelines: https://github.com/angular/angular/blob/master/CONTRIBUTING.md#commit
• Tests for the changes have been added (for bug fixes / features)
• Docs have been added / updated (for bug fixes / features)

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
• angular.io application / infrastructure changes
• Other... Please describe:

What is the current behavior?

Issue Number: #29164

When loading a module that doesn't provide RouterModule.forChild() preloader will get stuck in a infinite loop and throw ERROR Error: Maximum call stack size exceeded.

https://stackblitz.com/edit/angular-issue-repro2-atwzck?file=src%2Fapp%2Ffeature1%2Ffeature1.module.ts

The issue is that child module's Injector will look to its parent Injector when it doesn't find any ROUTES so it will return routes for it's parent module instead. This will load the child again that returns its parent's routes and so on.

What is the new behavior?

Lazy loaded modules without RouterModule.forChild() won't break the app.

I was thinking that this might throw an exception telling me that I'm lazy loading a module that doesn't define any routes but maybe it is a valid use-case (for example I'm loading a module that only does some logic in its constructor).

Does this PR introduce a breaking change?

• Yes
• No

Other information

I only changed one line and added one test that would fail with Error: 1 timer(s) still in the queue. without this change:

https://github.com/angular/angular/compare/master...martinsik:issue-29164?expand=1#diff-2a29f955e9e56e86ee957a9768a19929R44

All other changes were made by running yarn gulp format

Closes #29164

[atscott]

Hi @martinsik, thanks for another router fix! Can you update the commit message to match what you have in the "current behavior" section (without the stackblitz link) and also add a similar comment to the LoadedRouterConfig line that you changed to describe why InjectFlags.Self is required (again, pretty much exactly what you have in the current behavior section)?

[atscott]

presubmit

[martinsik]

Commit message updated.

[atscott]

Presubmit is showing internal test failures when running with Ivy. Needs more investigation.

[martinsik]

@atscott Hey, pls do you have any update on this :)?

[atscott]

Suggested change

	flatten(module.injector.get(ROUTES, undefined, InjectFlags.Self)).map(standardizeConfig),
	flatten(module.injector.get(ROUTES, undefined, InjectFlags.Self|InjectFlags.Optional)).map(standardizeConfig),

[atscott]

The failure that was found happened because your test is only verifying behavior on the preloader, which has a catchError line

angular/packages/router/src/router_preloader.ts

Line 43 in 209a365

return fn().pipe(catchError(() => of(null)));

Your change introduces a newly thrown error that would have otherwise not happened. The following test will fail if added to the integration.spec.ts (please add this test):

      @NgModule({})
      class EmptyModule{}

      fit('should allow navigation to modules with no routes', fakeAsync(() => {
        (TestBed.inject(NgModuleFactoryLoader) as SpyNgModuleFactoryLoader).stubbedModules = {
          empty: EmptyModule,
        };
        const router = TestBed.inject(Router);
        const fixture = createRoot(router, RootCmp);

        router.resetConfig([{path: 'lazy', loadChildren: 'empty'}]);

        router.navigateByUrl('/lazy');
        advance(fixture);
      }));

I'm not exactly sure what the implications are for this. My understanding of the change you're making is that previously, navigation to this route would have inherited the parent routes. Even though it's infinitely circular, the problem wouldn't really surface unless there was preloading involved. I don't know exactly how this type of route is used so I don't know if the InjectFlags.Self change negatively affects how that works. (Edit: potentially, this is something that enables a hybrid app to handle the route in AngularJS and also do some work on the Angular side, but that's just a shot in the dark.)

Please add the InjectFlags.Optional and I'll run more tests to see if anything comes up. Thank you!

[martinsik]

@atscott Hi, I addressed what you mentioned above and also added the test you made into integration.spec.ts.

[atscott]

global presbumit

[atscott]

caretaker note: just to be safe, let's merge and sync this one on its own. I don't anticipate issues as a result of this fix, but you never know.

[angular-automatic-lock-bot]

This issue has been automatically locked due to inactivity.
Please file a new issue if you are encountering a similar or related problem.

Read more about our automatic conversation locking policy.

_{This action has been performed automatically by a bot.}