From 45216ccc0d3cbb9182f879559c8e451e1bbe32b4 Mon Sep 17 00:00:00 2001 From: Andrew Scott Date: Thu, 4 Mar 2021 08:46:50 -0800 Subject: [PATCH] fix(language-service): Only provide dom completions for inline templates (#41078) We currently provide completions for DOM elements in the schema as well as attributes when we are in the context of an external template. However, these completions are already provided by other extensions for HTML contexts (like Emmet). To avoid duplication of results, this commit updates the language service to exclude DOM completions for external templates. They are still provided for inline templates because those are not handled by the HTML language extensions. PR Close #41078 --- .../ivy/attribute_completions.ts | 5 +- packages/language-service/ivy/completions.ts | 28 ++++---- .../language-service/ivy/language_service.ts | 3 +- .../ivy/test/completions_spec.ts | 69 ++++++++++++++++--- 4 files changed, 80 insertions(+), 25 deletions(-) diff --git a/packages/language-service/ivy/attribute_completions.ts b/packages/language-service/ivy/attribute_completions.ts index 275a494343..295013862d 100644 --- a/packages/language-service/ivy/attribute_completions.ts +++ b/packages/language-service/ivy/attribute_completions.ts @@ -180,7 +180,8 @@ export type AttributeCompletion = DomAttributeCompletion|DomPropertyCompletion| */ export function buildAttributeCompletionTable( component: ts.ClassDeclaration, element: TmplAstElement|TmplAstTemplate, - checker: TemplateTypeChecker): Map { + checker: TemplateTypeChecker, + includeDomSchemaAttributes: boolean): Map { const table = new Map(); // Use the `ElementSymbol` or `TemplateSymbol` to iterate over directives present on the node, and @@ -332,7 +333,7 @@ export function buildAttributeCompletionTable( } // Finally, add any DOM attributes not already covered by inputs. - if (element instanceof TmplAstElement) { + if (element instanceof TmplAstElement && includeDomSchemaAttributes) { for (const {attribute, property} of checker.getPotentialDomBindings(element.name)) { const isAlsoProperty = attribute === property; if (!table.has(attribute)) { diff --git a/packages/language-service/ivy/completions.ts b/packages/language-service/ivy/completions.ts index 069348950f..8c471a1a9f 100644 --- a/packages/language-service/ivy/completions.ts +++ b/packages/language-service/ivy/completions.ts @@ -57,7 +57,7 @@ export class CompletionBuilder { constructor( private readonly tsLS: ts.LanguageService, private readonly compiler: NgCompiler, private readonly component: ts.ClassDeclaration, private readonly node: N, - private readonly targetDetails: TemplateTarget) {} + private readonly targetDetails: TemplateTarget, private readonly inlineTemplate: boolean) {} /** * Analogue for `ts.LanguageService.getCompletionsAtPosition`. @@ -370,14 +370,18 @@ export class CompletionBuilder { const replacementSpan: ts.TextSpan = {start, length}; - const entries: ts.CompletionEntry[] = - Array.from(templateTypeChecker.getPotentialElementTags(this.component)) - .map(([tag, directive]) => ({ - kind: tagCompletionKind(directive), - name: tag, - sortText: tag, - replacementSpan, - })); + let potentialTags = Array.from(templateTypeChecker.getPotentialElementTags(this.component)); + if (!this.inlineTemplate) { + // If we are in an external template, don't provide non-Angular tags (directive === null) + // because we expect other extensions (i.e. Emmet) to provide those for HTML files. + potentialTags = potentialTags.filter(([_, directive]) => directive !== null); + } + const entries: ts.CompletionEntry[] = potentialTags.map(([tag, directive]) => ({ + kind: tagCompletionKind(directive), + name: tag, + sortText: tag, + replacementSpan, + })); return { entries, @@ -458,7 +462,7 @@ export class CompletionBuilder { } const attrTable = buildAttributeCompletionTable( - this.component, element, this.compiler.getTemplateTypeChecker()); + this.component, element, this.compiler.getTemplateTypeChecker(), this.inlineTemplate); let entries: ts.CompletionEntry[] = []; @@ -532,7 +536,7 @@ export class CompletionBuilder { } const attrTable = buildAttributeCompletionTable( - this.component, element, this.compiler.getTemplateTypeChecker()); + this.component, element, this.compiler.getTemplateTypeChecker(), this.inlineTemplate); if (!attrTable.has(name)) { return undefined; @@ -599,7 +603,7 @@ export class CompletionBuilder { } const attrTable = buildAttributeCompletionTable( - this.component, element, this.compiler.getTemplateTypeChecker()); + this.component, element, this.compiler.getTemplateTypeChecker(), this.inlineTemplate); if (!attrTable.has(name)) { return undefined; diff --git a/packages/language-service/ivy/language_service.ts b/packages/language-service/ivy/language_service.ts index 21908703a9..7167dbeb75 100644 --- a/packages/language-service/ivy/language_service.ts +++ b/packages/language-service/ivy/language_service.ts @@ -201,7 +201,8 @@ export class LanguageService { positionDetails.context.nodes[0] : positionDetails.context.node; return new CompletionBuilder( - this.tsLS, compiler, templateInfo.component, node, positionDetails); + this.tsLS, compiler, templateInfo.component, node, positionDetails, + isTypeScriptFile(fileName)); } getCompletionsAtPosition( diff --git a/packages/language-service/ivy/test/completions_spec.ts b/packages/language-service/ivy/test/completions_spec.ts index 62d4139dec..ff7ad570a0 100644 --- a/packages/language-service/ivy/test/completions_spec.ts +++ b/packages/language-service/ivy/test/completions_spec.ts @@ -299,10 +299,19 @@ describe('completions', () => { }); describe('element tag scope', () => { - it('should return DOM completions', () => { + it('should not return DOM completions for external template', () => { const {templateFile} = setup(`
`, ''); templateFile.moveCursorToText(''); const completions = templateFile.getCompletionsAtPosition(); + expectDoesNotContain( + completions, unsafeCastDisplayInfoKindToScriptElementKind(DisplayInfoKind.ELEMENT), + ['div', 'span']); + }); + + it('should return DOM completions', () => { + const {appFile} = setupInlineTemplate(`
`, ''); + appFile.moveCursorToText(''); + const completions = appFile.getCompletionsAtPosition(); expectContain( completions, unsafeCastDisplayInfoKindToScriptElementKind(DisplayInfoKind.ELEMENT), ['div', 'span']); @@ -422,11 +431,19 @@ describe('completions', () => { describe('element attribute scope', () => { describe('dom completions', () => { - it('should return completions for a new element attribute', () => { + it('should not return completions dom completions in external template', () => { const {templateFile} = setup(``, ''); templateFile.moveCursorToText(''); const completions = templateFile.getCompletionsAtPosition(); + expect(completions?.entries.length).toBe(0); + }); + + it('should return completions for a new element attribute', () => { + const {appFile} = setupInlineTemplate(``, ''); + appFile.moveCursorToText(''); + + const completions = appFile.getCompletionsAtPosition(); expectContain( completions, unsafeCastDisplayInfoKindToScriptElementKind(DisplayInfoKind.ATTRIBUTE), ['value']); @@ -436,24 +453,24 @@ describe('completions', () => { }); it('should return completions for a partial attribute', () => { - const {templateFile} = setup(``, ''); - templateFile.moveCursorToText(''); + const {appFile} = setupInlineTemplate(``, ''); + appFile.moveCursorToText(''); - const completions = templateFile.getCompletionsAtPosition(); + const completions = appFile.getCompletionsAtPosition(); expectContain( completions, unsafeCastDisplayInfoKindToScriptElementKind(DisplayInfoKind.ATTRIBUTE), ['value']); expectContain( completions, unsafeCastDisplayInfoKindToScriptElementKind(DisplayInfoKind.PROPERTY), ['[value]']); - expectReplacementText(completions, templateFile.contents, 'val'); + expectReplacementText(completions, appFile.contents, 'val'); }); it('should return completions for a partial property binding', () => { - const {templateFile} = setup(``, ''); - templateFile.moveCursorToText('[val¦]'); + const {appFile} = setupInlineTemplate(``, ''); + appFile.moveCursorToText('[val¦]'); - const completions = templateFile.getCompletionsAtPosition(); + const completions = appFile.getCompletionsAtPosition(); expectDoesNotContain( completions, unsafeCastDisplayInfoKindToScriptElementKind(DisplayInfoKind.ATTRIBUTE), ['value']); @@ -463,7 +480,7 @@ describe('completions', () => { expectContain( completions, unsafeCastDisplayInfoKindToScriptElementKind(DisplayInfoKind.PROPERTY), ['value']); - expectReplacementText(completions, templateFile.contents, 'val'); + expectReplacementText(completions, appFile.contents, 'val'); }); }); @@ -779,3 +796,35 @@ function setup( }); return {templateFile: project.openFile('test.html')}; } + +function setupInlineTemplate( + template: string, classContents: string, otherDeclarations: {[name: string]: string} = {}): { + appFile: OpenBuffer, +} { + const decls = ['AppCmp', ...Object.keys(otherDeclarations)]; + + const otherDirectiveClassDecls = Object.values(otherDeclarations).join('\n\n'); + + const env = LanguageServiceTestEnv.setup(); + const project = env.addProject('test', { + 'test.ts': ` + import {Component, Directive, NgModule, Pipe, TemplateRef} from '@angular/core'; + + @Component({ + template: '${template}', + selector: 'app-cmp', + }) + export class AppCmp { + ${classContents} + } + + ${otherDirectiveClassDecls} + + @NgModule({ + declarations: [${decls.join(', ')}], + }) + export class AppModule {} + `, + }); + return {appFile: project.openFile('test.ts')}; +}