From cc29b9cf938ae46eea000de77a2ef421aa3af633 Mon Sep 17 00:00:00 2001 From: Alex Rickabaugh Date: Mon, 27 Aug 2018 11:11:02 -0700 Subject: [PATCH] fix(ivy): use globally unique names for i18n constants (#25689) Closure compiler requires that the i18n message constants of the form const MSG_XYZ = goog.getMessage('...'); have names that are unique across an entire compilation, even if the variables themselves are local to a given module. This means that in practice these names must be unique in a codebase. The best way to guarantee this requirement is met is to encode the relative file name of the file into which the constant is being written into the constant name itself. This commit implements that solution. PR Close #25689 --- .../compiler-cli/src/ngcc/src/analyzer.ts | 6 ++- .../ngcc/src/transform/package_transformer.ts | 10 ++++- .../src/ngcc/test/analyzer_spec.ts | 2 +- .../test/rendering/esm2015_renderer_spec.ts | 2 +- .../ngcc/test/rendering/esm5_renderer_spec.ts | 2 +- .../src/ngcc/test/rendering/renderer_spec.ts | 2 +- .../src/ngtsc/annotations/src/component.ts | 16 ++++++- .../ngtsc/annotations/test/component_spec.ts | 3 +- packages/compiler-cli/src/ngtsc/program.ts | 11 ++++- packages/compiler/src/constant_pool.ts | 9 ++-- packages/compiler/src/render3/view/api.ts | 7 ++++ .../compiler/src/render3/view/compiler.ts | 4 +- .../compiler/src/render3/view/template.ts | 42 +++++++++++++++---- packages/core/src/render3/jit/directive.ts | 7 ++-- 14 files changed, 95 insertions(+), 28 deletions(-) diff --git a/packages/compiler-cli/src/ngcc/src/analyzer.ts b/packages/compiler-cli/src/ngcc/src/analyzer.ts index f55a6e6948..295d921048 100644 --- a/packages/compiler-cli/src/ngcc/src/analyzer.ts +++ b/packages/compiler-cli/src/ngcc/src/analyzer.ts @@ -49,14 +49,16 @@ export class Analyzer { handlers: DecoratorHandler[] = [ new BaseDefDecoratorHandler(this.typeChecker, this.host), new ComponentDecoratorHandler( - this.typeChecker, this.host, this.scopeRegistry, false, this.resourceLoader), + this.typeChecker, this.host, this.scopeRegistry, false, this.resourceLoader, this.rootDirs), new DirectiveDecoratorHandler(this.typeChecker, this.host, this.scopeRegistry, false), new InjectableDecoratorHandler(this.host, false), new NgModuleDecoratorHandler(this.typeChecker, this.host, this.scopeRegistry, false), new PipeDecoratorHandler(this.typeChecker, this.host, this.scopeRegistry, false), ]; - constructor(private typeChecker: ts.TypeChecker, private host: NgccReflectionHost) {} + constructor( + private typeChecker: ts.TypeChecker, private host: NgccReflectionHost, + private rootDirs: string[]) {} /** * Analyize a parsed file to generate the information about decorated classes that diff --git a/packages/compiler-cli/src/ngcc/src/transform/package_transformer.ts b/packages/compiler-cli/src/ngcc/src/transform/package_transformer.ts index 06d3c6aabe..815d129b48 100644 --- a/packages/compiler-cli/src/ngcc/src/transform/package_transformer.ts +++ b/packages/compiler-cli/src/ngcc/src/transform/package_transformer.ts @@ -65,13 +65,21 @@ export class PackageTransformer { // Create the TS program and necessary helpers. // TODO : create a custom compiler host that reads from .bak files if available. const host = ts.createCompilerHost(options); + let rootDirs: string[]|undefined = undefined; + if (options.rootDirs !== undefined) { + rootDirs = options.rootDirs; + } else if (options.rootDir !== undefined) { + rootDirs = [options.rootDir]; + } else { + rootDirs = [host.getCurrentDirectory()]; + } const packageProgram = ts.createProgram([entryPoint.entryFileName], options, host); const typeChecker = packageProgram.getTypeChecker(); const dtsMapper = new DtsMapper(entryPoint.entryRoot, entryPoint.dtsEntryRoot); const reflectionHost = this.getHost(format, packageProgram, dtsMapper); const parser = this.getFileParser(format, packageProgram, reflectionHost); - const analyzer = new Analyzer(typeChecker, reflectionHost); + const analyzer = new Analyzer(typeChecker, reflectionHost, rootDirs); const renderer = this.getRenderer(format, packageProgram, reflectionHost); // Parse and analyze the files. diff --git a/packages/compiler-cli/src/ngcc/test/analyzer_spec.ts b/packages/compiler-cli/src/ngcc/test/analyzer_spec.ts index d33e8ea7c3..adf1f63198 100644 --- a/packages/compiler-cli/src/ngcc/test/analyzer_spec.ts +++ b/packages/compiler-cli/src/ngcc/test/analyzer_spec.ts @@ -80,7 +80,7 @@ describe('Analyzer', () => { program = makeProgram(TEST_PROGRAM); const file = createParsedFile(program); const analyzer = new Analyzer( - program.getTypeChecker(), new Fesm2015ReflectionHost(program.getTypeChecker())); + program.getTypeChecker(), new Fesm2015ReflectionHost(program.getTypeChecker()), ['']); testHandler = createTestHandler(); analyzer.handlers = [testHandler]; result = analyzer.analyzeFile(file); diff --git a/packages/compiler-cli/src/ngcc/test/rendering/esm2015_renderer_spec.ts b/packages/compiler-cli/src/ngcc/test/rendering/esm2015_renderer_spec.ts index a8ff47e7f0..398df52a94 100644 --- a/packages/compiler-cli/src/ngcc/test/rendering/esm2015_renderer_spec.ts +++ b/packages/compiler-cli/src/ngcc/test/rendering/esm2015_renderer_spec.ts @@ -17,7 +17,7 @@ function setup(file: {name: string, contents: string}) { const program = makeProgram(file); const host = new Fesm2015ReflectionHost(program.getTypeChecker()); const parser = new Esm2015FileParser(program, host); - const analyzer = new Analyzer(program.getTypeChecker(), host); + const analyzer = new Analyzer(program.getTypeChecker(), host, ['']); const renderer = new Esm2015Renderer(host); return {analyzer, host, parser, program, renderer}; } diff --git a/packages/compiler-cli/src/ngcc/test/rendering/esm5_renderer_spec.ts b/packages/compiler-cli/src/ngcc/test/rendering/esm5_renderer_spec.ts index a054be832a..7de4e0ab24 100644 --- a/packages/compiler-cli/src/ngcc/test/rendering/esm5_renderer_spec.ts +++ b/packages/compiler-cli/src/ngcc/test/rendering/esm5_renderer_spec.ts @@ -17,7 +17,7 @@ function setup(file: {name: string, contents: string}) { const program = makeProgram(file); const host = new Esm5ReflectionHost(program.getTypeChecker()); const parser = new Esm5FileParser(program, host); - const analyzer = new Analyzer(program.getTypeChecker(), host); + const analyzer = new Analyzer(program.getTypeChecker(), host, ['']); const renderer = new Esm5Renderer(host); return {analyzer, host, parser, program, renderer}; } diff --git a/packages/compiler-cli/src/ngcc/test/rendering/renderer_spec.ts b/packages/compiler-cli/src/ngcc/test/rendering/renderer_spec.ts index c58559ea64..3ea9649089 100644 --- a/packages/compiler-cli/src/ngcc/test/rendering/renderer_spec.ts +++ b/packages/compiler-cli/src/ngcc/test/rendering/renderer_spec.ts @@ -46,7 +46,7 @@ function analyze(file: {name: string, contents: string}) { const program = makeProgram(file); const host = new Fesm2015ReflectionHost(program.getTypeChecker()); const parser = new Esm2015FileParser(program, host); - const analyzer = new Analyzer(program.getTypeChecker(), host); + const analyzer = new Analyzer(program.getTypeChecker(), host, ['']); const parsedFiles = parser.parseFile(program.getSourceFile(file.name) !); return parsedFiles.map(file => analyzer.analyzeFile(file)); diff --git a/packages/compiler-cli/src/ngtsc/annotations/src/component.ts b/packages/compiler-cli/src/ngtsc/annotations/src/component.ts index a805467676..06c5daaad3 100644 --- a/packages/compiler-cli/src/ngtsc/annotations/src/component.ts +++ b/packages/compiler-cli/src/ngtsc/annotations/src/component.ts @@ -29,7 +29,7 @@ export class ComponentDecoratorHandler implements DecoratorHandler(); @@ -111,9 +111,21 @@ export class ComponentDecoratorHandler implements DecoratorHandler((previous, rootDir) => { + const candidate = path.posix.relative(rootDir, filePath); + if (previous === undefined || candidate.length < previous.length) { + return candidate; + } else { + return previous; + } + }, undefined) !; + const template = parseTemplate( templateStr, `${node.getSourceFile().fileName}#${node.name!.text}/template.html`, - {preserveWhitespaces}); + {preserveWhitespaces}, relativeFilePath); if (template.errors !== undefined) { throw new Error( `Errors parsing template: ${template.errors.map(e => e.toString()).join(', ')}`); diff --git a/packages/compiler-cli/src/ngtsc/annotations/test/component_spec.ts b/packages/compiler-cli/src/ngtsc/annotations/test/component_spec.ts index 8c235b0202..4decbfdde9 100644 --- a/packages/compiler-cli/src/ngtsc/annotations/test/component_spec.ts +++ b/packages/compiler-cli/src/ngtsc/annotations/test/component_spec.ts @@ -39,7 +39,8 @@ describe('ComponentDecoratorHandler', () => { const checker = program.getTypeChecker(); const host = new TypeScriptReflectionHost(checker); const handler = new ComponentDecoratorHandler( - checker, host, new SelectorScopeRegistry(checker, host), false, new NoopResourceLoader()); + checker, host, new SelectorScopeRegistry(checker, host), false, new NoopResourceLoader(), + ['']); const TestCmp = getDeclaration(program, 'entry.ts', 'TestCmp', ts.isClassDeclaration); const detected = handler.detect(TestCmp, host.getDecoratorsOfDeclaration(TestCmp)); if (detected === undefined) { diff --git a/packages/compiler-cli/src/ngtsc/program.ts b/packages/compiler-cli/src/ngtsc/program.ts index ecae966b34..7b73608579 100644 --- a/packages/compiler-cli/src/ngtsc/program.ts +++ b/packages/compiler-cli/src/ngtsc/program.ts @@ -29,11 +29,20 @@ export class NgtscProgram implements api.Program { private _coreImportsFrom: ts.SourceFile|null|undefined = undefined; private _reflector: TypeScriptReflectionHost|undefined = undefined; private _isCore: boolean|undefined = undefined; + private rootDirs: string[]; constructor( rootNames: ReadonlyArray, private options: api.CompilerOptions, host: api.CompilerHost, oldProgram?: api.Program) { + this.rootDirs = []; + if (options.rootDirs !== undefined) { + this.rootDirs.push(...options.rootDirs); + } else if (options.rootDir !== undefined) { + this.rootDirs.push(options.rootDir); + } else { + this.rootDirs.push(host.getCurrentDirectory()); + } this.resourceLoader = host.readResource !== undefined ? new HostResourceLoader(host.readResource.bind(host)) : new FileResourceLoader(); @@ -177,7 +186,7 @@ export class NgtscProgram implements api.Program { const handlers = [ new BaseDefDecoratorHandler(checker, this.reflector), new ComponentDecoratorHandler( - checker, this.reflector, scopeRegistry, this.isCore, this.resourceLoader), + checker, this.reflector, scopeRegistry, this.isCore, this.resourceLoader, this.rootDirs), new DirectiveDecoratorHandler(checker, this.reflector, scopeRegistry, this.isCore), new InjectableDecoratorHandler(this.reflector, this.isCore), new NgModuleDecoratorHandler(checker, this.reflector, scopeRegistry, this.isCore), diff --git a/packages/compiler/src/constant_pool.ts b/packages/compiler/src/constant_pool.ts index aa95cd0a8a..33575cc4cf 100644 --- a/packages/compiler/src/constant_pool.ts +++ b/packages/compiler/src/constant_pool.ts @@ -122,7 +122,8 @@ export class ConstantPool { // */ // const MSG_XYZ = goog.getMsg('message'); // ``` - getTranslation(message: string, meta: {description?: string, meaning?: string}): o.Expression { + getTranslation(message: string, meta: {description?: string, meaning?: string}, suffix: string): + o.Expression { // The identity of an i18n message depends on the message and its meaning const key = meta.meaning ? `${message}\u0000\u0000${meta.meaning}` : message; @@ -138,7 +139,7 @@ export class ConstantPool { } // Call closure to get the translation - const variable = o.variable(this.freshTranslationName()); + const variable = o.variable(this.freshTranslationName(suffix)); const fnCall = o.variable(GOOG_GET_MSG).callFn([o.literal(message)]); const msgStmt = variable.set(fnCall).toDeclStmt(o.INFERRED_TYPE, [o.StmtModifier.Final]); this.statements.push(msgStmt); @@ -257,8 +258,8 @@ export class ConstantPool { private freshName(): string { return this.uniqueName(CONSTANT_PREFIX); } - private freshTranslationName(): string { - return this.uniqueName(TRANSLATION_PREFIX).toUpperCase(); + private freshTranslationName(suffix: string): string { + return this.uniqueName(TRANSLATION_PREFIX + suffix).toUpperCase(); } private keyOf(expression: o.Expression) { diff --git a/packages/compiler/src/render3/view/api.ts b/packages/compiler/src/render3/view/api.ts index a91b3a2246..d1177d65a8 100644 --- a/packages/compiler/src/render3/view/api.ts +++ b/packages/compiler/src/render3/view/api.ts @@ -127,6 +127,13 @@ export interface R3ComponentMetadata extends R3DirectiveMetadata { * Selectors found in the tags in the template. */ ngContentSelectors: string[]; + + /** + * Path to the .ts file in which this template's generated code will be included, relative to + * the compilation root. This will be used to generate identifiers that need to be globally + * unique in certain contexts (such as g3). + */ + relativeContextFilePath: string; }; /** diff --git a/packages/compiler/src/render3/view/compiler.ts b/packages/compiler/src/render3/view/compiler.ts index ff1db8bf4a..250d46497b 100644 --- a/packages/compiler/src/render3/view/compiler.ts +++ b/packages/compiler/src/render3/view/compiler.ts @@ -209,7 +209,8 @@ export function compileComponentFromMetadata( const template = meta.template; const templateBuilder = new TemplateDefinitionBuilder( constantPool, BindingScope.ROOT_SCOPE, 0, templateTypeName, templateName, meta.viewQueries, - directiveMatcher, directivesUsed, meta.pipes, pipesUsed, R3.namespaceHTML); + directiveMatcher, directivesUsed, meta.pipes, pipesUsed, R3.namespaceHTML, + meta.template.relativeContextFilePath); const templateFunctionExpression = templateBuilder.buildTemplateFunction( template.nodes, [], template.hasNgContent, template.ngContentSelectors); @@ -309,6 +310,7 @@ export function compileComponentFromRender2( nodes: render3Ast.nodes, hasNgContent: render3Ast.hasNgContent, ngContentSelectors: render3Ast.ngContentSelectors, + relativeContextFilePath: '', }, directives: typeMapToExpressionMap(directiveTypeBySel, outputCtx), pipes: typeMapToExpressionMap(pipeTypeByName, outputCtx), diff --git a/packages/compiler/src/render3/view/template.ts b/packages/compiler/src/render3/view/template.ts index 03e79d28aa..1ba335163e 100644 --- a/packages/compiler/src/render3/view/template.ts +++ b/packages/compiler/src/render3/view/template.ts @@ -96,18 +96,25 @@ export class TemplateDefinitionBuilder implements t.Visitor, LocalResolver // Number of binding slots private _bindingSlots = 0; + private fileBasedI18nSuffix: string; + constructor( private constantPool: ConstantPool, parentBindingScope: BindingScope, private level = 0, private contextName: string|null, private templateName: string|null, private viewQueries: R3QueryMetadata[], private directiveMatcher: SelectorMatcher|null, private directives: Set, private pipeTypeByName: Map, - private pipes: Set, private _namespace: o.ExternalReference) { + private pipes: Set, private _namespace: o.ExternalReference, + private relativeContextFilePath: string) { // view queries can take up space in data and allocation happens earlier (in the "viewQuery" // function) this._dataIndex = viewQueries.length; this._bindingScope = parentBindingScope.nestedScope(level); + // Turn the relative context file path into an identifier by replacing non-alphanumeric + // characters with underscores. + this.fileBasedI18nSuffix = relativeContextFilePath.replace(/[^A-Za-z0-9]/g, '_') + '_'; + this._valueConverter = new ValueConverter( constantPool, () => this.allocateDataSlot(), (numSlots: number) => this.allocatePureFunctionSlots(numSlots), @@ -385,7 +392,7 @@ export class TemplateDefinitionBuilder implements t.Visitor, LocalResolver attributes.push(o.literal(name)); if (attrI18nMetas.hasOwnProperty(name)) { const meta = parseI18nMeta(attrI18nMetas[name]); - const variable = this.constantPool.getTranslation(value, meta); + const variable = this.constantPool.getTranslation(value, meta, this.fileBasedI18nSuffix); attributes.push(variable); } else { attributes.push(o.literal(value)); @@ -698,7 +705,8 @@ export class TemplateDefinitionBuilder implements t.Visitor, LocalResolver // Create the template function const templateVisitor = new TemplateDefinitionBuilder( this.constantPool, this._bindingScope, this.level + 1, contextName, templateName, [], - this.directiveMatcher, this.directives, this.pipeTypeByName, this.pipes, this._namespace); + this.directiveMatcher, this.directives, this.pipeTypeByName, this.pipes, this._namespace, + this.fileBasedI18nSuffix); // Nested templates must not be visited until after their parent templates have completed // processing, so they are queued here until after the initial pass. Otherwise, we wouldn't @@ -764,7 +772,7 @@ export class TemplateDefinitionBuilder implements t.Visitor, LocalResolver // ``` visitSingleI18nTextChild(text: t.Text, i18nMeta: string) { const meta = parseI18nMeta(i18nMeta); - const variable = this.constantPool.getTranslation(text.value, meta); + const variable = this.constantPool.getTranslation(text.value, meta, this.fileBasedI18nSuffix); this.creationInstruction( text.sourceSpan, R3.text, [o.literal(this.allocateDataSlot()), variable]); } @@ -1336,14 +1344,25 @@ function interpolate(args: o.Expression[]): o.Expression { * @param templateUrl URL to use for source mapping of the parsed template */ export function parseTemplate( - template: string, templateUrl: string, options: {preserveWhitespaces?: boolean} = {}): - {errors?: ParseError[], nodes: t.Node[], hasNgContent: boolean, ngContentSelectors: string[]} { + template: string, templateUrl: string, options: {preserveWhitespaces?: boolean} = {}, + relativeContextFilePath: string): { + errors?: ParseError[], + nodes: t.Node[], + hasNgContent: boolean, + ngContentSelectors: string[], + relativeContextFilePath: string +} { const bindingParser = makeBindingParser(); const htmlParser = new HtmlParser(); const parseResult = htmlParser.parse(template, templateUrl); if (parseResult.errors && parseResult.errors.length > 0) { - return {errors: parseResult.errors, nodes: [], hasNgContent: false, ngContentSelectors: []}; + return { + errors: parseResult.errors, + nodes: [], + hasNgContent: false, + ngContentSelectors: [], relativeContextFilePath + }; } let rootNodes: html.Node[] = parseResult.rootNodes; @@ -1354,10 +1373,15 @@ export function parseTemplate( const {nodes, hasNgContent, ngContentSelectors, errors} = htmlAstToRender3Ast(rootNodes, bindingParser); if (errors && errors.length > 0) { - return {errors, nodes: [], hasNgContent: false, ngContentSelectors: []}; + return { + errors, + nodes: [], + hasNgContent: false, + ngContentSelectors: [], relativeContextFilePath + }; } - return {nodes, hasNgContent, ngContentSelectors}; + return {nodes, hasNgContent, ngContentSelectors, relativeContextFilePath}; } /** diff --git a/packages/core/src/render3/jit/directive.ts b/packages/core/src/render3/jit/directive.ts index cd7c5e35b5..712a8aadcb 100644 --- a/packages/core/src/render3/jit/directive.ts +++ b/packages/core/src/render3/jit/directive.ts @@ -54,10 +54,11 @@ export function compileComponent(type: Type, metadata: Component): void { const constantPool = new ConstantPool(); // Parse the template and check for errors. - const template = - parseTemplate(metadata.template !, `ng://${stringify(type)}/template.html`, { + const template = parseTemplate( + metadata.template !, `ng://${stringify(type)}/template.html`, { preserveWhitespaces: metadata.preserveWhitespaces || false, - }); + }, + ''); if (template.errors !== undefined) { const errors = template.errors.map(err => err.toString()).join(', '); throw new Error(