From 0226a11c185da2d1e6f7833972d3f12205a6ae59 Mon Sep 17 00:00:00 2001 From: Andrew Scott Date: Wed, 17 Mar 2021 11:31:57 -0700 Subject: [PATCH] fix(language-service): Only provide Angular property completions in templates (#41278) When possible, the @angular/language-service should only provide information related to Angular. When there is an embedded language, like inline templates, editor extensions should have the ability to create virtual documents and forward the requests to the relevant providers for that language type (see https://github.com/angular/vscode-ng-language-service/pull/1212). This commit removes all dom schema completions in both inline and external templates and provides only the Angular syntax for property completions on elements. PR Close #41278 --- .../ivy/attribute_completions.ts | 47 +++++-------------- packages/language-service/ivy/completions.ts | 16 +++---- .../language-service/ivy/language_service.ts | 3 +- .../ivy/test/completions_spec.ts | 21 +++++---- 4 files changed, 34 insertions(+), 53 deletions(-) diff --git a/packages/language-service/ivy/attribute_completions.ts b/packages/language-service/ivy/attribute_completions.ts index 295013862d..4752c3983d 100644 --- a/packages/language-service/ivy/attribute_completions.ts +++ b/packages/language-service/ivy/attribute_completions.ts @@ -68,9 +68,11 @@ export interface DomAttributeCompletion { attribute: string; /** - * Whether this attribute is also a DOM property. + * Whether this attribute is also a DOM property. Note that this is required to be `true` because + * we only want to provide DOM attributes when there is an Angular syntax associated with them + * (`[propertyName]=""`). */ - isAlsoProperty: boolean; + isAlsoProperty: true; } /** @@ -180,8 +182,7 @@ export type AttributeCompletion = DomAttributeCompletion|DomPropertyCompletion| */ export function buildAttributeCompletionTable( component: ts.ClassDeclaration, element: TmplAstElement|TmplAstTemplate, - checker: TemplateTypeChecker, - includeDomSchemaAttributes: boolean): Map { + checker: TemplateTypeChecker): Map { const table = new Map(); // Use the `ElementSymbol` or `TemplateSymbol` to iterate over directives present on the node, and @@ -333,22 +334,16 @@ export function buildAttributeCompletionTable( } // Finally, add any DOM attributes not already covered by inputs. - if (element instanceof TmplAstElement && includeDomSchemaAttributes) { + if (element instanceof TmplAstElement) { for (const {attribute, property} of checker.getPotentialDomBindings(element.name)) { const isAlsoProperty = attribute === property; - if (!table.has(attribute)) { + if (!table.has(attribute) && isAlsoProperty) { table.set(attribute, { kind: AttributeCompletionKind.DomAttribute, attribute, isAlsoProperty, }); } - if (!isAlsoProperty && !table.has(property)) { - table.set(property, { - kind: AttributeCompletionKind.DomProperty, - property, - }); - } } } @@ -449,30 +444,14 @@ export function addAttributeCompletionEntries( break; } case AttributeCompletionKind.DomAttribute: { - if (isAttributeContext) { - // Offer a completion of an attribute binding. - entries.push({ - kind: unsafeCastDisplayInfoKindToScriptElementKind(DisplayInfoKind.ATTRIBUTE), - name: completion.attribute, - sortText: completion.attribute, - replacementSpan, - }); - if (completion.isAlsoProperty) { - // Offer a completion of a property binding to the DOM property. - entries.push({ - kind: unsafeCastDisplayInfoKindToScriptElementKind(DisplayInfoKind.PROPERTY), - name: `[${completion.attribute}]`, - // In the case of DOM attributes, the property binding should sort after the attribute - // binding. - sortText: completion.attribute + '_1', - replacementSpan, - }); - } - } else if (completion.isAlsoProperty) { + if (isAttributeContext && completion.isAlsoProperty) { + // Offer a completion of a property binding to the DOM property. entries.push({ kind: unsafeCastDisplayInfoKindToScriptElementKind(DisplayInfoKind.PROPERTY), - name: completion.attribute, - sortText: completion.attribute, + name: `[${completion.attribute}]`, + // In the case of DOM attributes, the property binding should sort after the attribute + // binding. + sortText: completion.attribute + '_1', replacementSpan, }); } diff --git a/packages/language-service/ivy/completions.ts b/packages/language-service/ivy/completions.ts index 8c471a1a9f..49f6b13682 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 inlineTemplate: boolean) {} + private readonly targetDetails: TemplateTarget) {} /** * Analogue for `ts.LanguageService.getCompletionsAtPosition`. @@ -371,11 +371,9 @@ export class CompletionBuilder { const replacementSpan: ts.TextSpan = {start, length}; 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); - } + // 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, @@ -462,7 +460,7 @@ export class CompletionBuilder { } const attrTable = buildAttributeCompletionTable( - this.component, element, this.compiler.getTemplateTypeChecker(), this.inlineTemplate); + this.component, element, this.compiler.getTemplateTypeChecker()); let entries: ts.CompletionEntry[] = []; @@ -536,7 +534,7 @@ export class CompletionBuilder { } const attrTable = buildAttributeCompletionTable( - this.component, element, this.compiler.getTemplateTypeChecker(), this.inlineTemplate); + this.component, element, this.compiler.getTemplateTypeChecker()); if (!attrTable.has(name)) { return undefined; @@ -603,7 +601,7 @@ export class CompletionBuilder { } const attrTable = buildAttributeCompletionTable( - this.component, element, this.compiler.getTemplateTypeChecker(), this.inlineTemplate); + this.component, element, this.compiler.getTemplateTypeChecker()); 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 cbbff8d462..3ac3d5ff49 100644 --- a/packages/language-service/ivy/language_service.ts +++ b/packages/language-service/ivy/language_service.ts @@ -211,8 +211,7 @@ export class LanguageService { positionDetails.context.nodes[0] : positionDetails.context.node; return new CompletionBuilder( - this.tsLS, compiler, templateInfo.component, node, positionDetails, - isTypeScriptFile(fileName)); + this.tsLS, compiler, templateInfo.component, node, positionDetails); } getCompletionsAtPosition( diff --git a/packages/language-service/ivy/test/completions_spec.ts b/packages/language-service/ivy/test/completions_spec.ts index ff7ad570a0..b881a614e7 100644 --- a/packages/language-service/ivy/test/completions_spec.ts +++ b/packages/language-service/ivy/test/completions_spec.ts @@ -308,11 +308,11 @@ describe('completions', () => { ['div', 'span']); }); - it('should return DOM completions', () => { + it('should not return DOM completions for inline template', () => { const {appFile} = setupInlineTemplate(`
`, ''); appFile.moveCursorToText(''); const completions = appFile.getCompletionsAtPosition(); - expectContain( + expectDoesNotContain( completions, unsafeCastDisplayInfoKindToScriptElementKind(DisplayInfoKind.ELEMENT), ['div', 'span']); }); @@ -431,20 +431,25 @@ describe('completions', () => { describe('element attribute scope', () => { describe('dom completions', () => { - it('should not return completions dom completions in external template', () => { + it('should return dom property completions in external template', () => { const {templateFile} = setup(``, ''); templateFile.moveCursorToText(''); const completions = templateFile.getCompletionsAtPosition(); - expect(completions?.entries.length).toBe(0); + expectDoesNotContain( + completions, unsafeCastDisplayInfoKindToScriptElementKind(DisplayInfoKind.ATTRIBUTE), + ['value']); + expectContain( + completions, unsafeCastDisplayInfoKindToScriptElementKind(DisplayInfoKind.PROPERTY), + ['[value]']); }); - it('should return completions for a new element attribute', () => { + it('should return completions for a new element property', () => { const {appFile} = setupInlineTemplate(``, ''); appFile.moveCursorToText(''); const completions = appFile.getCompletionsAtPosition(); - expectContain( + expectDoesNotContain( completions, unsafeCastDisplayInfoKindToScriptElementKind(DisplayInfoKind.ATTRIBUTE), ['value']); expectContain( @@ -457,7 +462,7 @@ describe('completions', () => { appFile.moveCursorToText(''); const completions = appFile.getCompletionsAtPosition(); - expectContain( + expectDoesNotContain( completions, unsafeCastDisplayInfoKindToScriptElementKind(DisplayInfoKind.ATTRIBUTE), ['value']); expectContain( @@ -477,7 +482,7 @@ describe('completions', () => { expectDoesNotContain( completions, unsafeCastDisplayInfoKindToScriptElementKind(DisplayInfoKind.PROPERTY), ['[value]']); - expectContain( + expectDoesNotContain( completions, unsafeCastDisplayInfoKindToScriptElementKind(DisplayInfoKind.PROPERTY), ['value']); expectReplacementText(completions, appFile.contents, 'val');