From d70c26cc067ee90e72114507747c67d7b68e9423 Mon Sep 17 00:00:00 2001 From: Andrew Scott Date: Thu, 17 Dec 2020 09:50:02 -0800 Subject: [PATCH] refactor(language-service): Have TemplateTarget recognize two way bindings (#40185) Adjust the visitor logic of the template target as well as the consumption of the visitor result to account for two-way bindings. This sets up downstream consumers for being able to handle the possibility of a template position that targets both an input and an output. PR Close #40185 --- .../language-service/ivy/template_target.ts | 94 ++++++++++--------- .../ivy/test/legacy/template_target_spec.ts | 43 +++++---- packages/language-service/ivy/utils.ts | 10 ++ 3 files changed, 86 insertions(+), 61 deletions(-) diff --git a/packages/language-service/ivy/template_target.ts b/packages/language-service/ivy/template_target.ts index 6c94a2b544..09650a2df3 100644 --- a/packages/language-service/ivy/template_target.ts +++ b/packages/language-service/ivy/template_target.ts @@ -156,11 +156,6 @@ export function getTargetAtPosition(template: t.Node[], position: number): Templ } } - let parent: t.Node|e.AST|null = null; - if (path.length >= 2) { - parent = path[path.length - 2]; - } - // Given the candidate node, determine the full targeted context. let nodeInContext: TargetContext; if (candidate instanceof e.AST) { @@ -191,7 +186,16 @@ export function getTargetAtPosition(template: t.Node[], position: number): Templ (candidate instanceof t.BoundAttribute || candidate instanceof t.BoundEvent || candidate instanceof t.TextAttribute) && candidate.keySpan !== undefined) { - if (isWithin(position, candidate.keySpan)) { + const previousCandidate = path[path.length - 2]; + if (candidate instanceof t.BoundEvent && previousCandidate instanceof t.BoundAttribute && + candidate.name === previousCandidate.name + 'Change') { + const boundAttribute: t.BoundAttribute = previousCandidate; + const boundEvent: t.BoundEvent = candidate; + nodeInContext = { + kind: TargetNodeKind.TwoWayBindingContext, + nodes: [boundAttribute, boundEvent], + }; + } else if (isWithin(position, candidate.keySpan)) { nodeInContext = { kind: TargetNodeKind.AttributeInKeyContext, node: candidate, @@ -209,6 +213,13 @@ export function getTargetAtPosition(template: t.Node[], position: number): Templ }; } + let parent: t.Node|e.AST|null = null; + if (nodeInContext.kind === TargetNodeKind.TwoWayBindingContext && path.length >= 3) { + parent = path[path.length - 3]; + } else if (path.length >= 2) { + parent = path[path.length - 2]; + } + return {position, context: nodeInContext, template: context, parent}; } @@ -247,20 +258,25 @@ class TemplateTargetVisitor implements t.Visitor { private constructor(private readonly position: number) {} visit(node: t.Node) { - const last: t.Node|e.AST|undefined = this.path[this.path.length - 1]; - if (last && isTemplateNodeWithKeyAndValue(last) && isWithin(this.position, last.keySpan)) { - // We've already identified that we are within a `keySpan` of a node. - // We should stop processing nodes at this point to prevent matching - // any other nodes. This can happen when the end span of a different node - // touches the start of the keySpan for the candidate node. Because - // our `isWithin` logic is inclusive on both ends, we can match both nodes. - return; - } const {start, end} = getSpanIncludingEndTag(node); if (!isWithin(this.position, {start, end})) { return; } + const last: t.Node|e.AST|undefined = this.path[this.path.length - 1]; + const withinKeySpanOfLastNode = + last && isTemplateNodeWithKeyAndValue(last) && isWithin(this.position, last.keySpan); + const withinKeySpanOfCurrentNode = + isTemplateNodeWithKeyAndValue(node) && isWithin(this.position, node.keySpan); + if (withinKeySpanOfLastNode && !withinKeySpanOfCurrentNode) { + // We've already identified that we are within a `keySpan` of a node. + // Unless we are _also_ in the `keySpan` of the current node (happens with two way bindings), + // we should stop processing nodes at this point to prevent matching any other nodes. This can + // happen when the end span of a different node touches the start of the keySpan for the + // candidate node. Because our `isWithin` logic is inclusive on both ends, we can match both + // nodes. + return; + } if (isTemplateNodeWithKeyAndValue(node) && !isWithinKeyValue(this.position, node)) { // If cursor is within source span but not within key span or value span, // do not return the node. @@ -272,31 +288,33 @@ class TemplateTargetVisitor implements t.Visitor { } visitElement(element: t.Element) { + this.visitElementOrTemplate(element); + } + + + visitTemplate(template: t.Template) { + this.visitElementOrTemplate(template); + } + + visitElementOrTemplate(element: t.Template|t.Element) { this.visitAll(element.attributes); this.visitAll(element.inputs); this.visitAll(element.outputs); + if (element instanceof t.Template) { + this.visitAll(element.templateAttrs); + } this.visitAll(element.references); - const last: t.Node|e.AST|undefined = this.path[this.path.length - 1]; + if (element instanceof t.Template) { + this.visitAll(element.variables); + } + // If we get here and have not found a candidate node on the element itself, proceed with // looking for a more specific node on the element children. - if (last === element) { - this.visitAll(element.children); + if (this.path[this.path.length - 1] !== element) { + return; } - } - visitTemplate(template: t.Template) { - this.visitAll(template.attributes); - this.visitAll(template.inputs); - this.visitAll(template.outputs); - this.visitAll(template.templateAttrs); - this.visitAll(template.references); - this.visitAll(template.variables); - const last: t.Node|e.AST|undefined = this.path[this.path.length - 1]; - // If we get here and have not found a candidate node on the template itself, proceed with - // looking for a more specific node on the template children. - if (last === template) { - this.visitAll(template.children); - } + this.visitAll(element.children); } visitContent(content: t.Content) { @@ -321,18 +339,6 @@ class TemplateTargetVisitor implements t.Visitor { } visitBoundEvent(event: t.BoundEvent) { - const isTwoWayBinding = - this.path.some(n => n instanceof t.BoundAttribute && event.name === n.name + 'Change'); - if (isTwoWayBinding) { - // For two-way binding aka banana-in-a-box, there are two matches: - // BoundAttribute and BoundEvent. Both have the same spans. We choose to - // return BoundAttribute because it matches the identifier name verbatim. - // TODO: For operations like go to definition, ideally we want to return - // both. - this.path.pop(); // remove bound event from the AST path - return; - } - // An event binding with no value (e.g. `(event|)`) parses to a `BoundEvent` with a // `LiteralPrimitive` handler with value `'ERROR'`, as opposed to a property binding with no // value which has an `EmptyExpr` as its value. This is a synthetic node created by the binding diff --git a/packages/language-service/ivy/test/legacy/template_target_spec.ts b/packages/language-service/ivy/test/legacy/template_target_spec.ts index 52e58a7903..56b62ade24 100644 --- a/packages/language-service/ivy/test/legacy/template_target_spec.ts +++ b/packages/language-service/ivy/test/legacy/template_target_spec.ts @@ -10,7 +10,7 @@ import {ParseError, parseTemplate} from '@angular/compiler'; import * as e from '@angular/compiler/src/expression_parser/ast'; // e for expression AST import * as t from '@angular/compiler/src/render3/r3_ast'; // t for template AST -import {getTargetAtPosition, SingleNodeTarget, TargetNodeKind} from '../../template_target'; +import {getTargetAtPosition, SingleNodeTarget, TargetNodeKind, TwoWayBindingContext} from '../../template_target'; import {isExpressionNode, isTemplateNode} from '../../utils'; interface ParseResult { @@ -180,11 +180,11 @@ describe('getTargetAtPosition for template AST', () => { it('should locate template bound attribute key in two-way binding', () => { const {errors, nodes, position} = parse(``); expect(errors).toBe(null); - const {context} = getTargetAtPosition(nodes, position)!; - const {node} = context as SingleNodeTarget; - expect(isTemplateNode(node!)).toBe(true); - expect(node).toBeInstanceOf(t.BoundAttribute); - expect((node as t.BoundAttribute).name).toBe('foo'); + const {context, parent} = getTargetAtPosition(nodes, position)!; + expect(parent).toBeInstanceOf(t.Template); + const {nodes: [boundAttribute, boundEvent]} = context as TwoWayBindingContext; + expect(boundAttribute.name).toBe('foo'); + expect(boundEvent.name).toBe('fooChange'); }); it('should locate template bound attribute value in two-way binding', () => { @@ -193,8 +193,12 @@ describe('getTargetAtPosition for template AST', () => { const {context} = getTargetAtPosition(nodes, position)!; const {node} = context as SingleNodeTarget; expect(isExpressionNode(node!)).toBe(true); - expect(node).toBeInstanceOf(e.PropertyRead); - expect((node as e.PropertyRead).name).toBe('bar'); + // It doesn't actually matter if the template target returns the read or the write. + // When the template target returns a property read, we only use the LHS downstream because the + // RHS would have its own node in the AST that would have been returned instead. The LHS of the + // `e.PropertyWrite` is the same as the `e.PropertyRead`. + expect((node instanceof e.PropertyRead) || (node instanceof e.PropertyWrite)).toBeTrue(); + expect((node as e.PropertyRead | e.PropertyWrite).name).toBe('bar'); }); it('should locate template bound event key', () => { @@ -342,21 +346,26 @@ describe('getTargetAtPosition for template AST', () => { it('should locate bound attribute key in two-way binding', () => { const {errors, nodes, position} = parse(``); expect(errors).toBe(null); - const {context} = getTargetAtPosition(nodes, position)!; - const {node} = context as SingleNodeTarget; - expect(isTemplateNode(node!)).toBe(true); - expect(node).toBeInstanceOf(t.BoundAttribute); - expect((node as t.BoundAttribute).name).toBe('foo'); + const {context, parent} = getTargetAtPosition(nodes, position)!; + expect(parent).toBeInstanceOf(t.Element); + const {nodes: [boundAttribute, boundEvent]} = context as TwoWayBindingContext; + expect(boundAttribute.name).toBe('foo'); + expect(boundEvent.name).toBe('fooChange'); }); - it('should locate bound attribute value in two-way binding', () => { + it('should locate node when in value span of two-way binding', () => { const {errors, nodes, position} = parse(``); expect(errors).toBe(null); - const {context} = getTargetAtPosition(nodes, position)!; + const {context, parent} = getTargetAtPosition(nodes, position)!; + // It doesn't actually matter if the template target returns the read or the write. + // When the template target returns a property read, we only use the LHS downstream because the + // RHS would have its own node in the AST that would have been returned instead. The LHS of the + // `e.PropertyWrite` is the same as the `e.PropertyRead`. + expect((parent instanceof t.BoundAttribute) || (parent instanceof t.BoundEvent)).toBe(true); const {node} = context as SingleNodeTarget; expect(isExpressionNode(node!)).toBe(true); - expect(node).toBeInstanceOf(e.PropertyRead); - expect((node as e.PropertyRead).name).toBe('bar'); + expect((node instanceof e.PropertyRead) || (node instanceof e.PropertyWrite)).toBeTrue(); + expect((node as e.PropertyRead | e.PropertyWrite).name).toBe('bar'); }); it('should locate switch value in ICUs', () => { diff --git a/packages/language-service/ivy/utils.ts b/packages/language-service/ivy/utils.ts index 7c67840c3c..57c80704f2 100644 --- a/packages/language-service/ivy/utils.ts +++ b/packages/language-service/ivy/utils.ts @@ -54,6 +54,16 @@ export function isTemplateNodeWithKeyAndValue(node: t.Node|e.AST): node is NodeW return isTemplateNode(node) && node.hasOwnProperty('keySpan'); } +export function isWithinKey(position: number, node: NodeWithKeyAndValue): boolean { + let {keySpan, valueSpan} = node; + if (valueSpan === undefined && node instanceof TmplAstBoundEvent) { + valueSpan = node.handlerSpan; + } + const isWithinKeyValue = + isWithin(position, keySpan) || !!(valueSpan && isWithin(position, valueSpan)); + return isWithinKeyValue; +} + export function isWithinKeyValue(position: number, node: NodeWithKeyAndValue): boolean { let {keySpan, valueSpan} = node; if (valueSpan === undefined && node instanceof TmplAstBoundEvent) {