Skip to content

Commit

Permalink
fix(ngcc): render new definitions using the inner name of the class (#…
Browse files Browse the repository at this point in the history 
…33533)

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
  • Loading branch information
@petebacondarwin @atscott
petebacondarwin authored and atscott committed Nov 5, 2019
1 parent 93a23b9 commit 85298e3
Show file tree
Hide file tree
Showing 3 changed files with 156 additions and 32 deletions.
2 changes: 1 addition & 1 deletion packages/compiler-cli/ngcc/src/packages/transformer.ts
View file
Expand Up @@ -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);

Expand Down
56 changes: 29 additions & 27 deletions packages/compiler-cli/ngcc/src/rendering/renderer.ts
View file
Expand Up @@ -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`.
Expand All @@ -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,
Expand Down Expand Up @@ -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) {
Expand Down Expand Up @@ -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');
}
}

/**
Expand All @@ -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.
Expand Down
130 changes: 126 additions & 4 deletions packages/compiler-cli/ngcc/test/rendering/renderer_spec.ts
View file
Expand Up @@ -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';
Expand Down Expand Up @@ -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) {
Expand All @@ -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();
Expand All @@ -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,
Expand Down Expand Up @@ -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} =
Expand Down

0 comments on commit 85298e3

Please sign in to comment.