From 371a2c82ea3f026e5eb4cbdc77fa3bcf03d588b1 Mon Sep 17 00:00:00 2001 From: Andrew Scott Date: Wed, 9 Dec 2020 09:31:17 -0800 Subject: [PATCH] refactor(language-service): adjust hybrid visitor to not throw away valid results (#40047) The visitor has a check in it with the goal of preventing the structural directive parent elements from matching when we have already found the candidate we want. However, this code did not check to ensure that it was looking at the correct type of node for this case and was evaluating this logic in places it shouldn't. This special check can be more easily done by simply not traversing the template children if we've already found a candidate on the template node itself. PR Close #40047 --- .../language-service/ivy/template_target.ts | 27 +++++++++---------- .../ivy/test/legacy/template_target_spec.ts | 12 +++++++++ 2 files changed, 24 insertions(+), 15 deletions(-) diff --git a/packages/language-service/ivy/template_target.ts b/packages/language-service/ivy/template_target.ts index e39180919f..3207cdcd42 100644 --- a/packages/language-service/ivy/template_target.ts +++ b/packages/language-service/ivy/template_target.ts @@ -102,19 +102,6 @@ class TemplateTargetVisitor implements t.Visitor { visit(node: t.Node) { const {start, end} = getSpanIncludingEndTag(node); if (isWithin(this.position, {start, end})) { - const length = end - start; - const last: t.Node|e.AST|undefined = this.path[this.path.length - 1]; - if (last) { - const {start, end} = isTemplateNode(last) ? getSpanIncludingEndTag(last) : last.sourceSpan; - const lastLength = end - start; - if (length > lastLength) { - // The current node has a span that is larger than the last node found - // so we do not descend into it. This typically means we have found - // a candidate in one of the root nodes so we do not need to visit - // other root nodes. - return; - } - } this.path.push(node); node.visit(this); } @@ -125,7 +112,12 @@ class TemplateTargetVisitor implements t.Visitor { this.visitAll(element.inputs); this.visitAll(element.outputs); this.visitAll(element.references); - this.visitAll(element.children); + 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 element itself, proceed with + // looking for a more specific node on the element children. + if (last === element) { + this.visitAll(element.children); + } } visitTemplate(template: t.Template) { @@ -135,7 +127,12 @@ class TemplateTargetVisitor implements t.Visitor { this.visitAll(template.templateAttrs); this.visitAll(template.references); this.visitAll(template.variables); - this.visitAll(template.children); + 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); + } } visitContent(content: t.Content) { 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 adbfb5f6d2..f54a374edc 100644 --- a/packages/language-service/ivy/test/legacy/template_target_spec.ts +++ b/packages/language-service/ivy/test/legacy/template_target_spec.ts @@ -577,6 +577,18 @@ describe('findNodeAtPosition for microsyntax expression', () => { expect((node as t.BoundAttribute).name).toBe('ngForOf'); }); + it('should locate bound attribute key when cursor is at the start', () => { + const {errors, nodes, position} = parse(`
`); + expect(errors).toBe(null); + // TODO(atscott): Fix this - we throw away the result because we match the variable node, after + // the attribute binding, then throw away the result because we aren't in the variable key + expect(getTargetAtPosition(nodes, position)).toBeNull(); + // const {node} = getTargetAtPosition(nodes, position)!; + // expect(isTemplateNode(node!)).toBe(true); + // expect(node).toBeInstanceOf(t.BoundAttribute); + // expect((node as t.BoundAttribute).name).toBe('ngForOf'); + }); + it('should locate bound attribute key for trackBy', () => { const {errors, nodes, position} = parse(`
`);