From 6b859daea49be41195bb8d93f1d02fc09b8bc42a Mon Sep 17 00:00:00 2001 From: Vikram Subramanian Date: Sun, 22 Jul 2018 00:05:53 -0700 Subject: [PATCH] fix(core): stop reusing provider definitions across NgModuleRef instances (#25022) Fixes #25018. Instantiating a NgModuleRef from NgModuleFactory reuses the NgModuleDefinition if it is already present. However the NgModuleDefinition has a providers array which modified when tree shakable providers are instantiated. This corrupts the provider definitions the next time the same factory is used to create a new NgModuleRef - Two provider definitions can end up with the same index anf the injector could potentially return a completely wrong object for a provider token. This scenario is more likely on the server where the same NgModuleFactory is reused across requests. The fix clones the cached NgModuleDefinition so that any tree shakable providers added later do not affect the cached copy. PR Close #25022 --- packages/core/src/view/entrypoint.ts | 21 +++++++++- .../hello_world_r2/bundle.golden_symbols.json | 3 ++ .../test/linker/ng_module_integration_spec.ts | 42 ++++++++++++++++++- 3 files changed, 62 insertions(+), 4 deletions(-) diff --git a/packages/core/src/view/entrypoint.ts b/packages/core/src/view/entrypoint.ts index 7bd118e6cf..ff2933baf1 100644 --- a/packages/core/src/view/entrypoint.ts +++ b/packages/core/src/view/entrypoint.ts @@ -12,7 +12,7 @@ import {NgModuleFactory, NgModuleRef} from '../linker/ng_module_factory'; import {Type} from '../type'; import {initServicesIfNeeded} from './services'; -import {NgModuleDefinitionFactory, ProviderOverride, Services, ViewDefinition} from './types'; +import {NgModuleDefinition, NgModuleDefinitionFactory, NgModuleProviderDef, ProviderOverride, Services, ViewDefinition} from './types'; import {resolveDefinition} from './util'; export function overrideProvider(override: ProviderOverride) { @@ -38,6 +38,20 @@ export function createNgModuleFactory( return new NgModuleFactory_(ngModuleType, bootstrapComponents, defFactory); } +function cloneNgModuleDefinition(def: NgModuleDefinition): NgModuleDefinition { + const providers = Array.from(def.providers); + const modules = Array.from(def.modules); + const providersByKey: {[tokenKey: string]: NgModuleProviderDef} = {}; + for (const key in def.providersByKey) { + providersByKey[key] = def.providersByKey[key]; + } + + return { + factory: def.factory, + isRoot: def.isRoot, providers, modules, providersByKey, + }; +} + class NgModuleFactory_ extends NgModuleFactory { constructor( public readonly moduleType: Type, private _bootstrapComponents: Type[], @@ -49,7 +63,10 @@ class NgModuleFactory_ extends NgModuleFactory { create(parentInjector: Injector|null): NgModuleRef { initServicesIfNeeded(); - const def = resolveDefinition(this._ngModuleDefFactory); + // Clone the NgModuleDefinition so that any tree shakeable provider definition + // added to this instance of the NgModuleRef doesn't affect the cached copy. + // See https://github.com/angular/angular/issues/25018. + const def = cloneNgModuleDefinition(resolveDefinition(this._ngModuleDefFactory)); return Services.createNgModuleRef( this.moduleType, parentInjector || Injector.NULL, this._bootstrapComponents, def); } diff --git a/packages/core/test/bundling/hello_world_r2/bundle.golden_symbols.json b/packages/core/test/bundling/hello_world_r2/bundle.golden_symbols.json index cfb0615eeb..cabdda2fb0 100644 --- a/packages/core/test/bundling/hello_world_r2/bundle.golden_symbols.json +++ b/packages/core/test/bundling/hello_world_r2/bundle.golden_symbols.json @@ -2414,6 +2414,9 @@ { "name": "checkStable" }, + { + "name": "cloneNgModuleDefinition" + }, { "name": "collectReferences" }, diff --git a/packages/core/test/linker/ng_module_integration_spec.ts b/packages/core/test/linker/ng_module_integration_spec.ts index 45f530f76d..6a9369abd7 100644 --- a/packages/core/test/linker/ng_module_integration_spec.ts +++ b/packages/core/test/linker/ng_module_integration_spec.ts @@ -8,10 +8,13 @@ import {ANALYZE_FOR_ENTRY_COMPONENTS, CUSTOM_ELEMENTS_SCHEMA, Compiler, Component, ComponentFactoryResolver, Directive, HostBinding, Inject, Injectable, InjectionToken, Injector, Input, NgModule, NgModuleRef, Optional, Pipe, Provider, Self, Type, forwardRef, getModuleFactory} from '@angular/core'; import {Console} from '@angular/core/src/console'; +import {InjectableDef, defineInjectable} from '@angular/core/src/di/defs'; +import {NgModuleData} from '@angular/core/src/view/types'; +import {tokenKey} from '@angular/core/src/view/util'; import {ComponentFixture, TestBed, inject} from '@angular/core/testing'; import {expect} from '@angular/platform-browser/testing/src/matchers'; -import {InternalNgModuleRef} from '../../src/linker/ng_module_factory'; +import {InternalNgModuleRef, NgModuleFactory} from '../../src/linker/ng_module_factory'; import {clearModulesForTest} from '../../src/linker/ng_module_factory_loader'; import {stringify} from '../../src/util'; @@ -117,9 +120,13 @@ function declareTests({useJit}: {useJit: boolean}) { injector = _injector; })); + function createModuleFactory(moduleType: Type): NgModuleFactory { + return compiler.compileModuleSync(moduleType); + } + function createModule( moduleType: Type, parentInjector?: Injector | null): NgModuleRef { - return compiler.compileModuleSync(moduleType).create(parentInjector || null); + return createModuleFactory(moduleType).create(parentInjector || null); } function createComp(compType: Type, moduleType: Type): ComponentFixture { @@ -1290,6 +1297,37 @@ function declareTests({useJit}: {useJit: boolean}) { `Invalid provider for the NgModule 'ImportedModule1' - only instances of Provider and Type are allowed, got: [?broken?]`); }); }); + + describe('tree shakable providers', () => { + it('definition should not persist across NgModuleRef instances', () => { + @NgModule() + class SomeModule { + } + + class Bar { + static ngInjectableDef: InjectableDef = defineInjectable({ + factory: () => new Bar(), + providedIn: SomeModule, + }); + } + + const factory = createModuleFactory(SomeModule); + const ngModuleRef1 = factory.create(null); + + // Inject a tree shakeable provider token. + ngModuleRef1.injector.get(Bar); + + // Tree Shakeable provider definition should get added to the NgModule data. + const providerDef1 = (ngModuleRef1 as NgModuleData)._def.providersByKey[tokenKey(Bar)]; + expect(providerDef1).not.toBeUndefined(); + + // Instantiate the same module. The tree shakeable provider + // definition should not already be present. + const ngModuleRef2 = factory.create(null); + const providerDef2 = (ngModuleRef2 as NgModuleData)._def.providersByKey[tokenKey(Bar)]; + expect(providerDef2).toBeUndefined(); + }); + }); }); }); }