From afd68e5674e99a64fe5acc40e08cc8386dc6a454 Mon Sep 17 00:00:00 2001 From: Kristiyan Kostadinov Date: Sun, 23 May 2021 12:14:02 +0200 Subject: [PATCH] feat(compiler): emit diagnostic for shadow dom components with an invalid selector (#42245) This is based on a discussion we had a few weeks ago. Currently if a component uses `ViewEncapsulation.ShadowDom` and its selector doesn't meet the requirements for a custom element tag name, a vague error will be thrown at runtime saying something like "Element does not support attachShadowRoot". These changes add a new diagnostic to the compiler that validates the component selector and gives a better error message during compilation. PR Close #42245 --- aio/content/errors/NG2009.md | 32 ++++++ .../public-api/compiler-cli/error_code.d.ts | 1 + .../src/ngtsc/annotations/src/component.ts | 64 +++++++++-- .../src/ngtsc/diagnostics/src/error_code.ts | 7 ++ .../compiler-cli/test/ngtsc/ngtsc_spec.ts | 105 ++++++++++++++++++ 5 files changed, 197 insertions(+), 12 deletions(-) create mode 100644 aio/content/errors/NG2009.md diff --git a/aio/content/errors/NG2009.md b/aio/content/errors/NG2009.md new file mode 100644 index 0000000000..3f9d535be7 --- /dev/null +++ b/aio/content/errors/NG2009.md @@ -0,0 +1,32 @@ +@name Invalid Shadow DOM selector +@category compiler +@shortDescription Component selector does not match shadow DOM requirements + +@description +The selector of a component using `ViewEncapsulation.ShadowDom` doesn't match the custom element tag name requirements. + +In order for a tag name to be considered a valid custom element name, it has to: +* Be in lower case. +* Contain a hyphen. +* Start with a letter (a-z). + +@debugging +Rename your component's selector so that it matches the requirements. + +**Before:** +```typescript +@Component({ + selector: 'comp', + encapsulation: ViewEncapsulation.ShadowDom + ... +}) +``` + +**After:** +```typescript +@Component({ + selector: 'app-comp', + encapsulation: ViewEncapsulation.ShadowDom + ... +}) +``` diff --git a/goldens/public-api/compiler-cli/error_code.d.ts b/goldens/public-api/compiler-cli/error_code.d.ts index 55d3c29d87..25154c5c60 100644 --- a/goldens/public-api/compiler-cli/error_code.d.ts +++ b/goldens/public-api/compiler-cli/error_code.d.ts @@ -15,6 +15,7 @@ export declare enum ErrorCode { DIRECTIVE_INHERITS_UNDECORATED_CTOR = 2006, UNDECORATED_CLASS_USING_ANGULAR_FEATURES = 2007, COMPONENT_RESOURCE_NOT_FOUND = 2008, + COMPONENT_INVALID_SHADOW_DOM_SELECTOR = 2009, SYMBOL_NOT_EXPORTED = 3001, SYMBOL_EXPORTED_UNDER_DIFFERENT_NAME = 3002, IMPORT_CYCLE_DETECTED = 3003, diff --git a/packages/compiler-cli/src/ngtsc/annotations/src/component.ts b/packages/compiler-cli/src/ngtsc/annotations/src/component.ts index ab0da27d6e..c497ab6564 100644 --- a/packages/compiler-cli/src/ngtsc/annotations/src/component.ts +++ b/packages/compiler-cli/src/ngtsc/annotations/src/component.ts @@ -7,10 +7,11 @@ */ import {compileClassMetadata, compileComponentFromMetadata, compileDeclareClassMetadata, compileDeclareComponentFromMetadata, ConstantPool, CssSelector, DeclarationListEmitMode, DeclareComponentTemplateInfo, DEFAULT_INTERPOLATION_CONFIG, DomElementSchemaRegistry, Expression, ExternalExpr, FactoryTarget, InterpolationConfig, LexerRange, makeBindingParser, ParsedTemplate, ParseSourceFile, parseTemplate, R3ClassMetadata, R3ComponentMetadata, R3TargetBinder, R3UsedDirectiveMetadata, SelectorMatcher, Statement, TmplAstNode, WrappedNodeExpr} from '@angular/compiler'; +import {ViewEncapsulation} from '@angular/compiler/src/core'; import * as ts from 'typescript'; import {Cycle, CycleAnalyzer, CycleHandlingStrategy} from '../../cycles'; -import {ErrorCode, FatalDiagnosticError, makeRelatedInformation} from '../../diagnostics'; +import {ErrorCode, FatalDiagnosticError, makeDiagnostic, makeRelatedInformation} from '../../diagnostics'; import {absoluteFrom, relative} from '../../file_system'; import {ImportedFile, ModuleResolver, Reference, ReferenceEmitter} from '../../imports'; import {DependencyTracker} from '../../incremental/api'; @@ -338,6 +339,16 @@ export class ComponentDecoratorHandler implements // Next, read the `@Component`-specific fields. const {decorator: component, metadata, inputs, outputs} = directiveResult; + const encapsulation: number = + this._resolveEnumValue(component, 'encapsulation', 'ViewEncapsulation') ?? + ViewEncapsulation.Emulated; + const changeDetection: number|null = + this._resolveEnumValue(component, 'changeDetection', 'ChangeDetectionStrategy'); + + let animations: Expression|null = null; + if (component.has('animations')) { + animations = new WrappedNodeExpr(component.get('animations')!); + } // Go through the root directories for this project, and select the one with the smallest // relative path representation. @@ -426,6 +437,18 @@ export class ComponentDecoratorHandler implements } } + if (encapsulation === ViewEncapsulation.ShadowDom && metadata.selector !== null) { + const selectorError = checkCustomElementSelectorForErrors(metadata.selector); + if (selectorError !== null) { + if (diagnostics === undefined) { + diagnostics = []; + } + diagnostics.push(makeDiagnostic( + ErrorCode.COMPONENT_INVALID_SHADOW_DOM_SELECTOR, component.get('selector')!, + selectorError)); + } + } + // If inline styles were preprocessed use those let inlineStyles: string[]|null = null; if (this.preanalyzeStylesCache.has(node)) { @@ -455,17 +478,6 @@ export class ComponentDecoratorHandler implements styles.push(...template.styles); } - const encapsulation: number = - this._resolveEnumValue(component, 'encapsulation', 'ViewEncapsulation') || 0; - - const changeDetection: number|null = - this._resolveEnumValue(component, 'changeDetection', 'ChangeDetectionStrategy'); - - let animations: Expression|null = null; - if (component.has('animations')) { - animations = new WrappedNodeExpr(component.get('animations')!); - } - const output: AnalysisOutput = { analysis: { baseClass: readBaseClass(node, this.reflector, this.evaluator), @@ -1431,3 +1443,31 @@ function makeCyclicImportInfo( `The ${type} '${name}' is used in the template but importing it would create a cycle: `; return makeRelatedInformation(ref.node, message + path); } + + +/** + * Checks whether a selector is a valid custom element tag name. + * Based loosely on https://github.com/sindresorhus/validate-element-name. + */ +function checkCustomElementSelectorForErrors(selector: string): string|null { + // Avoid flagging components with an attribute or class selector. This isn't bulletproof since it + // won't catch cases like `foo[]bar`, but we don't need it to be. This is mainly to avoid flagging + // something like `foo-bar[baz]` incorrectly. + if (selector.includes('.') || (selector.includes('[') && selector.includes(']'))) { + return null; + } + + if (!(/^[a-z]/.test(selector))) { + return 'Selector of a ShadowDom-encapsulated component must start with a lower case letter.'; + } + + if (/[A-Z]/.test(selector)) { + return 'Selector of a ShadowDom-encapsulated component must all be in lower case.'; + } + + if (!selector.includes('-')) { + return 'Selector of a component that uses ViewEncapsulation.ShadowDom must contain a hyphen.'; + } + + return null; +} diff --git a/packages/compiler-cli/src/ngtsc/diagnostics/src/error_code.ts b/packages/compiler-cli/src/ngtsc/diagnostics/src/error_code.ts index 63f8b01f2b..a0dc5e3f5a 100644 --- a/packages/compiler-cli/src/ngtsc/diagnostics/src/error_code.ts +++ b/packages/compiler-cli/src/ngtsc/diagnostics/src/error_code.ts @@ -50,6 +50,12 @@ export enum ErrorCode { */ COMPONENT_RESOURCE_NOT_FOUND = 2008, + /** + * Raised when a component uses `ShadowDom` view encapsulation, but its selector + * does not match the shadow DOM tag name requirements. + */ + COMPONENT_INVALID_SHADOW_DOM_SELECTOR = 2009, + SYMBOL_NOT_EXPORTED = 3001, SYMBOL_EXPORTED_UNDER_DIFFERENT_NAME = 3002, /** @@ -215,6 +221,7 @@ export const COMPILER_ERRORS_WITH_GUIDES = new Set([ ErrorCode.SCHEMA_INVALID_ELEMENT, ErrorCode.SCHEMA_INVALID_ATTRIBUTE, ErrorCode.MISSING_REFERENCE_TARGET, + ErrorCode.COMPONENT_INVALID_SHADOW_DOM_SELECTOR, ]); /** diff --git a/packages/compiler-cli/test/ngtsc/ngtsc_spec.ts b/packages/compiler-cli/test/ngtsc/ngtsc_spec.ts index e6509e141e..1b179a25c6 100644 --- a/packages/compiler-cli/test/ngtsc/ngtsc_spec.ts +++ b/packages/compiler-cli/test/ngtsc/ngtsc_spec.ts @@ -7864,6 +7864,111 @@ export const Foo = Foo__PRE_R3__; }); }); + describe('shadow DOM selector diagnostics', () => { + it('should emit a diagnostic when a selector does not include a hyphen', () => { + env.write('test.ts', ` + import {Component, ViewEncapsulation} from '@angular/core'; + @Component({ + template: '', + selector: 'cmp', + encapsulation: ViewEncapsulation.ShadowDom + }) + export class TestCmp {} + `); + const diags = env.driveDiagnostics(); + + expect(diags.length).toBe(1); + expect(diags[0].messageText) + .toBe( + 'Selector of a component that uses ViewEncapsulation.ShadowDom must contain a hyphen.'); + expect(getDiagnosticSourceCode(diags[0])).toBe(`'cmp'`); + }); + + it('should emit a diagnostic when a selector includes uppercase letters', () => { + env.write('test.ts', ` + import {Component, ViewEncapsulation} from '@angular/core'; + @Component({ + template: '', + selector: 'my-Comp', + encapsulation: ViewEncapsulation.ShadowDom + }) + export class TestCmp {} + `); + const diags = env.driveDiagnostics(); + + expect(diags.length).toBe(1); + expect(diags[0].messageText) + .toBe('Selector of a ShadowDom-encapsulated component must all be in lower case.'); + expect(getDiagnosticSourceCode(diags[0])).toBe(`'my-Comp'`); + }); + + it('should emit a diagnostic when a selector starts with a digit', () => { + env.write('test.ts', ` + import {Component, ViewEncapsulation} from '@angular/core'; + @Component({ + template: '', + selector: '123-comp', + encapsulation: ViewEncapsulation.ShadowDom + }) + export class TestCmp {} + `); + const diags = env.driveDiagnostics(); + + expect(diags.length).toBe(1); + expect(diags[0].messageText) + .toBe( + 'Selector of a ShadowDom-encapsulated component must start with a lower case letter.'); + expect(getDiagnosticSourceCode(diags[0])).toBe(`'123-comp'`); + }); + + it('should emit a diagnostic when a selector starts with a hyphen', () => { + env.write('test.ts', ` + import {Component, ViewEncapsulation} from '@angular/core'; + @Component({ + template: '', + selector: '-comp', + encapsulation: ViewEncapsulation.ShadowDom + }) + export class TestCmp {} + `); + const diags = env.driveDiagnostics(); + + expect(diags.length).toBe(1); + expect(diags[0].messageText) + .toBe( + 'Selector of a ShadowDom-encapsulated component must start with a lower case letter.'); + expect(getDiagnosticSourceCode(diags[0])).toBe(`'-comp'`); + }); + + it('should not emit a diagnostic for a component using an attribute selector', () => { + env.write('test.ts', ` + import {Component, ViewEncapsulation} from '@angular/core'; + @Component({ + template: '', + selector: '[button]', + encapsulation: ViewEncapsulation.ShadowDom + }) + export class TestCmp {} + `); + const diags = env.driveDiagnostics(); + expect(diags.length).toBe(0); + }); + + it('should not emit a diagnostic for a component using a class selector', () => { + env.write('test.ts', ` + import {Component, ViewEncapsulation} from '@angular/core'; + @Component({ + template: '', + selector: '.button', + encapsulation: ViewEncapsulation.ShadowDom + }) + export class TestCmp {} + `); + const diags = env.driveDiagnostics(); + expect(diags.length).toBe(0); + }); + }); + describe('i18n errors', () => { it('reports a diagnostics on nested i18n sections', () => { env.write('test.ts', `