fix(ivy): use a single constant pool per source file (#25392)

Previously, ngtsc used a new ConstantPool for each decorator
compilation. This could result in collisions between constants in the
top-level scope.

Now, ngtsc uses a single ConstantPool for each source file being
compiled, and merges the constant statements into the file after the
import section.

PR Close #25392
This commit is contained in:
Alex Rickabaugh
2018-06-27 08:41:11 -07:00
committed by Ben Lesh
parent 9c92a6fc7a
commit fba276d3d1
9 changed files with 115 additions and 25 deletions

View File

@ -5,11 +5,14 @@
* Use of this source code is governed by an MIT-style license that can be
* found in the LICENSE file at https://angular.io/license
*/
import {ConstantPool} from '@angular/compiler';
import * as fs from 'fs';
import * as ts from 'typescript';
import {ComponentDecoratorHandler, DirectiveDecoratorHandler, InjectableDecoratorHandler, NgModuleDecoratorHandler, PipeDecoratorHandler, ResourceLoader, SelectorScopeRegistry} from '../../ngtsc/annotations';
import {Decorator} from '../../ngtsc/host';
import {CompileResult, DecoratorHandler} from '../../ngtsc/transform';
import {NgccReflectionHost} from './host/ngcc_host';
import {ParsedClass} from './parsing/parsed_class';
import {ParsedFile} from './parsing/parsed_file';
@ -25,6 +28,7 @@ export interface AnalyzedClass<T = any> extends ParsedClass {
export interface AnalyzedFile {
analyzedClasses: AnalyzedClass[];
sourceFile: ts.SourceFile;
constantPool: ConstantPool;
}
export interface MatchingHandler<T> {
@ -59,17 +63,19 @@ export class Analyzer {
* @param file The file to be analysed for decorated classes.
*/
analyzeFile(file: ParsedFile): AnalyzedFile {
const constantPool = new ConstantPool();
const analyzedClasses =
file.decoratedClasses.map(clazz => this.analyzeClass(file.sourceFile, clazz))
file.decoratedClasses.map(clazz => this.analyzeClass(file.sourceFile, constantPool, clazz))
.filter(isDefined);
return {
analyzedClasses,
sourceFile: file.sourceFile,
sourceFile: file.sourceFile, constantPool,
};
}
protected analyzeClass(file: ts.SourceFile, clazz: ParsedClass): AnalyzedClass|undefined {
protected analyzeClass(file: ts.SourceFile, pool: ConstantPool, clazz: ParsedClass): AnalyzedClass
|undefined {
const matchingHandlers =
this.handlers.map(handler => ({handler, decorator: handler.detect(clazz.decorators)}))
.filter(isMatchingHandler);
@ -84,7 +90,7 @@ export class Analyzer {
const {handler, decorator} = matchingHandlers[0];
const {analysis, diagnostics} = handler.analyze(clazz.declaration, decorator);
let compilation = handler.compile(clazz.declaration, analysis);
let compilation = handler.compile(clazz.declaration, analysis, pool);
if (!Array.isArray(compilation)) {
compilation = [compilation];
}

View File

@ -22,6 +22,20 @@ export class Esm2015Renderer extends Renderer {
imports.forEach(i => { output.appendLeft(0, `import * as ${i.as} from '${i.name}';\n`); });
}
addConstants(output: MagicString, constants: string, file: ts.SourceFile): void {
if (constants === '') {
return;
}
const insertionPoint = file.statements.reduce((prev, stmt) => {
if (ts.isImportDeclaration(stmt) || ts.isImportEqualsDeclaration(stmt) ||
ts.isNamespaceImport(stmt)) {
return stmt.getEnd();
}
return prev;
}, 0);
output.appendLeft(insertionPoint, '\n' + constants + '\n');
}
/**
* Add the definitions to each decorated class
*/

View File

@ -11,7 +11,7 @@ import * as ts from 'typescript';
import MagicString from 'magic-string';
import {commentRegex, mapFileCommentRegex, fromJSON, fromSource, fromMapFileSource, fromObject, generateMapFileComment, removeComments, removeMapFileComments, SourceMapConverter} from 'convert-source-map';
import {SourceMapConsumer, SourceMapGenerator, RawSourceMap} from 'source-map';
import {Expression, Statement, WrappedNodeExpr, WritePropExpr} from '@angular/compiler';
import {ConstantPool, Expression, Statement, WrappedNodeExpr, WritePropExpr} from '@angular/compiler';
import {AnalyzedClass, AnalyzedFile} from '../analyzer';
import {Decorator} from '../../../ngtsc/host';
import {ImportManager, translateStatement} from '../../../ngtsc/transform';
@ -79,6 +79,10 @@ export abstract class Renderer {
this.trackDecorators(clazz.decorators, decoratorsToRemove);
});
this.addConstants(
outputText, renderConstantPool(file.sourceFile, file.constantPool, importManager),
file.sourceFile);
this.addImports(outputText, importManager.getAllImports(file.sourceFile.fileName, null));
// QUESTION: do we need to remove contructor param metadata and property decorators?
this.removeDecorators(outputText, decoratorsToRemove);
@ -86,6 +90,8 @@ export abstract class Renderer {
return this.renderSourceAndMap(file, input, outputText, targetPath);
}
protected abstract addConstants(output: MagicString, constants: string, file: ts.SourceFile):
void;
protected abstract addImports(output: MagicString, imports: {name: string, as: string}[]): void;
protected abstract addDefinitions(
output: MagicString, analyzedClass: AnalyzedClass, definitions: string): void;
@ -207,6 +213,17 @@ export function mergeSourceMaps(
return merged;
}
/**
* Render the constant pool as source code for the given class.
*/
export function renderConstantPool(
sourceFile: ts.SourceFile, constantPool: ConstantPool, imports: ImportManager): string {
const printer = ts.createPrinter();
return constantPool.statements.map(stmt => translateStatement(stmt, imports))
.map(stmt => printer.printNode(ts.EmitHint.Unspecified, stmt, sourceFile))
.join('\n');
}
/**
* Render the definitions as source code for the given class.
* @param sourceFile The file containing the class to process.

View File

@ -56,6 +56,36 @@ A.decorators = [
});
describe('addConstants', () => {
it('should insert the given constants after imports in the source file', () => {
const PROGRAM = {
name: 'some/file.js',
contents: `
/* A copyright notice */
import {Directive} from '@angular/core';
export class A {}
A.decorators = [
{ type: Directive, args: [{ selector: '[a]' }] },
{ type: Other }
];
// Some other content`
};
const {renderer, program} = setup(PROGRAM);
const file = program.getSourceFile('some/file.js');
if (file === undefined) {
throw new Error(`Could not find source file`);
}
const output = new MagicString(PROGRAM.contents);
renderer.addConstants(output, 'const x = 3;', file);
expect(output.toString()).toContain(`
import {Directive} from '@angular/core';
const x = 3;
export class A {}`);
});
});
describe('addDefinitions', () => {
it('should insert the definitions directly after the class declaration', () => {
const PROGRAM = {

View File

@ -20,6 +20,9 @@ class TestRenderer extends Renderer {
addImports(output: MagicString, imports: {name: string, as: string}[]) {
output.prepend('\n// ADD IMPORTS\n');
}
addConstants(output: MagicString, constants: string, file: ts.SourceFile): void {
output.prepend('\n// ADD CONSTANTS\n');
}
addDefinitions(output: MagicString, analyzedClass: AnalyzedClass, definitions: string) {
output.prepend('\n// ADD DEFINITIONS\n');
}
@ -65,7 +68,8 @@ describe('Renderer', () => {
]
});
const RENDERED_CONTENTS =
`\n// REMOVE DECORATORS\n\n// ADD IMPORTS\n\n// ADD DEFINITIONS\n` + INPUT_PROGRAM.contents;
`\n// REMOVE DECORATORS\n\n// ADD IMPORTS\n\n// ADD CONSTANTS\n\n// ADD DEFINITIONS\n` +
INPUT_PROGRAM.contents;
const OUTPUT_PROGRAM_MAP = fromObject({
'version': 3,
'file': '/output_file.js',
@ -74,14 +78,14 @@ describe('Renderer', () => {
'import { Directive } from \'@angular/core\';\nexport class A {\n foo(x) {\n return x;\n }\n}\nA.decorators = [\n { type: Directive, args: [{ selector: \'[a]\' }] }\n];\n'
],
'names': [],
'mappings': ';;;;;;AAAA;;;;;;;;;'
'mappings': ';;;;;;;;AAAA;;;;;;;;;'
});
const MERGED_OUTPUT_PROGRAM_MAP = fromObject({
'version': 3,
'sources': ['/file.ts'],
'names': [],
'mappings': ';;;;;;AAAA',
'mappings': ';;;;;;;;AAAA',
'file': '/output_file.js',
'sourcesContent': [
'import { Directive } from \'@angular/core\';\nexport class A {\n foo(x: string): string {\n return x;\n }\n static decorators = [\n { type: Directive, args: [{ selector: \'[a]\' }] }\n ];\n}'