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

global presubmit

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.

@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?

@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.

@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.

@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 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.

@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

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

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.

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.

global presubmit

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.

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