iframe-proxy

Wykks · 2018-10-18T21:32:42Z

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:

Click on customers
Look at the console "CustomersModule Loaded!"
Wait at least 5 seconds
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 · 2019-01-09T17:48:21Z

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 · 2019-01-09T18:36:48Z

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 👍

Add four test to check the interaction of PreloadingStrategy and normal router navigation under differing scenarios. Checking: * Prevention of duplicate loading of modules. related to angular#26557 * Prevention of duplicate RouteConfigLoad(Start|End) events related to angular#22842 * Ensuring preload strategy remains active for submodules if needed The selected preload strategy should still decide when to load submodules * Possibility of memory leak with unfinished preload subscription related to angular#26557

Store an observable on the route for the currently outstanding module load factory. The replaces the fix angular#26557 as that allowed overlapping requests to still load the module twice. Fixes angular#26557 angular#22842

Clear the stored loader promise so that subsequent load will try the fetch again. The restores the same behaviour as before we cached the loader to fix angular#26557

atscott · 2020-08-07T22:08:11Z

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 · 2020-09-07T16:19:52Z

Fix router to ensure that a route module is only loaded once especially in relation to the use of preload strategies with delayed or partial loading. Add test to check the interaction of PreloadingStrategy and normal router navigation under differing scenarios. Checking: * Prevention of duplicate loading of modules. related to angular#26557 * Prevention of duplicate RouteConfigLoad(Start|End) events related to angular#22842 * Ensuring preload strategy remains active for submodules if needed The selected preload strategy should still decide when to load submodules * Possibility of memory leak with unfinished preload subscription related to angular#26557 * Ensure that the stored loader promise is cleared so that subsequent load will try the fetch again. * Add error handle error from loadChildren * Ensure we handle error from with NgModule create Fixes angular#26557 angular#22842 angular#26557

#40389) Fix router to ensure that a route module is only loaded once especially in relation to the use of preload strategies with delayed or partial loading. Add test to check the interaction of PreloadingStrategy and normal router navigation under differing scenarios. Checking: * Prevention of duplicate loading of modules. related to #26557 * Prevention of duplicate RouteConfigLoad(Start|End) events related to #22842 * Ensuring preload strategy remains active for submodules if needed The selected preload strategy should still decide when to load submodules * Possibility of memory leak with unfinished preload subscription related to #26557 * Ensure that the stored loader promise is cleared so that subsequent load will try the fetch again. * Add error handle error from loadChildren * Ensure we handle error from with NgModule create Fixes #26557 #22842 #26557 PR Close #40389

googlebot added the cla: yes label Oct 18, 2018

Wykks force-pushed the patch-1 branch from c023705 to 5a659c4 Compare October 18, 2018 21:43

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

301d6bb

Wykks force-pushed the patch-1 branch from 5a659c4 to 301d6bb Compare October 18, 2018 21:50

Wykks changed the base branch from 7.0.x to master October 25, 2018 12:01

Wykks changed the base branch from master to 7.0.x October 25, 2018 12:01

benlesh requested a review from jasonaden January 8, 2019 23:57

benlesh added type: bug/fix area: router labels Jan 8, 2019

ngbot Bot added this to the needsTriage milestone Jan 8, 2019

jasonaden suggested changes Jan 9, 2019

View reviewed changes

benlesh added risk: low memory leak Issue related to a memory leak labels Jan 10, 2019

pgammans mentioned this pull request Apr 22, 2020

Fix module loading to handle interaction between route preload strategy and route navigation #36760

Closed

7 tasks

atscott closed this Aug 7, 2020

angular-automatic-lock-bot Bot locked and limited conversation to collaborators Sep 7, 2020

atscott reopened this Feb 11, 2021

atscott closed this Feb 11, 2021

Sunbelt Computer Software

PL/B Language Development and Support

Navigation Menu

Uh oh!

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

fix(router): unsubscribe preload if the route has been preloaded#26557
Wykks wants to merge 1 commit into
angular:7.0.xfrom
Wykks:patch-1

Wykks commented Oct 18, 2018 •

edited

Loading

Uh oh!

jasonaden commented Jan 9, 2019

Uh oh!

jasonaden left a comment

Uh oh!

Wykks commented Jan 9, 2019

Uh oh!

atscott commented Aug 7, 2020

Uh oh!

angular-automatic-lock-bot Bot commented Sep 7, 2020

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

	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));
	}
	}));
	}
	}
	}

Sunbelt Computer Software

PL/B Language Development and Support

Uh oh!

Conversation

Wykks commented Oct 18, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

PR Checklist

PR Type

What is the current behavior?

What is the new behavior?

Does this PR introduce a breaking change?

Other information

Uh oh!

jasonaden commented Jan 9, 2019

Uh oh!

jasonaden left a comment

Choose a reason for hiding this comment

Uh oh!

Wykks commented Jan 9, 2019

Uh oh!

atscott commented Aug 7, 2020

Uh oh!

angular-automatic-lock-bot Bot commented Sep 7, 2020

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

Wykks commented Oct 18, 2018 •

edited

Loading