Fix module loading to ensure each module is loaded only once

[pgammans]

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?

This PR addresses the issues found in the issues #22842, PR #26557, PR #36760, PR #26557

It also replaces the PR for #26557 which has a number of issues and is an

By forcing the subscription to close if a module has a lazy sub-module then the these are passed to the PreloadingStrategy class as the observable has already terminated.
It doesn't actually prevent the load as the fetch may overlap with the preload fetch. Ie both the pre-loader and navigation can start a load both will finish.
Issue Number: #26557, #22842

What is the new behavior?

By Prevent loading a route module twice, we prevent the sending of multiple RouteConfigLoadEnd events, and the duplicate console log as show in the stackbiz example on #26557

Does this PR introduce a breaking change?

• Yes
• No

Other information

This is an updated version of my originally and closed PR #36760 (due to my failure to update) but addressing @atscott comments.

[atscott]

@pgammans - Thanks for resubmitting this PR with the updates! A few small nits in the test and one question about finding a strategy to avoid using share but otherwise LGTM

[atscott]

@pgammans one more thing - could you squash all of your commits into one?

[atscott]

global presubmit

[atscott]

Hi @pgammans - sorry for the slow response. After running internal tests, the approach we made to avoid introducing the share operator caused failures. Using the share like was there initially does work though.

I don't know if we can justify the additional payload size increase to address this edge case where a preloader and a navigation duplicate the loading of a lazy route. That would increase the payload for everyone and only prevent an extra load of a lazy module in a rare scenario so I think the cost-benefit doesn't quite work out in our favor.

What we can certainly do now though is keep the check that you added in preloadConfig to partially address the issue.

[pgammans]

@atscott can i ask what the failure was and to see if i can develop a third solution that addresses all the issues.

Obviously share does something that addresses this issue it may be possible to implement a fix for this without all the overheads that the generic share function will need to address and

load(parentInjector: Injector, route: Route): Observable<LoadedRouterConfig> {
   if (route._loader$) {
     return route._loader$;
   }

   if (this.onLoadStartListener) {
     this.onLoadStartListener(route);
   }
   const moduleFactory$ = this.loadModuleFactory(route.loadChildren!);
   route._loader$ =  new ConnectableObservable(
     moduleFactory$.pipe(
       map((factory: NgModuleFactory<any>) => {
         if (this.onLoadEndListener) {
           this.onLoadEndListener(route);
         }
         const module = factory.create(parentInjector);
         // When loading a module that doesn't provide `RouterModule.forChild()` preloader will get
         // stuck in an infinite loop. The 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.or
         return new LoadedRouterConfig(
             flatten(
                 module.injector.get(ROUTES, undefined, InjectFlags.Self | InjectFlags.Optional))
                 .map(standardizeConfig),
             module);
       }),
       catchError((err) => {
         route._loader$ = undefined;
         throw err;
       }),
     ),
     () => new Subject<LoadedRouterConfig>()
   ).pipe(refCount())
   return route._loader$;

Even this may be smaller as remove some of share features.
How can i test the size change?

[atscott]

@pgammans - I haven't been able to debug the exact issue internally so I unfortunately can't provide much more information now. It's a few of the application's webdriver integration test that are failing and I'll have to figure out how to spin up the app locally to debug it.
The snippet with ConnectableObservable and refCount also passes the tests but I'm still concerned about the tradeoff in bundle size vs impact. Running the bundle size check locally doesn't seem to work (on our plate to investigate soon) but if you make the change to your PR, the CI will be able to run the test and tell you the new bundle size.

[pgammans]

@atscott
I assume this is OK, CI says no size change. I assume this is as ConnectableObservable is already allocated in the routers golden file.

[atscott]

suggestion: What do you think of extracting this first argument of the ConnectableObservable to a variable so it's easier to see the two arguments?

[pgammans]

I've push a change for this and rebased to master.
I'm not 100% sure i agree as i think it breaks the link that the effective 'share' is part of the loader, but i can see an argument for separation of the loader from the share.

[atscott]

@pgammans - Yep, I believe you are correct. It looks like the goldens also already have the refcount operator. I want to run some final tests on this but at the moment it's looking good. Thanks for the work on this!

[petebacondarwin]

Reviewed-for: size-tracking

[pgammans]

@petebacondarwin I have applied your suggested commit for the spelling mistake, rebased onto master. I have also squashed all the commits as I assume this would be wanted.

[pgammans]

@atscott the size of aio-local-viewengine has also increased do you want a commit to increase ti size too?

expected 432078, actual: 432647 so an increase of 569 bytes

[atscott]

FYI - this fix was reverted as there were internal failures during the sync that need further investigation.

[atscott]

Okay, so the issue is that by de-duplicating the module loading and using the same router config objects, we change how routes are re-used. That is, an ActivatedRoute/component that was not previously re-used will now be re-used because the route objects have the same reference.
This is actually the correct behavior, but it's something we'll still have to address. Devs may need to update their tests if they accidentally rely on the current broken behavior. I'm still investigating how widespread this problem is.

[atscott]

Update: There was only one test failure (the one from yesterday) in the global test results. That indicates the issue isn't widespread enough to require further action other than a fix to this one test. After that is done, we can resubmit this PR.

[atscott]

global presubmit

[atscott]

caretaker note: Since this PR previously had to be rolled back, I think it'd be a good idea to merge and sync on its own.

[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.}