diff --git a/packages/core/src/di/interface/provider.ts b/packages/core/src/di/interface/provider.ts index d78314457f..e45a8c55db 100644 --- a/packages/core/src/di/interface/provider.ts +++ b/packages/core/src/di/interface/provider.ts @@ -365,3 +365,9 @@ export interface ClassProvider extends ClassSansProvider { */ export type Provider = TypeProvider | ValueProvider | ClassProvider | ConstructorProvider | ExistingProvider | FactoryProvider | any[]; + +/** + * Describes a function that is used to process provider list (for example in case of provider + * overrides). + */ +export type ProcessProvidersFunction = (providers: Provider[]) => Provider[]; \ No newline at end of file diff --git a/packages/core/src/metadata/resource_loading.ts b/packages/core/src/metadata/resource_loading.ts index fa7657752c..ce4d7be738 100644 --- a/packages/core/src/metadata/resource_loading.ts +++ b/packages/core/src/metadata/resource_loading.ts @@ -6,6 +6,7 @@ * found in the LICENSE file at https://angular.io/license */ +import {Type} from '../interface/type'; import {Component} from './directives'; @@ -42,9 +43,9 @@ import {Component} from './directives'; * contents of the resolved URL. Browser's `fetch()` method is a good default implementation. */ export function resolveComponentResources( - resourceResolver: (url: string) => (Promise}>)): Promise { + resourceResolver: (url: string) => (Promise}>)): Promise { // Store all promises which are fetching the resources. - const urlFetches: Promise[] = []; + const componentResolved: Promise[] = []; // Cache so that we don't fetch the same resource more than once. const urlMap = new Map>(); @@ -53,50 +54,68 @@ export function resolveComponentResources( if (!promise) { const resp = resourceResolver(url); urlMap.set(url, promise = resp.then(unwrapResponse)); - urlFetches.push(promise); } return promise; } - componentResourceResolutionQueue.forEach((component: Component) => { + componentResourceResolutionQueue.forEach((component: Component, type: Type) => { + const promises: Promise[] = []; if (component.templateUrl) { - cachedResourceResolve(component.templateUrl).then((template) => { + promises.push(cachedResourceResolve(component.templateUrl).then((template) => { component.template = template; - }); + })); } const styleUrls = component.styleUrls; const styles = component.styles || (component.styles = []); const styleOffset = component.styles.length; styleUrls && styleUrls.forEach((styleUrl, index) => { styles.push(''); // pre-allocate array. - cachedResourceResolve(styleUrl).then((style) => { + promises.push(cachedResourceResolve(styleUrl).then((style) => { styles[styleOffset + index] = style; styleUrls.splice(styleUrls.indexOf(styleUrl), 1); if (styleUrls.length == 0) { component.styleUrls = undefined; } - }); + })); }); + const fullyResolved = Promise.all(promises).then(() => componentDefResolved(type)); + componentResolved.push(fullyResolved); }); clearResolutionOfComponentResourcesQueue(); - return Promise.all(urlFetches).then(() => null); + return Promise.all(componentResolved).then(() => undefined); } -const componentResourceResolutionQueue: Set = new Set(); +let componentResourceResolutionQueue = new Map, Component>(); -export function maybeQueueResolutionOfComponentResources(metadata: Component) { +// Track when existing ngComponentDef for a Type is waiting on resources. +const componentDefPendingResolution = new Set>(); + +export function maybeQueueResolutionOfComponentResources(type: Type, metadata: Component) { if (componentNeedsResolution(metadata)) { - componentResourceResolutionQueue.add(metadata); + componentResourceResolutionQueue.set(type, metadata); + componentDefPendingResolution.add(type); } } +export function isComponentDefPendingResolution(type: Type): boolean { + return componentDefPendingResolution.has(type); +} + export function componentNeedsResolution(component: Component): boolean { return !!( (component.templateUrl && !component.template) || component.styleUrls && component.styleUrls.length); } -export function clearResolutionOfComponentResourcesQueue() { - componentResourceResolutionQueue.clear(); +export function clearResolutionOfComponentResourcesQueue(): Map, Component> { + const old = componentResourceResolutionQueue; + componentResourceResolutionQueue = new Map(); + return old; +} + +export function restoreComponentResolutionQueue(queue: Map, Component>): void { + componentDefPendingResolution.clear(); + queue.forEach((_, type) => componentDefPendingResolution.add(type)); + componentResourceResolutionQueue = queue; } export function isComponentResourceResolutionQueueEmpty() { @@ -106,3 +125,7 @@ export function isComponentResourceResolutionQueueEmpty() { function unwrapResponse(response: string | {text(): Promise}): string|Promise { return typeof response == 'string' ? response : response.text(); } + +function componentDefResolved(type: Type): void { + componentDefPendingResolution.delete(type); +} \ No newline at end of file diff --git a/packages/core/src/render3/features/providers_feature.ts b/packages/core/src/render3/features/providers_feature.ts index e95d389c5a..8468c9d754 100644 --- a/packages/core/src/render3/features/providers_feature.ts +++ b/packages/core/src/render3/features/providers_feature.ts @@ -5,7 +5,7 @@ * Use of this source code is governed by an MIT-style license that can be * found in the LICENSE file at https://angular.io/license */ -import {Provider} from '../../di/interface/provider'; +import {ProcessProvidersFunction, Provider} from '../../di/interface/provider'; import {providersResolver} from '../di_setup'; import {DirectiveDef} from '../interfaces/definition'; @@ -39,7 +39,12 @@ import {DirectiveDef} from '../interfaces/definition'; */ export function ProvidersFeature(providers: Provider[], viewProviders: Provider[] = []) { return (definition: DirectiveDef) => { - definition.providersResolver = (def: DirectiveDef) => - providersResolver(def, providers, viewProviders); + definition.providersResolver = + (def: DirectiveDef, processProvidersFn?: ProcessProvidersFunction) => { + return providersResolver( + def, // + processProvidersFn ? processProvidersFn(providers) : providers, // + viewProviders); + }; }; } diff --git a/packages/core/src/render3/interfaces/definition.ts b/packages/core/src/render3/interfaces/definition.ts index 2f285bad69..1f8cacc62c 100644 --- a/packages/core/src/render3/interfaces/definition.ts +++ b/packages/core/src/render3/interfaces/definition.ts @@ -7,6 +7,7 @@ */ import {SchemaMetadata, ViewEncapsulation} from '../../core'; +import {ProcessProvidersFunction} from '../../di/interface/provider'; import {Type} from '../../interface/type'; import {CssSelectorList} from './projection'; @@ -138,7 +139,9 @@ export interface DirectiveDef extends BaseDef { type: Type; /** Function that resolves providers and publishes them into the DI system. */ - providersResolver: ((def: DirectiveDef) => void)|null; + providersResolver: + ((def: DirectiveDef, processProvidersFn?: ProcessProvidersFunction) => + void)|null; /** The selectors that will be used to match nodes to this directive. */ readonly selectors: CssSelectorList; diff --git a/packages/core/src/render3/jit/directive.ts b/packages/core/src/render3/jit/directive.ts index 07387940d2..c7fc2f587b 100644 --- a/packages/core/src/render3/jit/directive.ts +++ b/packages/core/src/render3/jit/directive.ts @@ -37,7 +37,7 @@ import {flushModuleScopingQueueAsMuchAsPossible, patchComponentDefWithScope, tra export function compileComponent(type: Type, metadata: Component): void { let ngComponentDef: any = null; // Metadata may have resources which need to be resolved. - maybeQueueResolutionOfComponentResources(metadata); + maybeQueueResolutionOfComponentResources(type, metadata); Object.defineProperty(type, NG_COMPONENT_DEF, { get: () => { const compiler = getCompilerFacade(); diff --git a/packages/core/src/render3/jit/module.ts b/packages/core/src/render3/jit/module.ts index faf86c541d..c7c758b489 100644 --- a/packages/core/src/render3/jit/module.ts +++ b/packages/core/src/render3/jit/module.ts @@ -357,6 +357,12 @@ export function patchComponentDefWithScope( componentDef.pipeDefs = () => Array.from(transitiveScopes.compilation.pipes).map(pipe => getPipeDef(pipe) !); componentDef.schemas = transitiveScopes.schemas; + + // Since we avoid Components/Directives/Pipes recompiling in case there are no overrides, we + // may face a problem where previously compiled defs available to a given Component/Directive + // are cached in TView and may become stale (in case any of these defs gets recompiled). In + // order to avoid this problem, we force fresh TView to be created. + componentDef.template.ngPrivateData = undefined; } /** diff --git a/packages/core/test/linker/ng_module_integration_spec.ts b/packages/core/test/linker/ng_module_integration_spec.ts index 669d48da96..1a0132d137 100644 --- a/packages/core/test/linker/ng_module_integration_spec.ts +++ b/packages/core/test/linker/ng_module_integration_spec.ts @@ -136,6 +136,15 @@ function declareTests(config?: {useJit: boolean}) { } function createComp(compType: Type, moduleType: Type): ComponentFixture { + const componentDef = (compType as any).ngComponentDef; + if (componentDef) { + // Since we avoid Components/Directives/Pipes recompiling in case there are no overrides, we + // may face a problem where previously compiled defs available to a given + // Component/Directive are cached in TView and may become stale (in case any of these defs + // gets recompiled). In order to avoid this problem, we force fresh TView to be created. + componentDef.template.ngPrivateData = null; + } + const ngModule = createModule(moduleType, injector); const cf = ngModule.componentFactoryResolver.resolveComponentFactory(compType) !; diff --git a/packages/core/test/test_bed_spec.ts b/packages/core/test/test_bed_spec.ts index 07e2445fe4..51c7ea7905 100644 --- a/packages/core/test/test_bed_spec.ts +++ b/packages/core/test/test_bed_spec.ts @@ -294,8 +294,8 @@ describe('TestBed', () => { TestBed.resetTestingModule(); const defAfterReset = (SomeComponent as any).ngComponentDef; - expect(defAfterReset.pipeDefs().length).toEqual(0); - expect(defAfterReset.directiveDefs().length).toEqual(1); // component + expect(defAfterReset.pipeDefs).toBe(null); + expect(defAfterReset.directiveDefs).toBe(null); }); it('should cleanup ng defs for classes with no ng annotations (in case of inheritance)', diff --git a/packages/core/testing/src/r3_test_bed.ts b/packages/core/testing/src/r3_test_bed.ts index 1270ce51ff..d385180bba 100644 --- a/packages/core/testing/src/r3_test_bed.ts +++ b/packages/core/testing/src/r3_test_bed.ts @@ -51,11 +51,12 @@ import { CompilerOptions, StaticProvider, COMPILER_OPTIONS, + ɵDirectiveDef as DirectiveDef, } from '@angular/core'; // clang-format on import {ResourceLoader} from '@angular/compiler'; -import {clearResolutionOfComponentResourcesQueue, componentNeedsResolution, resolveComponentResources} from '../../src/metadata/resource_loading'; +import {clearResolutionOfComponentResourcesQueue, componentNeedsResolution, resolveComponentResources, maybeQueueResolutionOfComponentResources, isComponentDefPendingResolution, restoreComponentResolutionQueue} from '../../src/metadata/resource_loading'; import {ComponentFixture} from './component_fixture'; import {MetadataOverride} from './metadata_override'; import {ComponentResolver, DirectiveResolver, NgModuleResolver, PipeResolver, Resolver} from './resolvers'; @@ -260,10 +261,12 @@ export class TestBedRender3 implements Injector, TestBed { private _instantiated: boolean = false; private _globalCompilationChecked = false; + private _originalComponentResolutionQueue: Map, Component>|null = null; + // Map that keeps initial version of component/directive/pipe defs in case // we compile a Type again, thus overriding respective static fields. This is // required to make sure we restore defs to their initial states between test runs - private _initiaNgDefs: Map, [string, PropertyDescriptor|undefined]> = new Map(); + private _initialNgDefs: Map, [string, PropertyDescriptor|undefined]> = new Map(); /** * Initialize the environment for testing with a compiler factory, a PlatformRef, and an @@ -337,7 +340,7 @@ export class TestBedRender3 implements Injector, TestBed { this._activeFixtures = []; // restore initial component/directive/pipe defs - this._initiaNgDefs.forEach((value: [string, PropertyDescriptor], type: Type) => { + this._initialNgDefs.forEach((value: [string, PropertyDescriptor], type: Type) => { const [prop, descriptor] = value; if (!descriptor) { // Delete operations are generally undesirable since they have performance implications on @@ -351,8 +354,8 @@ export class TestBedRender3 implements Injector, TestBed { Object.defineProperty(type, prop, descriptor); } }); - this._initiaNgDefs.clear(); - clearResolutionOfComponentResourcesQueue(); + this._initialNgDefs.clear(); + this._restoreComponentResolutionQueue(); } configureCompiler(config: {providers?: any[]; useJit?: boolean;}): void { @@ -383,21 +386,40 @@ export class TestBedRender3 implements Injector, TestBed { } compileComponents(): Promise { + this._clearComponentResolutionQueue(); + const resolvers = this._getResolvers(); const declarations: Type[] = flatten(this._declarations || EMPTY_ARRAY, resolveForwardRef); const componentOverrides: [Type, Component][] = []; + const providerOverrides: (() => void)[] = []; let hasAsyncResources = false; // Compile the components declared by this module + // TODO(FW-1178): `compileComponents` should not duplicate `_compileNgModule` logic declarations.forEach(declaration => { const component = resolvers.component.resolve(declaration); if (component) { - // We make a copy of the metadata to ensure that we don't mutate the original metadata - const metadata = {...component}; - compileComponent(declaration, metadata); - componentOverrides.push([declaration, metadata]); - hasAsyncResources = hasAsyncResources || componentNeedsResolution(component); + if (!declaration.hasOwnProperty(NG_COMPONENT_DEF) || + isComponentDefPendingResolution(declaration) || // + // Compiler provider overrides (like ResourceLoader) might affect the outcome of + // compilation, so we trigger `compileComponent` in case we have compilers overrides. + this._compilerProviders.length > 0 || + this._hasTypeOverrides(declaration, this._componentOverrides) || + this._hasTemplateOverrides(declaration)) { + this._storeNgDef(NG_COMPONENT_DEF, declaration); + // We make a copy of the metadata to ensure that we don't mutate the original metadata + const metadata = {...component}; + compileComponent(declaration, metadata); + componentOverrides.push([declaration, metadata]); + hasAsyncResources = hasAsyncResources || componentNeedsResolution(component); + } else if (this._hasProviderOverrides(component.providers)) { + // Queue provider override operations, since fetching ngComponentDef (to patch it) might + // trigger re-compilation, which will fail because component resources are not yet fully + // resolved at this moment. The queue is drained once all resources are resolved. + providerOverrides.push( + () => this._patchDefWithProviderOverrides(declaration, NG_COMPONENT_DEF)); + } } }); @@ -407,6 +429,7 @@ export class TestBedRender3 implements Injector, TestBed { // are only available until the next TestBed reset (when `resetTestingModule` is called) this.overrideComponent(override[0], {set: override[1]}); }); + providerOverrides.forEach((overrideFn: () => void) => overrideFn()); }; // If the component has no async resources (templateUrl, styleUrls), we can finish @@ -558,9 +581,9 @@ export class TestBedRender3 implements Injector, TestBed { } private _storeNgDef(prop: string, type: Type) { - if (!this._initiaNgDefs.has(type)) { + if (!this._initialNgDefs.has(type)) { const currentDef = Object.getOwnPropertyDescriptor(type, prop); - this._initiaNgDefs.set(type, [prop, currentDef]); + this._initialNgDefs.set(type, [prop, currentDef]); } } @@ -651,16 +674,56 @@ export class TestBedRender3 implements Injector, TestBed { return this._compilerInjector; } + /** + * Clears current components resolution queue, but stores the state of the queue, so we can + * restore it later. Clearing the queue is required before we try to compile components (via + * `TestBed.compileComponents`), so that component defs are in sync with the resolution queue. + */ + private _clearComponentResolutionQueue() { + if (this._originalComponentResolutionQueue === null) { + this._originalComponentResolutionQueue = new Map(); + } + clearResolutionOfComponentResourcesQueue().forEach( + (value, key) => this._originalComponentResolutionQueue !.set(key, value)); + } + + /** + * Restores component resolution queue to the previously saved state. This operation is performed + * as a part of restoring the state after completion of the current set of tests (that might + * potentially mutate the state). + */ + private _restoreComponentResolutionQueue() { + if (this._originalComponentResolutionQueue !== null) { + restoreComponentResolutionQueue(this._originalComponentResolutionQueue); + this._originalComponentResolutionQueue = null; + } + } + + // TODO(FW-1179): define better types for all Provider-related operations, avoid using `any`. + private _getProvidersOverrides(providers: any): any[] { + if (!providers || !providers.length) return []; + // There are two flattening operations here. The inner flatten() operates on the metadata's + // providers and applies a mapping function which retrieves overrides for each incoming + // provider. The outer flatten() then flattens the produced overrides array. If this is not + // done, the array can contain other empty arrays (e.g. `[[], []]`) which leak into the + // providers array and contaminate any error messages that might be generated. + return flatten(flatten(providers, (provider: any) => this._getProviderOverrides(provider))); + } + + private _hasProviderOverrides(providers: any) { + return this._getProvidersOverrides(providers).length > 0; + } + + private _hasTypeOverrides(type: Type, overrides: [Type, MetadataOverride][]) { + return overrides.some((override: [Type, MetadataOverride]) => override[0] === type); + } + + private _hasTemplateOverrides(type: Type) { return this._templateOverrides.has(type); } + private _getMetaWithOverrides(meta: Component|Directive|NgModule, type?: Type) { const overrides: {providers?: any[], template?: string} = {}; if (meta.providers && meta.providers.length) { - // There are two flattening operations here. The inner flatten() operates on the metadata's - // providers and applies a mapping function which retrieves overrides for each incoming - // provider. The outer flatten() then flattens the produced overrides array. If this is not - // done, the array can contain other empty arrays (e.g. `[[], []]`) which leak into the - // providers array and contaminate any error messages that might be generated. - const providerOverrides = - flatten(flatten(meta.providers, (provider: any) => this._getProviderOverrides(provider))); + const providerOverrides = this._getProvidersOverrides(meta.providers); if (providerOverrides.length) { overrides.providers = [...meta.providers, ...providerOverrides]; } @@ -672,6 +735,19 @@ export class TestBedRender3 implements Injector, TestBed { return Object.keys(overrides).length ? {...meta, ...overrides} : meta; } + private _patchDefWithProviderOverrides(declaration: Type, field: string) { + const def = (declaration as any)[field]; + if (def && def.providersResolver) { + this._storeNgDef(field, declaration); + const resolver = def.providersResolver; + const processProvidersFn = (providers: any[]) => { + const overrides = this._getProvidersOverrides(providers); + return [...providers, ...overrides]; + }; + def.providersResolver = (ngDef: DirectiveDef) => resolver(ngDef, processProvidersFn); + } + } + /** * @internal */ @@ -694,31 +770,45 @@ export class TestBedRender3 implements Injector, TestBed { const declarations: Type[] = flatten(ngModule.declarations || EMPTY_ARRAY, resolveForwardRef); - const compiledComponents: Type[] = []; + const declaredComponents: Type[] = []; // Compile the components, directives and pipes declared by this module declarations.forEach(declaration => { const component = this._resolvers.component.resolve(declaration); if (component) { - this._storeNgDef(NG_COMPONENT_DEF, declaration); - const metadata = this._getMetaWithOverrides(component, declaration); - compileComponent(declaration, metadata); - compiledComponents.push(declaration); + if (!declaration.hasOwnProperty(NG_COMPONENT_DEF) || + this._hasTypeOverrides(declaration, this._componentOverrides) || + this._hasTemplateOverrides(declaration)) { + this._storeNgDef(NG_COMPONENT_DEF, declaration); + const metadata = this._getMetaWithOverrides(component, declaration); + compileComponent(declaration, metadata); + } else if (this._hasProviderOverrides(component.providers)) { + this._patchDefWithProviderOverrides(declaration, NG_COMPONENT_DEF); + } + declaredComponents.push(declaration); return; } const directive = this._resolvers.directive.resolve(declaration); if (directive) { - this._storeNgDef(NG_DIRECTIVE_DEF, declaration); - const metadata = this._getMetaWithOverrides(directive); - compileDirective(declaration, metadata); + if (!declaration.hasOwnProperty(NG_DIRECTIVE_DEF) || + this._hasTypeOverrides(declaration, this._directiveOverrides)) { + this._storeNgDef(NG_DIRECTIVE_DEF, declaration); + const metadata = this._getMetaWithOverrides(directive); + compileDirective(declaration, metadata); + } else if (this._hasProviderOverrides(directive.providers)) { + this._patchDefWithProviderOverrides(declaration, NG_DIRECTIVE_DEF); + } return; } const pipe = this._resolvers.pipe.resolve(declaration); if (pipe) { - this._storeNgDef(NG_PIPE_DEF, declaration); - compilePipe(declaration, pipe); + if (!declaration.hasOwnProperty(NG_PIPE_DEF) || + this._hasTypeOverrides(declaration, this._pipeOverrides)) { + this._storeNgDef(NG_PIPE_DEF, declaration); + compilePipe(declaration, pipe); + } return; } }); @@ -727,7 +817,7 @@ export class TestBedRender3 implements Injector, TestBed { const calcTransitiveScopesFor = (moduleType: NgModuleType) => transitiveScopesFor( moduleType, (ngModule: NgModuleType) => this._compileNgModule(ngModule)); const transitiveScope = calcTransitiveScopesFor(moduleType); - compiledComponents.forEach(cmp => { + declaredComponents.forEach(cmp => { const scope = this._templateOverrides.has(cmp) ? // if we have template override via `TestBed.overrideTemplateUsingTestingModule` - // define Component scope as TestingModule scope, instead of the scope of NgModule diff --git a/packages/platform-browser-dynamic/test/testing_public_browser_spec.ts b/packages/platform-browser-dynamic/test/testing_public_browser_spec.ts index 0c54024787..993a564b10 100644 --- a/packages/platform-browser-dynamic/test/testing_public_browser_spec.ts +++ b/packages/platform-browser-dynamic/test/testing_public_browser_spec.ts @@ -144,9 +144,7 @@ if (isBrowser) { }); }), 10000); // Long timeout here because this test makes an actual ResourceLoader - // request, and - // is slow - // on Edge. + // request, and is slow on Edge. }); }); } diff --git a/packages/platform-browser/test/testing_public_spec.ts b/packages/platform-browser/test/testing_public_spec.ts index ae85c71ecf..77abf5fba4 100644 --- a/packages/platform-browser/test/testing_public_spec.ts +++ b/packages/platform-browser/test/testing_public_spec.ts @@ -900,17 +900,24 @@ class CompWithUrlTemplate { () => { const itPromise = patchJasmineIt(); + @Component({ + selector: 'comp', + templateUrl: '/base/angular/packages/platform-browser/test/static_assets/test.html' + }) + class InlineCompWithUrlTemplate { + } + expect( - () => - it('should fail', withModule( - {declarations: [CompWithUrlTemplate]}, - () => TestBed.createComponent(CompWithUrlTemplate)))) + () => it( + 'should fail', withModule( + {declarations: [InlineCompWithUrlTemplate]}, + () => TestBed.createComponent(InlineCompWithUrlTemplate)))) .toThrowError( ivyEnabled ? - `Component 'CompWithUrlTemplate' is not resolved: + `Component 'InlineCompWithUrlTemplate' is not resolved: - templateUrl: /base/angular/packages/platform-browser/test/static_assets/test.html Did you run and wait for 'resolveComponentResources()'?` : - `This test module uses the component ${stringify(CompWithUrlTemplate)} which is using a "templateUrl" or "styleUrls", but they were never compiled. ` + + `This test module uses the component ${stringify(InlineCompWithUrlTemplate)} which is using a "templateUrl" or "styleUrls", but they were never compiled. ` + `Please call "TestBed.compileComponents" before your test.`); restoreJasmineIt();