-
Notifications
You must be signed in to change notification settings - Fork 25.2k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Fix module loading to handle interaction between route preload strategy and route navigation #36760
Conversation
Add support for delayed loading of modules within SpyNgModuleFactoryLoader This allow us to test the loading modules that overlap
825728a
to
76f61df
Compare
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
If the preload strategy finds a route which is already loaded, do not reload it but ensure that and modules attached to child routes are still managed by the preload strategy.
76f61df
to
3a6db49
Compare
* Refactor delay function so we can it can be used with other stubbedModule modifiers * Add Error modifier. This allow us to test route failure * Add new test for stubbedModule modifiers
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
36904e2
to
df1fa8c
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The implementation is simple enough and looks relatively good to me. The tests need some work though so I'll re-review the implementation once those are redone. Please see the comments.
if (this.onLoadStartListener) { | ||
this.onLoadStartListener(route); | ||
} | ||
if (!route._loader$) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can we flip this to be an early return instead to reduce nesting of the main path?
if (route._loader$) {
return route._loader$;
}
@@ -31,13 +31,23 @@ import {ChildrenOutletContexts, ExtraOptions, NoPreloading, PreloadingStrategy, | |||
* class LoadedModule {} | |||
* | |||
* // sets up stubbedModules | |||
* loader.stubbedModules = {lazyModule: LoadedModule}; | |||
* loader.stubbedModules = { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't think the SpyNgModuleFactoryLoader
should be extended in this way. All of your tests can simply be rewritten to spy on the load
method and decide what to do on a case-by-case basis.
Additionally, the SpyNgModuleFactoryLoader
should be seen as deprecated anyways because it is using the string method of lazy loading children, which is deprecated. Instead, you can rewrite the tests to use spys for the loadChildren
functions like this:
const subLoadChildrenSpy = jasmine.createSpy();
const lazyLoadChildrenSpy = jasmine.createSpy();
{path: 'sub', loadChildren: subLoadChildrenSpy}
{path: 'lazy', loadChildren: lazyLoadChildrenSpy},
and then the following test could be:
fit('and cope with the loader throwing exceptions',
fakeAsync(inject(
[RouterPreloader, Router, NgModuleRef, Compiler],
( preloader: RouterPreloader, router: Router) => {
router.events.subscribe(e => {
if (e instanceof RouteConfigLoadEnd || e instanceof RouteConfigLoadStart) {
events.push(e);
}
});
let loadedConfig: LoadedRouterConfig;
const c = router.config;
loadedConfig = (c[0] as any)._loadedConfig;
expect(loadedConfig).toBeUndefined();
lazyLoadChildrenSpy.and.returnValue(throwError('Error: Fake module load error (expectedreload)'));
preloader.preload().subscribe((x) => {});
tick();
loadedConfig = (c[0] as any)._loadedConfig;
expect(loadedConfig).toBeUndefined();
router.navigateByUrl('/lazy/LoadedModule1').catch((reason) => {
expect(reason).toEqual('Error: Fake module load error (expectedreload)');
});
tick();
loadedConfig = (c[0] as any)._loadedConfig;
expect(loadedConfig).toBeUndefined();
expect((c[0] as any)._loader$).toBeUndefined();
lazyLoadChildrenSpy.and.returnValue(of(LoadedModule1));
router.navigateByUrl('/lazy/LoadedModule1').catch(() => {
expect('Not to get here').toBeUndefined('Not to throw');
});
tick();
expect(events.map(e => e.toString())).toEqual([
'RouteConfigLoadStart(path: lazy)', 'RouteConfigLoadStart(path: lazy)',
'RouteConfigLoadEnd(path: lazy)'
]);
})));
@@ -179,6 +182,289 @@ describe('RouterPreloader', () => { | |||
}))); | |||
}); | |||
|
|||
describe('should support preloading stratages', () => { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
These tests are too complicated. Please refactor them to be more simple and limit the assertions you're making in each one. It's hard to follow what assertions you're actually making in each one. You shouldn't be needing to share so many lines of assertions and setup for each one. Additionally, these should be rewritten to not use the SpyNgModuleFactoryLoader
as described below.
Closing due to lack of activity. If you still want to contribute a change, please open a new PR and I'd be happy to review it. Thanks! |
This issue has been automatically locked due to inactivity. Read more about our automatic conversation locking policy. This action has been performed automatically by a bot. |
PR Checklist
PR Type
What kind of change does this PR introduce?
What is the current behaviour?
This PR addresses the issues found in the issues #22842, and pr #26557
It also replaces the PR for #26557 which has a number of issues
Issue Number: #26557, #22842
What is the new behaviour?
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
We also add a new feature to SpyNgModuleFactoryLoader to enable us to emulate slow loading modules, this is required for testing.
Does this PR introduce a breaking change?
Other information / Questions