From 85d3b591b6f9782e6ccfc8afacac570db9f47ec5 Mon Sep 17 00:00:00 2001 From: Kara Erickson Date: Fri, 30 Mar 2018 16:07:37 -0700 Subject: [PATCH] refactor(ivy): generate pipe names instead of defs (#23104) PR Close #23104 --- .../compiler/src/render3/r3_pipe_compiler.ts | 5 +- .../compiler/src/render3/r3_view_compiler.ts | 50 +++++++++++++------ .../render3/r3_compiler_compliance_spec.ts | 23 ++++++--- packages/core/src/render3/pipe.ts | 7 ++- .../render3/compiler_canonical/pipes_spec.ts | 7 ++- packages/core/test/render3/pipe_spec.ts | 43 ---------------- 6 files changed, 59 insertions(+), 76 deletions(-) diff --git a/packages/compiler/src/render3/r3_pipe_compiler.ts b/packages/compiler/src/render3/r3_pipe_compiler.ts index 60b7b93565..6bf26f1563 100644 --- a/packages/compiler/src/render3/r3_pipe_compiler.ts +++ b/packages/compiler/src/render3/r3_pipe_compiler.ts @@ -24,7 +24,10 @@ export function compilePipe( mode: OutputMode) { const definitionMapValues: {key: string, quoted: boolean, value: o.Expression}[] = []; - // e.g. 'type: MyPipe` + // e.g. `name: 'myPipe'` + definitionMapValues.push({key: 'name', value: o.literal(pipe.name), quoted: false}); + + // e.g. `type: MyPipe` definitionMapValues.push( {key: 'type', value: outputCtx.importExpr(pipe.type.reference), quoted: false}); diff --git a/packages/compiler/src/render3/r3_view_compiler.ts b/packages/compiler/src/render3/r3_view_compiler.ts index f5ed8f8455..cf9f6d5fad 100644 --- a/packages/compiler/src/render3/r3_view_compiler.ts +++ b/packages/compiler/src/render3/r3_view_compiler.ts @@ -6,7 +6,7 @@ * found in the LICENSE file at https://angular.io/license */ -import {CompileDirectiveMetadata, CompileDirectiveSummary, CompilePipeSummary, CompileQueryMetadata, CompileTokenMetadata, CompileTypeMetadata, flatten, identifierName, rendererTypeName, sanitizeIdentifier, tokenReference, viewClassName} from '../compile_metadata'; +import {CompileDirectiveMetadata, CompileDirectiveSummary, CompilePipeSummary, CompileQueryMetadata, CompileTokenMetadata, CompileTypeMetadata, CompileTypeSummary, flatten, identifierName, rendererTypeName, sanitizeIdentifier, tokenReference, viewClassName} from '../compile_metadata'; import {CompileReflector} from '../compile_reflector'; import {BindingForm, BuiltinConverter, BuiltinFunctionCall, ConvertPropertyBindingResult, EventHandlerVars, LocalResolver, convertActionBinding, convertPropertyBinding, convertPropertyBindingBuiltins} from '../compiler_util/expression_converter'; import {ConstantPool, DefinitionKind} from '../constant_pool'; @@ -111,6 +111,14 @@ export function compileComponent( template: TemplateAst[], reflector: CompileReflector, bindingParser: BindingParser, mode: OutputMode) { const definitionMapValues: {key: string, quoted: boolean, value: o.Expression}[] = []; + // Set of pipe names for pipe exps that have already been stored in pipes[] (to avoid dupes) + const pipeSet = new Set(); + // Pipe expressions for pipes[] field in component def + const pipeExps: o.Expression[] = []; + + function addPipeDependency(summary: CompilePipeSummary): void { + addDependencyToComponent(outputCtx, summary, pipeSet, pipeExps); + } const field = (key: string, value: o.Expression | null) => { if (value) { @@ -154,11 +162,16 @@ export function compileComponent( new TemplateDefinitionBuilder( outputCtx, outputCtx.constantPool, reflector, CONTEXT_NAME, ROOT_SCOPE.nestedScope(), 0, component.template !.ngContentSelectors, templateTypeName, templateName, pipeMap, - component.viewQueries) + component.viewQueries, addPipeDependency) .buildTemplateFunction(template, []); field('template', templateFunctionExpression); + // e.g. `pipes: [MyPipe]` + if (pipeExps.length) { + field('pipes', o.literalArr(pipeExps)); + } + // e.g `inputs: {a: 'a'}` field('inputs', createInputsObject(component, outputCtx)); @@ -201,6 +214,19 @@ export function compileComponent( } } +// TODO: this should be used for addDirectiveDependency as well when Misko's PR goes in +function addDependencyToComponent( + outputCtx: OutputContext, summary: CompileTypeSummary, set: Set, + exps: o.Expression[]): void { + const importExpr = outputCtx.importExpr(summary.type.reference) as o.ExternalExpr; + const uniqueKey = importExpr.value.moduleName + ':' + importExpr.value.name; + + if (!set.has(uniqueKey)) { + set.add(uniqueKey); + exps.push(importExpr); + } +} + // TODO: Remove these when the things are fully supported function unknown(arg: o.Expression | o.Statement | TemplateAst): never { throw new Error( @@ -347,20 +373,16 @@ class TemplateDefinitionBuilder implements TemplateAstVisitor, LocalResolver { private reflector: CompileReflector, private contextParameter: string, private bindingScope: BindingScope, private level = 0, private ngContentSelectors: string[], private contextName: string|null, private templateName: string|null, - private pipes: Map, private viewQueries: CompileQueryMetadata[]) { + private pipes: Map, private viewQueries: CompileQueryMetadata[], + private addPipeDependency: (summary: CompilePipeSummary) => void) { this._valueConverter = new ValueConverter( outputCtx, () => this.allocateDataSlot(), (name, localName, slot, value) => { bindingScope.set(localName, value); const pipe = pipes.get(name) !; pipe || error(`Could not find pipe ${name}`); - const pipeDefinition = constantPool.getDefinition( - pipe.type.reference, DefinitionKind.Pipe, outputCtx, /* forceShared */ true); + this.addPipeDependency(pipe); this._creationMode.push( - o.importExpr(R3.pipe) - .callFn([ - o.literal(slot), pipeDefinition, pipeDefinition.callMethod(R3.NEW_METHOD, []) - ]) - .toStmt()); + o.importExpr(R3.pipe).callFn([o.literal(slot), o.literal(name)]).toStmt()); }); } @@ -736,7 +758,7 @@ class TemplateDefinitionBuilder implements TemplateAstVisitor, LocalResolver { const templateVisitor = new TemplateDefinitionBuilder( this.outputCtx, this.constantPool, this.reflector, templateContext, this.bindingScope.nestedScope(), this.level + 1, this.ngContentSelectors, contextName, - templateName, this.pipes, []); + templateName, this.pipes, [], this.addPipeDependency); const templateFunctionExpr = templateVisitor.buildTemplateFunction(ast.children, ast.variables); this._postfix.push(templateFunctionExpr.toDeclStmt(templateName, null)); } @@ -1044,11 +1066,7 @@ class ValueConverter extends AstMemoryEfficientTransformer { // AstMemoryEfficientTransformer visitPipe(ast: BindingPipe, context: any): AST { // Allocate a slot to create the pipe - let slot = this.pipeSlots.get(ast.name); - if (slot == null) { - slot = this.allocateSlot(); - this.pipeSlots.set(ast.name, slot); - } + const slot = this.allocateSlot(); const slotPseudoLocal = `PIPE:${slot}`; const target = new PropertyRead(ast.span, new ImplicitReceiver(ast.span), slotPseudoLocal); const bindingId = pipeBinding(ast.args); diff --git a/packages/compiler/test/render3/r3_compiler_compliance_spec.ts b/packages/compiler/test/render3/r3_compiler_compliance_spec.ts index df55aadd4a..25a7cfd127 100644 --- a/packages/compiler/test/render3/r3_compiler_compliance_spec.ts +++ b/packages/compiler/test/render3/r3_compiler_compliance_spec.ts @@ -97,7 +97,6 @@ describe('compiler compliance', () => { template: function ChildComponent_Template(ctx: IDENT, cm: IDENT) { if (cm) { $r3$.ɵT(0, 'child-view'); - } } });`; @@ -784,7 +783,10 @@ describe('compiler compliance', () => { transform(value: any, ...args: any[]) { return value; } } - @Component({selector: 'my-app', template: '{{name | myPipe:size | myPurePipe:size }}'}) + @Component({ + selector: 'my-app', + template: '{{name | myPipe:size | myPurePipe:size }}

{{ name | myPurePipe:size }}

' + }) export class MyApp { name = 'World'; size = 0; @@ -799,19 +801,18 @@ describe('compiler compliance', () => { it('should render pipes', () => { const MyPipeDefinition = ` static ngPipeDef = $r3$.ɵdefinePipe( - {type: MyPipe, factory: function MyPipe_Factory() { return new MyPipe(); }}); + {name: 'myPipe', type: MyPipe, factory: function MyPipe_Factory() { return new MyPipe(); }}); `; const MyPurePipeDefinition = ` static ngPipeDef = $r3$.ɵdefinePipe({ + name: 'myPurePipe', type: MyPurePipe, factory: function MyPurePipe_Factory() { return new MyPurePipe(); }, pure: true });`; const MyAppDefinition = ` - const $MyPurePipe_ngPipeDef$ = MyPurePipe.ngPipeDef; - const $MyPipe_ngPipeDef$ = MyPipe.ngPipeDef; … static ngComponentDef = $r3$.ɵdefineComponent({ type: MyApp, @@ -820,11 +821,17 @@ describe('compiler compliance', () => { template: function MyApp_Template(ctx: IDENT, cm: IDENT) { if (cm) { $r3$.ɵT(0); - $r3$.ɵPp(1, $MyPurePipe_ngPipeDef$, $MyPurePipe_ngPipeDef$.n()); - $r3$.ɵPp(2, $MyPipe_ngPipeDef$, $MyPipe_ngPipeDef$.n()); + $r3$.ɵPp(1, 'myPurePipe'); + $r3$.ɵPp(2, 'myPipe'); + $r3$.ɵE(3, 'p'); + $r3$.ɵT(4); + $r3$.ɵPp(5, 'myPurePipe'); + $r3$.ɵe(); } $r3$.ɵt(0, $r3$.ɵi1('', $r3$.ɵpb2(1, $r3$.ɵpb2(2,ctx.name, ctx.size), ctx.size), '')); - } + $r3$.ɵt(4, $r3$.ɵi1('', $r3$.ɵpb2(5, ctx.name, ctx.size), '')); + }, + pipes: [MyPurePipe, MyPipe] });`; const result = compile(files, angularFiles); diff --git a/packages/core/src/render3/pipe.ts b/packages/core/src/render3/pipe.ts index 156cfb4143..83880d3004 100644 --- a/packages/core/src/render3/pipe.ts +++ b/packages/core/src/render3/pipe.ts @@ -16,11 +16,10 @@ import {pureFunction1, pureFunction2, pureFunction3, pureFunction4, pureFunction * Create a pipe. * * @param index Pipe index where the pipe will be stored. - * @param pipeDef Pipe definition object for registering life cycle hooks. - * @param firstInstance (optional) The first instance of the pipe that can be reused for pure pipes. + * @param pipeName The name of the pipe * @returns T the instance of the pipe. */ -export function pipe(index: number, pipeName: string, firstInstance?: any): any { +export function pipe(index: number, pipeName: string): any { const tView = getTView(); let pipeDef: PipeDef; @@ -34,7 +33,7 @@ export function pipe(index: number, pipeName: string, firstInstance?: any): any pipeDef = tView.data[index] as PipeDef; } - const pipeInstance = pipeDef.pure && firstInstance ? firstInstance : pipeDef.n(); + const pipeInstance = pipeDef.n(); store(index, pipeInstance); return pipeInstance; } diff --git a/packages/core/test/render3/compiler_canonical/pipes_spec.ts b/packages/core/test/render3/compiler_canonical/pipes_spec.ts index b973e25cbc..9221efce35 100644 --- a/packages/core/test/render3/compiler_canonical/pipes_spec.ts +++ b/packages/core/test/render3/compiler_canonical/pipes_spec.ts @@ -155,12 +155,11 @@ describe('pipes', () => { selectors: [['my-app']], factory: function MyApp_Factory() { return new MyApp(); }, template: function MyApp_Template(ctx: $MyApp$, cm: $boolean$) { - let $pi$: $any$; if (cm) { $r3$.ɵT(0); - $pi$ = $r3$.ɵPp(1, 'myPurePipe'); + $r3$.ɵPp(1, 'myPurePipe'); $r3$.ɵT(2); - $r3$.ɵPp(3, 'myPurePipe', $pi$); + $r3$.ɵPp(3, 'myPurePipe'); $r3$.ɵC(4, C4, '', ['oneTimeIf', '']); } $r3$.ɵt(0, $r3$.ɵi1('', $r3$.ɵpb2(1, ctx.name, ctx.size), '')); @@ -173,7 +172,7 @@ describe('pipes', () => { if (cm) { $r3$.ɵE(0, 'div'); $r3$.ɵT(1); - $r3$.ɵPp(2, 'myPurePipe', $pi$); + $r3$.ɵPp(2, 'myPurePipe'); $r3$.ɵe(); } $r3$.ɵt(1, $r3$.ɵi1('', $r3$.ɵpb2(2, ctx.name, ctx.size), '')); diff --git a/packages/core/test/render3/pipe_spec.ts b/packages/core/test/render3/pipe_spec.ts index de043e1ca5..4590a93103 100644 --- a/packages/core/test/render3/pipe_spec.ts +++ b/packages/core/test/render3/pipe_spec.ts @@ -183,49 +183,6 @@ describe('pipe', () => { expect(renderToHtml(Template, person, null, defs)).toEqual('bart state:2'); expect(renderToHtml(Template, person, null, defs)).toEqual('bart state:2'); }); - - it('should cache pure pipes', () => { - function Template(ctx: any, cm: boolean) { - let pipeInstance: any; - if (cm) { - elementStart(0, 'div'); - pipeInstance = pipe(1, 'countingPipe'); - elementEnd(); - elementStart(2, 'div'); - pipe(3, 'countingPipe', pipeInstance); - elementEnd(); - container(4); - } - elementProperty(0, 'someProp', bind(pipeBind1(1, true))); - elementProperty(2, 'someProp', bind(pipeBind1(3, true))); - pipeInstances.push(load(1), load(3)); - containerRefreshStart(4); - { - for (let i of [1, 2]) { - let cm1 = embeddedViewStart(1); - { - if (cm1) { - elementStart(0, 'div'); - pipe(1, 'countingPipe', pipeInstance); - elementEnd(); - } - elementProperty(0, 'someProp', bind(pipeBind1(1, true))); - pipeInstances.push(load(1)); - } - embeddedViewEnd(); - } - } - containerRefreshEnd(); - } - - const pipeInstances: CountingPipe[] = []; - renderToHtml(Template, {}, null, defs, rendererFactory2); - expect(pipeInstances.length).toEqual(4); - expect(pipeInstances[0]).toBeAnInstanceOf(CountingPipe); - expect(pipeInstances[1]).toBe(pipeInstances[0]); - expect(pipeInstances[2]).toBe(pipeInstances[0]); - expect(pipeInstances[3]).toBe(pipeInstances[0]); - }); }); describe('impure', () => {