fix(router): unsubscribe preload if the route has been preloaded

[Wykks]

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?

When a lazyloaded route is loaded with a user interaction for example, the observable returned by preload from a PreloadingStrategy is not unsubscribed. Which do not stop the preload strategy from loading the module again.

This basic preload strategy load the module twice if the user navigate to the route before 5 seconds:

@Injectable({
  providedIn: 'root'
})
export class PreloadAfterAWhile implements PreloadingStrategy {
  preload(route: Route, load: () => Observable<any>) {
    return of(true).pipe(delay(5000), switchMap(load));
  }
}

Reproduced here: https://stackblitz.com/edit/angular-preload-issue
Step to reproduce:

1. Click on customers
2. Look at the console "CustomersModule Loaded!"
3. Wait at least 5 seconds
4. The CustomersModule is loaded again, because the delay observable from the preload strategy has not been canceled.

What is the new behavior?

Cancel the preload observable if the route is loaded.

Does this PR introduce a breaking change?

• Yes
• No

Other information

Also, the observable returned by load() is not cold, the loading start when the function is called, even if the observable is not subscribed. Is this intended ?

angular/packages/router/src/router_config_loader.ts

Lines 47 to 60 in c9488b5

	private loadModuleFactory(loadChildren: LoadChildren): Observable<NgModuleFactory<any>> {
	if (typeof loadChildren === 'string') {
	return from(this.loader.load(loadChildren));
	} else {
	return wrapIntoObservable(loadChildren()).pipe(mergeMap((t: any) => {
	if (t instanceof NgModuleFactory) {
	return of (t);
	} else {
	return from(this.compiler.compileModuleAsync(t));
	}
	}));
	}
	}
	}

this.loader.load(loadChildren) should be wrapped inside an observable.

[jasonaden]

Thanks for the PR. Can you please add a test? I would like to confirm this was failing in the past.

Looking at the code, I see a concatMap here. I haven't tested myself, but it looks to me like this will end up unsubscribing appropriately.

[jasonaden]

See comment about adding tests to confirm this is currently broken and this fixes it.

[Wykks]

I already made stackblitz to reproduce the issue, so you should be able to confirm the issue there.
I have no idea how to make a test for that, but I'll try 👍

[atscott]

Closing due to the issues mentioned in #36760

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

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