From 22b836338029127d344397d50516c3c8e183d677 Mon Sep 17 00:00:00 2001 From: Pete Bacon Darwin Date: Fri, 1 Nov 2019 16:55:10 +0000 Subject: [PATCH] fix(ngcc): render new definitions using the inner name of the class MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit When decorating classes with ivy definitions (e.g. `ɵfac` or `ɵdir`) the inner name of the class declaration must be used. This is because in ES5 the definitions are inside the class's IIFE where the outer declaration has not yet been initialized. --- .../ngcc/src/packages/transformer.ts | 2 +- .../ngcc/src/rendering/renderer.ts | 56 ++++---- .../ngcc/test/rendering/renderer_spec.ts | 124 +++++++++++++++++- 3 files changed, 150 insertions(+), 32 deletions(-) diff --git a/packages/compiler-cli/ngcc/src/packages/transformer.ts b/packages/compiler-cli/ngcc/src/packages/transformer.ts index 34480ad3e72e8f..06968990fd756c 100644 --- a/packages/compiler-cli/ngcc/src/packages/transformer.ts +++ b/packages/compiler-cli/ngcc/src/packages/transformer.ts @@ -83,7 +83,7 @@ export class Transformer { // Transform the source files and source maps. const srcFormatter = this.getRenderingFormatter(reflectionHost, bundle); - const renderer = new Renderer(srcFormatter, this.fs, this.logger, bundle); + const renderer = new Renderer(reflectionHost, srcFormatter, this.fs, this.logger, bundle); let renderedFiles = renderer.renderProgram( decorationAnalyses, switchMarkerAnalyses, privateDeclarationsAnalyses); diff --git a/packages/compiler-cli/ngcc/src/rendering/renderer.ts b/packages/compiler-cli/ngcc/src/rendering/renderer.ts index 171c1ec27eba9a..15603995533214 100644 --- a/packages/compiler-cli/ngcc/src/rendering/renderer.ts +++ b/packages/compiler-cli/ngcc/src/rendering/renderer.ts @@ -20,6 +20,7 @@ import {Logger} from '../logging/logger'; import {FileToWrite, getImportRewriter, stripExtension} from './utils'; import {RenderingFormatter, RedundantDecoratorMap} from './rendering_formatter'; import {extractSourceMap, renderSourceAndMap} from './source_maps'; +import {NgccReflectionHost} from '../host/ngcc_host'; /** * A base-class for rendering an `AnalyzedFile`. @@ -29,8 +30,8 @@ import {extractSourceMap, renderSourceAndMap} from './source_maps'; */ export class Renderer { constructor( - private srcFormatter: RenderingFormatter, private fs: FileSystem, private logger: Logger, - private bundle: EntryPointBundle) {} + private host: NgccReflectionHost, private srcFormatter: RenderingFormatter, + private fs: FileSystem, private logger: Logger, private bundle: EntryPointBundle) {} renderProgram( decorationAnalyses: DecorationAnalyses, switchMarkerAnalyses: SwitchMarkerAnalyses, @@ -81,7 +82,8 @@ export class Renderer { this.srcFormatter.removeDecorators(outputText, decoratorsToRemove); compiledFile.compiledClasses.forEach(clazz => { - const renderedDefinition = renderDefinitions(compiledFile.sourceFile, clazz, importManager); + const renderedDefinition = + this.renderDefinitions(compiledFile.sourceFile, clazz, importManager); this.srcFormatter.addDefinitions(outputText, clazz, renderedDefinition); if (!isEntryPoint && clazz.reexports.length > 0) { @@ -143,6 +145,30 @@ export class Renderer { }); return decoratorsToRemove; } + + /** + * Render the definitions as source code for the given class. + * @param sourceFile The file containing the class to process. + * @param clazz The class whose definitions are to be rendered. + * @param compilation The results of analyzing the class - this is used to generate the rendered + * definitions. + * @param imports An object that tracks the imports that are needed by the rendered definitions. + */ + private renderDefinitions( + sourceFile: ts.SourceFile, compiledClass: CompiledClass, imports: ImportManager): string { + const printer = createPrinter(); + const name = this.host.getInternalNameOfClass(compiledClass.declaration); + const translate = (stmt: Statement) => + translateStatement(stmt, imports, NOOP_DEFAULT_IMPORT_RECORDER); + const print = (stmt: Statement) => + printer.printNode(ts.EmitHint.Unspecified, translate(stmt), sourceFile); + const statements: Statement[] = compiledClass.compilation.map( + c => { return createAssignmentStatement(name, c.name, c.initializer); }); + for (const c of compiledClass.compilation) { + statements.push(...c.statements); + } + return statements.map(print).join('\n'); + } } /** @@ -157,30 +183,6 @@ export function renderConstantPool( .join('\n'); } -/** - * Render the definitions as source code for the given class. - * @param sourceFile The file containing the class to process. - * @param clazz The class whose definitions are to be rendered. - * @param compilation The results of analyzing the class - this is used to generate the rendered - * definitions. - * @param imports An object that tracks the imports that are needed by the rendered definitions. - */ -export function renderDefinitions( - sourceFile: ts.SourceFile, compiledClass: CompiledClass, imports: ImportManager): string { - const printer = createPrinter(); - const name = compiledClass.declaration.name; - const translate = (stmt: Statement) => - translateStatement(stmt, imports, NOOP_DEFAULT_IMPORT_RECORDER); - const print = (stmt: Statement) => - printer.printNode(ts.EmitHint.Unspecified, translate(stmt), sourceFile); - const statements: Statement[] = - compiledClass.compilation.map(c => createAssignmentStatement(name, c.name, c.initializer)); - for (const c of compiledClass.compilation) { - statements.push(...c.statements); - } - return statements.map(print).join('\n'); -} - /** * Create an Angular AST statement node that contains the assignment of the * compiled decorator to be applied to the class. diff --git a/packages/compiler-cli/ngcc/test/rendering/renderer_spec.ts b/packages/compiler-cli/ngcc/test/rendering/renderer_spec.ts index aab86206d3f710..dc62092aedd130 100644 --- a/packages/compiler-cli/ngcc/test/rendering/renderer_spec.ts +++ b/packages/compiler-cli/ngcc/test/rendering/renderer_spec.ts @@ -20,6 +20,7 @@ import {ModuleWithProvidersInfo} from '../../src/analysis/module_with_providers_ import {PrivateDeclarationsAnalyzer, ExportInfo} from '../../src/analysis/private_declarations_analyzer'; import {SwitchMarkerAnalyzer} from '../../src/analysis/switch_marker_analyzer'; import {Esm2015ReflectionHost} from '../../src/host/esm2015_host'; +import {Esm5ReflectionHost} from '../../src/host/esm5_host'; import {Renderer} from '../../src/rendering/renderer'; import {MockLogger} from '../helpers/mock_logger'; import {RenderingFormatter, RedundantDecoratorMap} from '../../src/rendering/rendering_formatter'; @@ -55,7 +56,8 @@ class TestRenderingFormatter implements RenderingFormatter { } function createTestRenderer( - packageName: string, files: TestFile[], dtsFiles?: TestFile[], mappingFiles?: TestFile[]) { + packageName: string, files: TestFile[], dtsFiles?: TestFile[], mappingFiles?: TestFile[], + isEs5 = false) { const logger = new MockLogger(); loadTestFiles(files); if (dtsFiles) { @@ -67,9 +69,10 @@ function createTestRenderer( const fs = getFileSystem(); const isCore = packageName === '@angular/core'; const bundle = makeTestEntryPointBundle( - 'test-package', 'esm2015', isCore, getRootFiles(files), dtsFiles && getRootFiles(dtsFiles)); + 'test-package', 'esm5', isCore, getRootFiles(files), dtsFiles && getRootFiles(dtsFiles)); const typeChecker = bundle.src.program.getTypeChecker(); - const host = new Esm2015ReflectionHost(logger, isCore, typeChecker, bundle.dts); + const host = isEs5 ? new Esm5ReflectionHost(logger, isCore, typeChecker, bundle.dts) : + new Esm2015ReflectionHost(logger, isCore, typeChecker, bundle.dts); const referencesRegistry = new NgccReferencesRegistry(host); const decorationAnalyses = new DecorationAnalyzer(fs, bundle, host, referencesRegistry).analyzeProgram(); @@ -86,7 +89,7 @@ function createTestRenderer( spyOn(testFormatter, 'rewriteSwitchableDeclarations').and.callThrough(); spyOn(testFormatter, 'addModuleWithProvidersParams').and.callThrough(); - const renderer = new Renderer(testFormatter, fs, logger, bundle); + const renderer = new Renderer(host, testFormatter, fs, logger, bundle); return {renderer, testFormatter, @@ -284,6 +287,119 @@ A.ɵdir = ɵngcc0.ɵɵdefineDirective({ type: A, selectors: [["", "a", ""]] }); .toBeGreaterThan(ngInjectorDef, 'setClassMetadata should follow ɵinj'); }); + it('should render directives using the inner class name if different from outer', () => { + const {renderer, decorationAnalyses, switchMarkerAnalyses, privateDeclarationsAnalyses, + testFormatter} = + createTestRenderer( + 'test-package', [{ + name: _('/node_modules/test-package/src/file.js'), + contents: ` + import { Directive } from '@angular/core'; + var OuterClass = /** @class */ (function () { + function InnerClass() {} + return InnerClass; + }()); + OuterClass.decorators = [{ type: Directive, args: [{ selector: '[test]' }] + export OuterClass;` + }], + undefined, undefined, /* isEs5 */ true); + + renderer.renderProgram( + decorationAnalyses, switchMarkerAnalyses, privateDeclarationsAnalyses); + + const addDefinitionsSpy = testFormatter.addDefinitions as jasmine.Spy; + const output = addDefinitionsSpy.calls.first().args[2]; + expect(output).toContain('InnerClass.ɵfac'); + expect(output).toContain('new (t || InnerClass)'); + expect(output).toContain('InnerClass.ɵdir'); + expect(output).toContain('type: InnerClass'); + expect(output).toContain('ɵsetClassMetadata(InnerClass'); + }); + + it('should render injectables using the inner class name if different from outer', () => { + const {renderer, decorationAnalyses, switchMarkerAnalyses, privateDeclarationsAnalyses, + testFormatter} = + createTestRenderer( + 'test-package', [{ + name: _('/node_modules/test-package/src/file.js'), + contents: ` + import { Injectable } from '@angular/core'; + var OuterClass = /** @class */ (function () { + function InnerClass() {} + return InnerClass; + }()); + OuterClass.decorators = [{ type: Injectable }] + export OuterClass;` + }], + undefined, undefined, /* isEs5 */ true); + + renderer.renderProgram( + decorationAnalyses, switchMarkerAnalyses, privateDeclarationsAnalyses); + + const addDefinitionsSpy = testFormatter.addDefinitions as jasmine.Spy; + const output = addDefinitionsSpy.calls.first().args[2]; + expect(output).toContain('InnerClass.ɵfac'); + expect(output).toContain('new (t || InnerClass)()'); + expect(output).toContain('InnerClass.ɵprov'); + expect(output).toContain('token: InnerClass'); + expect(output).toContain('ɵsetClassMetadata(InnerClass'); + }); + + it('should render ng-modules using the inner class name if different from outer', () => { + const {renderer, decorationAnalyses, switchMarkerAnalyses, privateDeclarationsAnalyses, + testFormatter} = + createTestRenderer( + 'test-package', [{ + name: _('/node_modules/test-package/src/file.js'), + contents: ` + import { NgModule } from '@angular/core'; + var OuterClass = /** @class */ (function () { + function InnerClass() {} + return InnerClass; + }()); + OuterClass.decorators = [{ type: NgModule, args: [{}] + export OuterClass;` + }], + undefined, undefined, /* isEs5 */ true); + + renderer.renderProgram( + decorationAnalyses, switchMarkerAnalyses, privateDeclarationsAnalyses); + + const addDefinitionsSpy = testFormatter.addDefinitions as jasmine.Spy; + const output = addDefinitionsSpy.calls.first().args[2]; + expect(output).toContain('InnerClass.ɵmod'); + expect(output).toContain('type: InnerClass'); + expect(output).toContain('ɵsetClassMetadata(InnerClass'); + }); + + it('should render pipes using the inner class name if different from outer', () => { + const {renderer, decorationAnalyses, switchMarkerAnalyses, privateDeclarationsAnalyses, + testFormatter} = + createTestRenderer( + 'test-package', [{ + name: _('/node_modules/test-package/src/file.js'), + contents: ` + import { Pipe } from '@angular/core'; + var OuterClass = /** @class */ (function () { + function InnerClass() {} + return InnerClass; + }()); + OuterClass.decorators = [{ type: Pipe, args: [{name: 'pipe'}] + export OuterClass;` + }], + undefined, undefined, /* isEs5 */ true); + + renderer.renderProgram( + decorationAnalyses, switchMarkerAnalyses, privateDeclarationsAnalyses); + + const addDefinitionsSpy = testFormatter.addDefinitions as jasmine.Spy; + const output = addDefinitionsSpy.calls.first().args[2]; + expect(output).toContain('InnerClass.ɵfac'); + expect(output).toContain('new (t || InnerClass)()'); + expect(output).toContain('InnerClass.ɵpipe'); + expect(output).toContain('ɵsetClassMetadata(InnerClass'); + }); + it('should render classes without decorators if class fields are decorated', () => { const {renderer, decorationAnalyses, switchMarkerAnalyses, privateDeclarationsAnalyses, testFormatter} =