From 755d2d572f00efa4b7362c71d0299bef1655788f Mon Sep 17 00:00:00 2001 From: Kara Erickson Date: Wed, 27 Nov 2019 15:52:34 -0800 Subject: [PATCH] refactor(ivy): remove unnecessary fac wrapper (#34076) For injectables, we currently generate a factory function in the injectable def (prov) that delegates to the factory function in the factory def (fac). It looks something like this: ``` factory: function(t) { return Svc.fac(t); } ``` The extra wrapper function is unnecessary since the args for the factory functions are the same. This commit changes the compiler to generate this instead: ``` factory: Svc.fac ``` Because we are generating less code for each injectable, we should see some modest code size savings. AIO's main bundle is about 1 KB smaller. PR Close #34076 --- aio/scripts/_payload-limits.json | 2 +- integration/_payload-limits.json | 4 +-- .../ngcc/src/analysis/decoration_analyzer.ts | 8 +++-- .../ngcc/test/integration/ngcc_spec.ts | 34 +++++++++++++++++-- packages/compiler-cli/src/ngtsc/program.ts | 6 ++-- .../compliance/r3_view_compiler_di_spec.ts | 20 ++++++----- .../compiler/src/injectable_compiler_2.ts | 18 ++++++---- 7 files changed, 68 insertions(+), 24 deletions(-) diff --git a/aio/scripts/_payload-limits.json b/aio/scripts/_payload-limits.json index 07ad2dff4d..13fe378848 100755 --- a/aio/scripts/_payload-limits.json +++ b/aio/scripts/_payload-limits.json @@ -12,7 +12,7 @@ "master": { "uncompressed": { "runtime-es2015": 2987, - "main-es2015": 463671, + "main-es2015": 462449, "polyfills-es2015": 52503 } } diff --git a/integration/_payload-limits.json b/integration/_payload-limits.json index 899cd7079a..0fc50bab35 100644 --- a/integration/_payload-limits.json +++ b/integration/_payload-limits.json @@ -3,7 +3,7 @@ "master": { "uncompressed": { "runtime-es2015": 1485, - "main-es2015": 142186, + "main-es2015": 141476, "polyfills-es2015": 36808 } } @@ -21,7 +21,7 @@ "master": { "uncompressed": { "runtime-es2015": 1485, - "main-es2015": 148320, + "main-es2015": 147610, "polyfills-es2015": 36808 } } diff --git a/packages/compiler-cli/ngcc/src/analysis/decoration_analyzer.ts b/packages/compiler-cli/ngcc/src/analysis/decoration_analyzer.ts index 83ccdeae2e..0c0b211006 100644 --- a/packages/compiler-cli/ngcc/src/analysis/decoration_analyzer.ts +++ b/packages/compiler-cli/ngcc/src/analysis/decoration_analyzer.ts @@ -94,6 +94,11 @@ export class DecorationAnalyzer { new DirectiveDecoratorHandler( this.reflectionHost, this.evaluator, this.fullRegistry, NOOP_DEFAULT_IMPORT_RECORDER, this.isCore, /* annotateForClosureCompiler */ false), + // Pipe handler must be before injectable handler in list so pipe factories are printed + // before injectable factories (so injectable factories can delegate to them) + new PipeDecoratorHandler( + this.reflectionHost, this.evaluator, this.metaRegistry, NOOP_DEFAULT_IMPORT_RECORDER, + this.isCore), new InjectableDecoratorHandler( this.reflectionHost, NOOP_DEFAULT_IMPORT_RECORDER, this.isCore, /* strictCtorDeps */ false, /* errorOnDuplicateProv */ false), @@ -101,9 +106,6 @@ export class DecorationAnalyzer { this.reflectionHost, this.evaluator, this.fullMetaReader, this.fullRegistry, this.scopeRegistry, this.referencesRegistry, this.isCore, /* routeAnalyzer */ null, this.refEmitter, NOOP_DEFAULT_IMPORT_RECORDER, /* annotateForClosureCompiler */ false), - new PipeDecoratorHandler( - this.reflectionHost, this.evaluator, this.metaRegistry, NOOP_DEFAULT_IMPORT_RECORDER, - this.isCore), ]; migrations: Migration[] = [ new UndecoratedParentMigration(), diff --git a/packages/compiler-cli/ngcc/test/integration/ngcc_spec.ts b/packages/compiler-cli/ngcc/test/integration/ngcc_spec.ts index 23dbc31670..8b161ac45e 100644 --- a/packages/compiler-cli/ngcc/test/integration/ngcc_spec.ts +++ b/packages/compiler-cli/ngcc/test/integration/ngcc_spec.ts @@ -205,13 +205,43 @@ runInEachFileSystem(() => { expect(countOccurrences(after, 'ɵfac')).toEqual(1); }); + // This is necessary to ensure XPipeDef.fac is defined when delegated from injectable def + it('should always generate factory def (fac) before injectable def (prov)', () => { + compileIntoFlatEs5Package('test-package', { + '/index.ts': ` + import {Injectable, Pipe, PipeTransform} from '@angular/core'; + + @Injectable() + @Pipe({ + name: 'myTestPipe' + }) + export class TestClass implements PipeTransform { + transform(value: any) { return value; } + } + `, + }); + + mainNgcc({ + basePath: '/node_modules', + targetEntryPointPath: 'test-package', + propertiesToConsider: ['main'], + }); + + const jsContents = fs.readFile(_(`/node_modules/test-package/index.js`)); + expect(jsContents) + .toContain( + `TestClass.ɵfac = function TestClass_Factory(t) { return new (t || TestClass)(); };\n` + + `TestClass.ɵpipe = ɵngcc0.ɵɵdefinePipe({ name: "myTestPipe", type: TestClass, pure: true });\n` + + `TestClass.ɵprov = ɵngcc0.ɵɵdefineInjectable({`); + }); + it('should add generic type for ModuleWithProviders and generate exports for private modules', () => { compileIntoApf('test-package', { '/index.ts': ` import {ModuleWithProviders} from '@angular/core'; import {InternalFooModule} from './internal'; - + export class FooModule { static forRoot(): ModuleWithProviders { return { @@ -222,7 +252,7 @@ runInEachFileSystem(() => { `, '/internal.ts': ` import {NgModule} from '@angular/core'; - + @NgModule() export class InternalFooModule {} `, diff --git a/packages/compiler-cli/src/ngtsc/program.ts b/packages/compiler-cli/src/ngtsc/program.ts index 38ddb48f37..9cc86026ee 100644 --- a/packages/compiler-cli/src/ngtsc/program.ts +++ b/packages/compiler-cli/src/ngtsc/program.ts @@ -629,6 +629,10 @@ export class NgtscProgram implements api.Program { new DirectiveDecoratorHandler( this.reflector, evaluator, metaRegistry, this.defaultImportTracker, this.isCore, this.closureCompilerEnabled), + // Pipe handler must be before injectable handler in list so pipe factories are printed + // before injectable factories (so injectable factories can delegate to them) + new PipeDecoratorHandler( + this.reflector, evaluator, metaRegistry, this.defaultImportTracker, this.isCore), new InjectableDecoratorHandler( this.reflector, this.defaultImportTracker, this.isCore, this.options.strictInjectionParameters || false), @@ -636,8 +640,6 @@ export class NgtscProgram implements api.Program { this.reflector, evaluator, this.metaReader, metaRegistry, scopeRegistry, referencesRegistry, this.isCore, this.routeAnalyzer, this.refEmitter, this.defaultImportTracker, this.closureCompilerEnabled, this.options.i18nInLocale), - new PipeDecoratorHandler( - this.reflector, evaluator, metaRegistry, this.defaultImportTracker, this.isCore), ]; return new IvyCompilation( diff --git a/packages/compiler-cli/test/compliance/r3_view_compiler_di_spec.ts b/packages/compiler-cli/test/compliance/r3_view_compiler_di_spec.ts index 8394d0424d..a777d79098 100644 --- a/packages/compiler-cli/test/compliance/r3_view_compiler_di_spec.ts +++ b/packages/compiler-cli/test/compliance/r3_view_compiler_di_spec.ts @@ -90,9 +90,7 @@ describe('compiler compliance: dependency injection', () => { const def = ` MyService.ɵprov = $r3$.ɵɵdefineInjectable({ token: MyService, - factory: function(t) { - return MyService.ɵfac(t); - }, + factory: MyService.ɵfac, providedIn: null }); `; @@ -342,16 +340,22 @@ describe('compiler compliance: dependency injection', () => { const result = compile(files, angularFiles); const source = result.source; - const MyPipeFactory = ` - MyPipe.ɵfac = function MyPipe_Factory(t) { return new (t || MyPipe)($r3$.ɵɵdirectiveInject(Service)); }; + // The prov definition must be last so MyPipe.fac is defined + const MyPipeDefs = ` + MyPipe.ɵfac = function MyPipe_Factory(t) { return new (t || MyPipe)(i0.ɵɵdirectiveInject(Service)); }; + MyPipe.ɵpipe = i0.ɵɵdefinePipe({ name: "myPipe", type: MyPipe, pure: true }); + MyPipe.ɵprov = i0.ɵɵdefineInjectable({ token: MyPipe, factory: MyPipe.ɵfac, providedIn: null }); `; - const MyOtherPipeFactory = ` + // The prov definition must be last so MyOtherPipe.fac is defined + const MyOtherPipeDefs = ` MyOtherPipe.ɵfac = function MyOtherPipe_Factory(t) { return new (t || MyOtherPipe)($r3$.ɵɵdirectiveInject(Service)); }; + MyOtherPipe.ɵpipe = i0.ɵɵdefinePipe({ name: "myOtherPipe", type: MyOtherPipe, pure: true }); + MyOtherPipe.ɵprov = i0.ɵɵdefineInjectable({ token: MyOtherPipe, factory: MyOtherPipe.ɵfac, providedIn: null }); `; - expectEmit(source, MyPipeFactory, 'Invalid pipe factory function'); - expectEmit(source, MyOtherPipeFactory, 'Invalid pipe factory function'); + expectEmit(source, MyPipeDefs, 'Invalid pipe factory function'); + expectEmit(source, MyOtherPipeDefs, 'Invalid pipe factory function'); expect(source.match(/MyPipe\.ɵfac =/g) !.length).toBe(1); expect(source.match(/MyOtherPipe\.ɵfac =/g) !.length).toBe(1); }); diff --git a/packages/compiler/src/injectable_compiler_2.ts b/packages/compiler/src/injectable_compiler_2.ts index 2f1c7b2ef9..dfd0cfe704 100644 --- a/packages/compiler/src/injectable_compiler_2.ts +++ b/packages/compiler/src/injectable_compiler_2.ts @@ -68,7 +68,8 @@ export function compileInjectable(meta: R3InjectableMetadata): InjectableDef { } else if (useClassOnSelf) { result = compileFactoryFunction(factoryMeta); } else { - result = delegateToFactory(meta.useClass); + result = delegateToFactory( + meta.type as o.WrappedNodeExpr, meta.useClass as o.WrappedNodeExpr); } } else if (meta.useFactory !== undefined) { if (meta.userDeps !== undefined) { @@ -99,7 +100,8 @@ export function compileInjectable(meta: R3InjectableMetadata): InjectableDef { expression: o.importExpr(Identifiers.inject).callFn([meta.useExisting]), }); } else { - result = delegateToFactory(meta.internalType); + result = delegateToFactory( + meta.type as o.WrappedNodeExpr, meta.internalType as o.WrappedNodeExpr); } const token = meta.internalType; @@ -117,11 +119,15 @@ export function compileInjectable(meta: R3InjectableMetadata): InjectableDef { }; } -function delegateToFactory(type: o.Expression) { +function delegateToFactory(type: o.WrappedNodeExpr, internalType: o.WrappedNodeExpr) { return { statements: [], - // () => type.ɵfac(t) - factory: o.fn([new o.FnParam('t', o.DYNAMIC_TYPE)], [new o.ReturnStatement(type.callMethod( - 'ɵfac', [o.variable('t')]))]) + // If types are the same, we can generate `factory: type.ɵfac` + // If types are different, we have to generate a wrapper function to ensure + // the internal type has been resolved (`factory: function(t) { return type.ɵfac(t); }`) + factory: type.node === internalType.node ? + internalType.prop('ɵfac') : + o.fn([new o.FnParam('t', o.DYNAMIC_TYPE)], [new o.ReturnStatement(internalType.callMethod( + 'ɵfac', [o.variable('t')]))]) }; }