From 85298e345dba1f44095011003226fe5933a665a3 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 (#33533) 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. PR Close #33533 --- .../ngcc/src/packages/transformer.ts | 2 +- .../ngcc/src/rendering/renderer.ts | 56 ++++---- .../ngcc/test/rendering/renderer_spec.ts | 130 +++++++++++++++++- 3 files changed, 156 insertions(+), 32 deletions(-) diff --git a/packages/compiler-cli/ngcc/src/packages/transformer.ts b/packages/compiler-cli/ngcc/src/packages/transformer.ts index 34480ad3e72e8..06968990fd756 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 171c1ec27eba9..1560399553321 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 aab86206d3f71..968bf13acad95 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,125 @@ 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, Directive } from '@angular/core'; + var DirectiveClass = /** @class */ (function () { + function DirectiveClass() {} + return DirectiveClass; + }()); + DirectiveClass.decorators = [{ type: Directive, args: [{selector: 'x'}] }]; + var OuterClass = /** @class */ (function () { + function InnerClass() {} + return InnerClass; + }()); + OuterClass.decorators = [{ type: NgModule, args: [{declarations: [DirectiveClass], exports: [DirectiveClass]}] + export OuterClass;` + }], + undefined, undefined, /* isEs5 */ true); + + renderer.renderProgram( + decorationAnalyses, switchMarkerAnalyses, privateDeclarationsAnalyses); + + const addDefinitionsSpy = testFormatter.addDefinitions as jasmine.Spy; + const output = addDefinitionsSpy.calls.all()[1].args[2]; + expect(output).toContain('InnerClass.ɵmod'); + expect(output).toContain('type: InnerClass'); + expect(output).toContain('ɵɵsetNgModuleScope(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} =