From 9c1f6fd06f8e15e243ac7fa66ec376caae6b22c1 Mon Sep 17 00:00:00 2001 From: Victor Berchet Date: Wed, 5 Jul 2017 14:51:39 -0700 Subject: [PATCH] fix(compiler): emits quoted keys only iff they are quoted in the original template fixes #14292 --- .../test/transformers/node_emitter_spec.ts | 16 +++++------ .../compiler/src/aot/summary_serializer.ts | 2 +- .../src/compiler_util/expression_converter.ts | 17 +++++++---- packages/compiler/src/output/output_ast.ts | 10 +++---- .../compiler/src/output/output_interpreter.ts | 5 ++-- packages/compiler/src/output/output_jit.ts | 2 +- .../src/view_compiler/view_compiler.ts | 28 +++++++++++-------- .../compiler/test/output/js_emitter_spec.ts | 8 +++++- .../compiler/test/output/ts_emitter_spec.ts | 16 +++++------ packages/core/src/view/pure_expression.ts | 11 +++++++- .../core/test/view/pure_expression_spec.ts | 17 +++++------ 11 files changed, 77 insertions(+), 55 deletions(-) diff --git a/packages/compiler-cli/test/transformers/node_emitter_spec.ts b/packages/compiler-cli/test/transformers/node_emitter_spec.ts index 74da22f6eb..2918e70535 100644 --- a/packages/compiler-cli/test/transformers/node_emitter_spec.ts +++ b/packages/compiler-cli/test/transformers/node_emitter_spec.ts @@ -21,7 +21,7 @@ const sameModuleIdentifier = new o.ExternalReference(null, 'someLocalId', null); const externalModuleIdentifier = new o.ExternalReference(anotherModuleUrl, 'someExternalId', null); -describe('TypeScriptEmitter', () => { +describe('TypeScriptNodeEmitter', () => { let context: MockAotContext; let host: MockCompilerHost; let emitter: TypeScriptNodeEmitter; @@ -166,15 +166,13 @@ describe('TypeScriptEmitter', () => { expect(emitStmt(o.literal(true).toStmt())).toEqual('true;'); expect(emitStmt(o.literal('someStr').toStmt())).toEqual(`"someStr";`); expect(emitStmt(o.literalArr([o.literal(1)]).toStmt())).toEqual(`[1];`); - expect(emitStmt(o.literalMap([['someKey', o.literal(1)]]).toStmt())) - .toEqual(`({ someKey: 1 });`); - }); - - it('should apply quotes to each entry within a map produced with literalMap when true', () => { - expect(emitStmt( - o.literalMap([['a', o.literal('a')], ['*', o.literal('star')]], null, true).toStmt()) + expect(emitStmt(o.literalMap([ + {key: 'someKey', value: o.literal(1), quoted: false}, + {key: 'a', value: o.literal('a'), quoted: false}, + {key: '*', value: o.literal('star'), quoted: true}, + ]).toStmt()) .replace(/\s+/gm, '')) - .toEqual(`({"a":"a","*":"star"});`); + .toEqual(`({someKey:1,a:"a","*":"star"});`); }); it('should support blank literals', () => { diff --git a/packages/compiler/src/aot/summary_serializer.ts b/packages/compiler/src/aot/summary_serializer.ts index 209fee2e9b..8931299a5c 100644 --- a/packages/compiler/src/aot/summary_serializer.ts +++ b/packages/compiler/src/aot/summary_serializer.ts @@ -298,7 +298,7 @@ class ForJitSerializer { } visitStringMap(map: {[key: string]: any}, context: any): any { return new o.LiteralMapExpr(Object.keys(map).map( - (key) => new o.LiteralMapEntry(key, visitValue(map[key], this, context)))); + (key) => new o.LiteralMapEntry(key, visitValue(map[key], this, context), false))); } visitPrimitive(value: any, context: any): any { return o.literal(value); } visitOther(value: any, context: any): any { diff --git a/packages/compiler/src/compiler_util/expression_converter.ts b/packages/compiler/src/compiler_util/expression_converter.ts index d61cea3037..a5abc8aa36 100644 --- a/packages/compiler/src/compiler_util/expression_converter.ts +++ b/packages/compiler/src/compiler_util/expression_converter.ts @@ -35,10 +35,16 @@ export function convertActionBinding( // Note: no caching for literal arrays in actions. return (args: o.Expression[]) => o.literalArr(args); }, - createLiteralMapConverter: (keys: string[]) => { + createLiteralMapConverter: (keys: {key: string, quoted: boolean}[]) => { // Note: no caching for literal maps in actions. - return (args: o.Expression[]) => - o.literalMap(<[string, o.Expression][]>keys.map((key, i) => [key, args[i]])); + return (values: o.Expression[]) => { + const entries = keys.map((k, i) => ({ + key: k.key, + value: values[i], + quoted: k.quoted, + })); + return o.literalMap(entries); + }; }, createPipeConverter: (name: string) => { throw new Error(`Illegal State: Actions are not allowed to contain pipes. Pipe: ${name}`); @@ -71,7 +77,7 @@ export interface BuiltinConverter { (args: o.Expression[]): o.Expression; } export interface BuiltinConverterFactory { createLiteralArrayConverter(argCount: number): BuiltinConverter; - createLiteralMapConverter(keys: string[]): BuiltinConverter; + createLiteralMapConverter(keys: {key: string, quoted: boolean}[]): BuiltinConverter; createPipeConverter(name: string, argCount: number): BuiltinConverter; } @@ -169,8 +175,9 @@ class _BuiltinAstConverter extends cdAst.AstTransformer { } visitLiteralMap(ast: cdAst.LiteralMap, context: any): any { const args = ast.values.map(ast => ast.visit(this, context)); + return new BuiltinFunctionCall( - ast.span, args, this._converterFactory.createLiteralMapConverter(ast.keys.map(k => k.key))); + ast.span, args, this._converterFactory.createLiteralMapConverter(ast.keys)); } } diff --git a/packages/compiler/src/output/output_ast.ts b/packages/compiler/src/output/output_ast.ts index 05a375889d..8cd09a93e1 100644 --- a/packages/compiler/src/output/output_ast.ts +++ b/packages/compiler/src/output/output_ast.ts @@ -476,7 +476,7 @@ export class LiteralArrayExpr extends Expression { } export class LiteralMapEntry { - constructor(public key: string, public value: Expression, public quoted: boolean = false) {} + constructor(public key: string, public value: Expression, public quoted: boolean) {} } export class LiteralMapExpr extends Expression { @@ -831,7 +831,7 @@ export class AstTransformer implements StatementVisitor, ExpressionVisitor { visitLiteralMapExpr(ast: LiteralMapExpr, context: any): any { const entries = ast.entries.map( (entry): LiteralMapEntry => new LiteralMapEntry( - entry.key, entry.value.visitExpression(this, context), entry.quoted, )); + entry.key, entry.value.visitExpression(this, context), entry.quoted)); const mapType = new MapType(ast.valueType, null); return this.transformExpr(new LiteralMapExpr(entries, mapType, ast.sourceSpan), context); } @@ -1151,10 +1151,10 @@ export function literalArr( } export function literalMap( - values: [string, Expression][], type: MapType | null = null, - quoted: boolean = false): LiteralMapExpr { + values: {key: string, quoted: boolean, value: Expression}[], + type: MapType | null = null): LiteralMapExpr { return new LiteralMapExpr( - values.map(entry => new LiteralMapEntry(entry[0], entry[1], quoted)), type, null); + values.map(e => new LiteralMapEntry(e.key, e.value, e.quoted)), type, null); } export function not(expr: Expression, sourceSpan?: ParseSourceSpan | null): NotExpr { diff --git a/packages/compiler/src/output/output_interpreter.ts b/packages/compiler/src/output/output_interpreter.ts index a87661f462..4339c78062 100644 --- a/packages/compiler/src/output/output_interpreter.ts +++ b/packages/compiler/src/output/output_interpreter.ts @@ -311,9 +311,8 @@ class StatementInterpreter implements o.StatementVisitor, o.ExpressionVisitor { return this.visitAllExpressions(ast.entries, ctx); } visitLiteralMapExpr(ast: o.LiteralMapExpr, ctx: _ExecutionContext): any { - const result = {}; - ast.entries.forEach( - (entry) => (result as any)[entry.key] = entry.value.visitExpression(this, ctx)); + const result: {[k: string]: any} = {}; + ast.entries.forEach(entry => result[entry.key] = entry.value.visitExpression(this, ctx)); return result; } visitCommaExpr(ast: o.CommaExpr, context: any): any { diff --git a/packages/compiler/src/output/output_jit.ts b/packages/compiler/src/output/output_jit.ts index b42b81c119..686e149ca6 100644 --- a/packages/compiler/src/output/output_jit.ts +++ b/packages/compiler/src/output/output_jit.ts @@ -50,7 +50,7 @@ class JitEmitterVisitor extends AbstractJsEmitterVisitor { createReturnStmt(ctx: EmitterVisitorContext) { const stmt = new o.ReturnStatement(new o.LiteralMapExpr(this._evalExportedVars.map( - resultVar => new o.LiteralMapEntry(resultVar, o.variable(resultVar))))); + resultVar => new o.LiteralMapEntry(resultVar, o.variable(resultVar), false)))); stmt.visitStatement(this, ctx); } diff --git a/packages/compiler/src/view_compiler/view_compiler.ts b/packages/compiler/src/view_compiler/view_compiler.ts index c95985e014..6398c06199 100644 --- a/packages/compiler/src/view_compiler/view_compiler.ts +++ b/packages/compiler/src/view_compiler/view_compiler.ts @@ -61,9 +61,9 @@ export class ViewCompiler { outputCtx.statements.push( renderComponentVar .set(o.importExpr(Identifiers.createRendererType2).callFn([new o.LiteralMapExpr([ - new o.LiteralMapEntry('encapsulation', o.literal(template.encapsulation)), - new o.LiteralMapEntry('styles', styles), - new o.LiteralMapEntry('data', new o.LiteralMapExpr(customRenderData)) + new o.LiteralMapEntry('encapsulation', o.literal(template.encapsulation), false), + new o.LiteralMapEntry('styles', styles, false), + new o.LiteralMapEntry('data', new o.LiteralMapExpr(customRenderData), false) ])])) .toDeclStmt( o.importType(Identifiers.RendererType2), @@ -160,8 +160,8 @@ class ViewBuilder implements TemplateAstVisitor, LocalResolver { nodeFlags: flags, nodeDef: o.importExpr(Identifiers.queryDef).callFn([ o.literal(flags), o.literal(queryId), - new o.LiteralMapExpr( - [new o.LiteralMapEntry(query.propertyName, o.literal(bindingType))]) + new o.LiteralMapExpr([new o.LiteralMapEntry( + query.propertyName, o.literal(bindingType), false)]) ]) })); }); @@ -504,8 +504,8 @@ class ViewBuilder implements TemplateAstVisitor, LocalResolver { nodeFlags: flags, nodeDef: o.importExpr(Identifiers.queryDef).callFn([ o.literal(flags), o.literal(queryId), - new o.LiteralMapExpr( - [new o.LiteralMapEntry(query.propertyName, o.literal(bindingType))]) + new o.LiteralMapExpr([new o.LiteralMapEntry( + query.propertyName, o.literal(bindingType), false)]) ]), })); }); @@ -705,23 +705,26 @@ class ViewBuilder implements TemplateAstVisitor, LocalResolver { return (args: o.Expression[]) => callCheckStmt(nodeIndex, args); } - createLiteralMapConverter(sourceSpan: ParseSourceSpan, keys: string[]): BuiltinConverter { + + createLiteralMapConverter(sourceSpan: ParseSourceSpan, keys: {key: string, quoted: boolean}[]): + BuiltinConverter { if (keys.length === 0) { const valueExpr = o.importExpr(Identifiers.EMPTY_MAP); return () => valueExpr; } + // function pureObjectDef(propToIndex: {[p: string]: number}): NodeDef + const map = o.literalMap(keys.map((e, i) => ({...e, value: o.literal(i)}))); const nodeIndex = this.nodes.length; - // function pureObjectDef(propertyNames: string[]): NodeDef this.nodes.push(() => ({ sourceSpan, nodeFlags: NodeFlags.TypePureObject, - nodeDef: o.importExpr(Identifiers.pureObjectDef).callFn([o.literalArr( - keys.map(key => o.literal(key)))]) + nodeDef: o.importExpr(Identifiers.pureObjectDef).callFn([map]) })); return (args: o.Expression[]) => callCheckStmt(nodeIndex, args); } + createPipeConverter(expression: UpdateExpression, name: string, argCount: number): BuiltinConverter { const pipe = this.usedPipes.find((pipeSummary) => pipeSummary.name === name) !; @@ -795,7 +798,8 @@ class ViewBuilder implements TemplateAstVisitor, LocalResolver { createLiteralArrayConverter: (argCount: number) => this.createLiteralArrayConverter( expression.sourceSpan, argCount), createLiteralMapConverter: - (keys: string[]) => this.createLiteralMapConverter(expression.sourceSpan, keys), + (keys: {key: string, quoted: boolean}[]) => + this.createLiteralMapConverter(expression.sourceSpan, keys), createPipeConverter: (name: string, argCount: number) => this.createPipeConverter(expression, name, argCount) }, diff --git a/packages/compiler/test/output/js_emitter_spec.ts b/packages/compiler/test/output/js_emitter_spec.ts index ff6e803798..e83b328315 100644 --- a/packages/compiler/test/output/js_emitter_spec.ts +++ b/packages/compiler/test/output/js_emitter_spec.ts @@ -99,7 +99,13 @@ export function main() { expect(emitStmt(o.literal(true).toStmt())).toEqual('true;'); expect(emitStmt(o.literal('someStr').toStmt())).toEqual(`'someStr';`); expect(emitStmt(o.literalArr([o.literal(1)]).toStmt())).toEqual(`[1];`); - expect(emitStmt(o.literalMap([['someKey', o.literal(1)]]).toStmt())).toEqual(`{someKey:1};`); + expect(emitStmt(o.literalMap([ + {key: 'someKey', value: o.literal(1), quoted: false}, + {key: 'a', value: o.literal('a'), quoted: false}, + {key: '*', value: o.literal('star'), quoted: true}, + ]).toStmt()) + .replace(/\s+/gm, '')) + .toEqual(`{someKey:1,a:'a','*':'star'};`); }); it('should break expressions into multiple lines if they are too long', () => { diff --git a/packages/compiler/test/output/ts_emitter_spec.ts b/packages/compiler/test/output/ts_emitter_spec.ts index 910654069a..b472ac6c12 100644 --- a/packages/compiler/test/output/ts_emitter_spec.ts +++ b/packages/compiler/test/output/ts_emitter_spec.ts @@ -151,7 +151,13 @@ export function main() { expect(emitStmt(o.literal(true).toStmt())).toEqual('true;'); expect(emitStmt(o.literal('someStr').toStmt())).toEqual(`'someStr';`); expect(emitStmt(o.literalArr([o.literal(1)]).toStmt())).toEqual(`[1];`); - expect(emitStmt(o.literalMap([['someKey', o.literal(1)]]).toStmt())).toEqual(`{someKey:1};`); + expect(emitStmt(o.literalMap([ + {key: 'someKey', value: o.literal(1), quoted: false}, + {key: 'a', value: o.literal('a'), quoted: false}, + {key: '*', value: o.literal('star'), quoted: true}, + ]).toStmt()) + .replace(/\s+/gm, '')) + .toEqual(`{someKey:1,a:'a','*':'star'};`); }); it('should break expressions into multiple lines if they are too long', () => { @@ -166,14 +172,6 @@ export function main() { ].join('\n')); }); - it('should apply quotes to each entry within a map produced with literalMap when true', () => { - expect( - emitStmt( - o.literalMap([['a', o.literal('a')], ['*', o.literal('star')]], null, true).toStmt()) - .replace(/\s+/gm, '')) - .toEqual(`{'a':'a','*':'star'};`); - }); - it('should support blank literals', () => { expect(emitStmt(o.literal(null).toStmt())).toEqual('(null as any);'); expect(emitStmt(o.literal(undefined).toStmt())).toEqual('(undefined as any);'); diff --git a/packages/core/src/view/pure_expression.ts b/packages/core/src/view/pure_expression.ts index 697f3a6bcc..d8ab3c87f3 100644 --- a/packages/core/src/view/pure_expression.ts +++ b/packages/core/src/view/pure_expression.ts @@ -18,7 +18,16 @@ export function pureArrayDef(argCount: number): NodeDef { return _pureExpressionDef(NodeFlags.TypePureArray, new Array(argCount)); } -export function pureObjectDef(propertyNames: string[]): NodeDef { +export function pureObjectDef(propToIndex: {[p: string]: number}): NodeDef { + const keys = Object.keys(propToIndex); + const nbKeys = keys.length; + const propertyNames = new Array(nbKeys); + for (let i = 0; i < nbKeys; i++) { + const key = keys[i]; + const index = propToIndex[key]; + propertyNames[index] = key; + } + return _pureExpressionDef(NodeFlags.TypePureObject, propertyNames); } diff --git a/packages/core/test/view/pure_expression_spec.ts b/packages/core/test/view/pure_expression_spec.ts index ef22ee27d6..997a91c3cc 100644 --- a/packages/core/test/view/pure_expression_spec.ts +++ b/packages/core/test/view/pure_expression_spec.ts @@ -6,9 +6,8 @@ * found in the LICENSE file at https://angular.io/license */ -import {Injector, PipeTransform, RenderComponentType, RootRenderer, Sanitizer, SecurityContext, ViewEncapsulation, WrappedValue} from '@angular/core'; -import {ArgumentType, NodeDef, NodeFlags, RootData, Services, ViewData, ViewDefinition, ViewFlags, ViewHandleEventFn, ViewUpdateFn, anchorDef, asProviderData, asPureExpressionData, directiveDef, elementDef, nodeValue, pipeDef, pureArrayDef, pureObjectDef, purePipeDef, rootRenderNodes, textDef, viewDef} from '@angular/core/src/view/index'; -import {inject} from '@angular/core/testing'; +import {PipeTransform} from '@angular/core'; +import {NodeDef, NodeFlags, Services, ViewData, ViewDefinition, ViewFlags, ViewUpdateFn, asProviderData, directiveDef, elementDef, nodeValue, pipeDef, pureArrayDef, pureObjectDef, purePipeDef, rootRenderNodes, viewDef} from '@angular/core/src/view/index'; import {ARG_TYPE_VALUES, checkNodeInlineOrDynamic, createRootView} from './helper'; @@ -38,8 +37,9 @@ export function main() { const {view, rootNodes} = createAndGetRootNodes(compViewDef( [ - elementDef(NodeFlags.None, null !, null !, 2, 'span'), pureArrayDef(2), - directiveDef(NodeFlags.None, null !, 0, Service, [], {data: [0, 'data']}) + elementDef(NodeFlags.None, null !, null !, 2, 'span'), + pureArrayDef(2), + directiveDef(NodeFlags.None, null !, 0, Service, [], {data: [0, 'data']}), ], (check, view) => { const pureValue = checkNodeInlineOrDynamic(check, view, 1, inlineDynamic, values); @@ -75,7 +75,7 @@ export function main() { const {view, rootNodes} = createAndGetRootNodes(compViewDef( [ - elementDef(NodeFlags.None, null !, null !, 2, 'span'), pureObjectDef(['a', 'b']), + elementDef(NodeFlags.None, null !, null !, 2, 'span'), pureObjectDef({a: 0, b: 1}), directiveDef(NodeFlags.None, null !, 0, Service, [], {data: [0, 'data']}) ], (check, view) => { @@ -116,8 +116,9 @@ export function main() { const {view, rootNodes} = createAndGetRootNodes(compViewDef( [ elementDef(NodeFlags.None, null !, null !, 3, 'span'), - pipeDef(NodeFlags.None, SomePipe, []), purePipeDef(2), - directiveDef(NodeFlags.None, null !, 0, Service, [], {data: [0, 'data']}) + pipeDef(NodeFlags.None, SomePipe, []), + purePipeDef(2), + directiveDef(NodeFlags.None, null !, 0, Service, [], {data: [0, 'data']}), ], (check, view) => { const pureValue = checkNodeInlineOrDynamic(