Skip to content

perf(core): do not recurse into modules that have already been registered #39514

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.

Sign up for GitHub

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

Closed
JoostK wants to merge 2 commits into from
Closed

perf(core): do not recurse into modules that have already been registered #39514

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
dfaf450
perf(core): do not recurse into modules that have already been regist…
JoostK Oct 30, 2020
2b05235
fixup! perf(core): do not recurse into modules that have already been…
JoostK Oct 30, 2020
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Next Next commit
perf(core): do not recurse into modules that have already been regist… 
…ered

When registering an NgModule based on its id, all transitively imported
NgModules are also registered. This commit introduces a visited set to
avoid traversing into NgModules that are reachable from multiple import
paths multiple times.

Fixes #39487
  • Loading branch information
@JoostK
JoostK committed Oct 30, 2020
commit dfaf45026352e29a0cb29ce2ed7071ab03b81af4
34 changes: 20 additions & 14 deletions packages/core/src/linker/ng_module_factory_registration.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -8,8 +8,9 @@


import {Type} from '../interface/type';
import {autoRegisterModuleById} from '../render3/definition';
import {autoRegisterModuleById, getNgModuleDef} from '../render3/definition';
import {NgModuleType} from '../render3/ng_module_ref';
import {maybeUnwrapFn} from '../render3/util/misc_utils';
import {stringify} from '../util/stringify';

import {NgModuleFactory} from './ng_module_factory';
Expand Down Expand Up @@ -39,20 +40,25 @@ function assertSameOrNotExisting(id: string, type: Type<any>|null, incoming: Typ
}
}

export function registerNgModuleType(ngModuleType: NgModuleType) {
if (ngModuleType.ɵmod.id !== null) {
const id = ngModuleType.ɵmod.id;
const existing = modules.get(id) as NgModuleType | null;
assertSameOrNotExisting(id, existing, ngModuleType);
modules.set(id, ngModuleType);
}
export function registerNgModuleType(ngModuleType: NgModuleType): void {
const visited = new Set<NgModuleType>();
recurse(ngModuleType);
function recurse(ngModuleType: NgModuleType): void {
const def = getNgModuleDef(ngModuleType, true);

@AndrewKushnir AndrewKushnir Oct 30, 2020

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
const def = getNgModuleDef(ngModuleType, true);
const def = getNgModuleDef(ngModuleType, /* throwNotFound */ true);

May be also add a quick comment on why it's safe to throw if moduleDef is not found?

const id = def.id;
if (id !== null) {
const existing = modules.get(id) as NgModuleType | null;
assertSameOrNotExisting(id, existing, ngModuleType);
modules.set(id, ngModuleType);
}

let imports = ngModuleType.ɵmod.imports;
if (imports instanceof Function) {
imports = imports();
}
if (imports) {
imports.forEach(i => registerNgModuleType(i as NgModuleType));
const imports = maybeUnwrapFn(def.imports) as NgModuleType[];
for (const i of imports) {
if (!visited.has(i)) {
visited.add(i);
recurse(i);
}
}
}
}

Expand Down
4 changes: 2 additions & 2 deletions packages/core/test/render3/providers_spec.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -6,7 +6,7 @@
* found in the LICENSE file at https://angular.io/license
*/

import {Component as _Component, ComponentFactoryResolver, ElementRef, Injectable as _Injectable, InjectFlags, InjectionToken, InjectorType, Provider, RendererFactory2, Type, ViewContainerRef, ɵNgModuleDef as NgModuleDef, ɵɵdefineInjectable, ɵɵdefineInjector, ɵɵinject} from '../../src/core';
import {Component as _Component, ComponentFactoryResolver, ElementRef, Injectable as _Injectable, InjectFlags, InjectionToken, InjectorType, Provider, RendererFactory2, Type, ViewContainerRef, ɵɵdefineInjectable, ɵɵdefineInjector, ɵɵdefineNgModule, ɵɵinject} from '../../src/core';
import {forwardRef} from '../../src/di/forward_ref';
import {createInjector} from '../../src/di/r3_injector';
import {injectComponentFactoryResolver, ɵɵdefineComponent, ɵɵdefineDirective, ɵɵdirectiveInject, ɵɵelement, ɵɵelementEnd, ɵɵelementStart, ɵɵgetInheritedFactory, ɵɵProvidersFeature, ɵɵtext, ɵɵtextInterpolate1} from '../../src/render3/index';
Expand Down Expand Up @@ -1092,7 +1092,7 @@ describe('providers', () => {
{provide: String, useValue: 'From module injector'}
]
});
static ɵmod: NgModuleDef<any> = {bootstrap: []} as any;
static ɵmod = ɵɵdefineNgModule({type: MyAppModule});
}
const myAppModuleFactory = new NgModuleFactory(MyAppModule);
const ngModuleRef = myAppModuleFactory.create(null);
Expand Down