From 50f8928d56bdbc1e65dfd5127ec00d8d934d2432 Mon Sep 17 00:00:00 2001 From: Kristiyan Kostadinov Date: Fri, 30 Sep 2022 09:24:34 +0200 Subject: [PATCH] refactor(core): add host directive definitions validation (#47589) Adds some logic to ensure that host directives are configured correctly. PR Close #47589 --- goldens/public-api/core/errors.md | 12 + packages/core/src/errors.ts | 6 + .../features/host_directives_feature.ts | 86 ++++- .../core/src/render3/instructions/shared.ts | 4 +- .../test/acceptance/host_directives_spec.ts | 361 ++++++++++++++++++ 5 files changed, 464 insertions(+), 5 deletions(-) diff --git a/goldens/public-api/core/errors.md b/goldens/public-api/core/errors.md index 5fc14081538..8d8790c4064 100644 --- a/goldens/public-api/core/errors.md +++ b/goldens/public-api/core/errors.md @@ -25,12 +25,24 @@ export const enum RuntimeErrorCode { // (undocumented) CYCLIC_DI_DEPENDENCY = -200, // (undocumented) + DUPLICATE_DIRECTITVE = 309, + // (undocumented) ERROR_HANDLER_NOT_FOUND = 402, // (undocumented) EXPORT_NOT_FOUND = -301, // (undocumented) EXPRESSION_CHANGED_AFTER_CHECKED = -100, // (undocumented) + HOST_DIRECTIVE_COMPONENT = 310, + // (undocumented) + HOST_DIRECTIVE_CONFLICTING_ALIAS = 312, + // (undocumented) + HOST_DIRECTIVE_NOT_STANDALONE = 308, + // (undocumented) + HOST_DIRECTIVE_UNDEFINED_BINDING = 311, + // (undocumented) + HOST_DIRECTIVE_UNRESOLVABLE = 307, + // (undocumented) IMPORT_PROVIDERS_FROM_STANDALONE = 800, // (undocumented) INJECTOR_ALREADY_DESTROYED = 205, diff --git a/packages/core/src/errors.ts b/packages/core/src/errors.ts index d517878135d..ff82ecbb87d 100644 --- a/packages/core/src/errors.ts +++ b/packages/core/src/errors.ts @@ -42,6 +42,12 @@ export const enum RuntimeErrorCode { UNKNOWN_ELEMENT = 304, TEMPLATE_STRUCTURE_ERROR = 305, INVALID_EVENT_BINDING = 306, + HOST_DIRECTIVE_UNRESOLVABLE = 307, + HOST_DIRECTIVE_NOT_STANDALONE = 308, + DUPLICATE_DIRECTITVE = 309, + HOST_DIRECTIVE_COMPONENT = 310, + HOST_DIRECTIVE_UNDEFINED_BINDING = 311, + HOST_DIRECTIVE_CONFLICTING_ALIAS = 312, // Bootstrap Errors MULTIPLE_PLATFORMS = 400, diff --git a/packages/core/src/render3/features/host_directives_feature.ts b/packages/core/src/render3/features/host_directives_feature.ts index 8bbadf9dc9c..9314a228fc7 100644 --- a/packages/core/src/render3/features/host_directives_feature.ts +++ b/packages/core/src/render3/features/host_directives_feature.ts @@ -6,10 +6,11 @@ * found in the LICENSE file at https://angular.io/license */ import {resolveForwardRef} from '../../di'; +import {RuntimeError, RuntimeErrorCode} from '../../errors'; import {Type} from '../../interface/type'; import {EMPTY_OBJ} from '../../util/empty'; -import {getDirectiveDef} from '../definition'; -import {DirectiveDef, HostDirectiveBindingMap, HostDirectiveDefs} from '../interfaces/definition'; +import {getComponentDef, getDirectiveDef} from '../definition'; +import {DirectiveDef, HostDirectiveBindingMap, HostDirectiveDef, HostDirectiveDefs} from '../interfaces/definition'; /** Values that can be used to define a host directive through the `HostDirectivesFeature`. */ type HostDirectiveConfig = Type|{ @@ -62,7 +63,9 @@ function findHostDirectiveDefs( for (const hostDirectiveConfig of currentDef.hostDirectives) { const hostDirectiveDef = getDirectiveDef(hostDirectiveConfig.directive)!; - // TODO(crisbeto): assert that the def exists. + if (typeof ngDevMode === 'undefined' || ngDevMode) { + validateHostDirective(hostDirectiveConfig, hostDirectiveDef, matchedDefs); + } // Host directives execute before the host so that its host bindings can be overwritten. findHostDirectiveDefs(hostDirectiveDef, matchedDefs, hostDirectiveDefs); @@ -89,3 +92,80 @@ function bindingArrayToMap(bindings: string[]|undefined): HostDirectiveBindingMa return result; } + +/** + * Verifies that the host directive has been configured correctly. + * @param hostDirectiveConfig Host directive configuration object. + * @param directiveDef Directive definition of the host directive. + * @param matchedDefs Directives that have been matched so far. + */ +function validateHostDirective( + hostDirectiveConfig: HostDirectiveDef, directiveDef: DirectiveDef|null, + matchedDefs: DirectiveDef[]): asserts directiveDef is DirectiveDef { + // TODO(crisbeto): implement more of these checks in the compiler. + const type = hostDirectiveConfig.directive; + + if (directiveDef === null) { + if (getComponentDef(type) !== null) { + throw new RuntimeError( + RuntimeErrorCode.HOST_DIRECTIVE_COMPONENT, + `Host directive ${type.name} cannot be a component.`); + } + + throw new RuntimeError( + RuntimeErrorCode.HOST_DIRECTIVE_UNRESOLVABLE, + `Could not resolve metadata for host directive ${type.name}. ` + + `Make sure that the ${type.name} class is annotated with an @Directive decorator.`); + } + + if (!directiveDef.standalone) { + throw new RuntimeError( + RuntimeErrorCode.HOST_DIRECTIVE_NOT_STANDALONE, + `Host directive ${directiveDef.type.name} must be standalone.`); + } + + if (matchedDefs.indexOf(directiveDef) > -1) { + throw new RuntimeError( + RuntimeErrorCode.DUPLICATE_DIRECTITVE, + `Directive ${directiveDef.type.name} matches multiple times on the same element. ` + + `Directives can only match an element once.`); + } + + validateMappings('input', directiveDef, hostDirectiveConfig.inputs); + validateMappings('output', directiveDef, hostDirectiveConfig.outputs); +} + +/** + * Checks that the host directive inputs/outputs configuration is valid. + * @param bindingType Kind of binding that is being validated. Used in the error message. + * @param def Definition of the host directive that is being validated against. + * @param hostDirectiveDefs Host directive mapping object that shold be validated. + */ +function validateMappings( + bindingType: 'input'|'output', def: DirectiveDef, + hostDirectiveDefs: HostDirectiveBindingMap) { + const className = def.type.name; + const bindings: Record = bindingType === 'input' ? def.inputs : def.outputs; + + for (const publicName in hostDirectiveDefs) { + if (hostDirectiveDefs.hasOwnProperty(publicName)) { + if (!bindings.hasOwnProperty(publicName)) { + throw new RuntimeError( + RuntimeErrorCode.HOST_DIRECTIVE_UNDEFINED_BINDING, + `Directive ${className} does not have an ${bindingType} with a public name of ${ + publicName}.`); + } + + const remappedPublicName = hostDirectiveDefs[publicName]; + + if (bindings.hasOwnProperty(remappedPublicName) && + bindings[remappedPublicName] !== publicName) { + throw new RuntimeError( + RuntimeErrorCode.HOST_DIRECTIVE_CONFLICTING_ALIAS, + `Cannot alias ${bindingType} ${publicName} of host directive ${className} to ${ + remappedPublicName}, because it already has a different ${ + bindingType} with the same public name.`); + } + } + } +} diff --git a/packages/core/src/render3/instructions/shared.ts b/packages/core/src/render3/instructions/shared.ts index a96d0424d03..957d1da66b7 100644 --- a/packages/core/src/render3/instructions/shared.ts +++ b/packages/core/src/render3/instructions/shared.ts @@ -25,7 +25,7 @@ import {diPublicInInjector, getNodeInjectable, getOrCreateNodeInjectorForNode} f import {throwMultipleComponentError} from '../errors'; import {executeCheckHooks, executeInitAndCheckHooks, incrementInitPhaseFlags} from '../hooks'; import {CONTAINER_HEADER_OFFSET, HAS_TRANSPLANTED_VIEWS, LContainer, MOVED_VIEWS} from '../interfaces/container'; -import {ComponentDef, ComponentTemplate, DirectiveDef, DirectiveDefListOrFactory, HostBindingsFunction, HostDirectiveDefs, PipeDefListOrFactory, RenderFlags, ViewQueriesFunction} from '../interfaces/definition'; +import {ComponentDef, ComponentTemplate, DirectiveDef, DirectiveDefListOrFactory, HostBindingsFunction, HostDirectiveBindingMap, HostDirectiveDefs, PipeDefListOrFactory, RenderFlags, ViewQueriesFunction} from '../interfaces/definition'; import {NodeInjectorFactory} from '../interfaces/injector'; import {getUniqueLViewId} from '../interfaces/lview_tracking'; import {AttributeMarker, InitialInputData, InitialInputs, LocalRefExtractor, PropertyAliases, PropertyAliasValue, TAttributes, TConstantsOrFactory, TContainerNode, TDirectiveHostNode, TElementContainerNode, TElementNode, TIcuContainerNode, TNode, TNodeFlags, TNodeType, TProjectionNode} from '../interfaces/node'; @@ -873,7 +873,7 @@ export function createTNode( function generatePropertyAliases( aliasMap: {[publicName: string]: string}, directiveIndex: number, propertyAliases: PropertyAliases|null, - hostDirectiveAliasMap: {[internalName: string]: string}|null): PropertyAliases|null { + hostDirectiveAliasMap: HostDirectiveBindingMap|null): PropertyAliases|null { for (let publicName in aliasMap) { if (aliasMap.hasOwnProperty(publicName)) { propertyAliases = propertyAliases === null ? {} : propertyAliases; diff --git a/packages/core/test/acceptance/host_directives_spec.ts b/packages/core/test/acceptance/host_directives_spec.ts index 934510f7260..a9f2fbaff24 100644 --- a/packages/core/test/acceptance/host_directives_spec.ts +++ b/packages/core/test/acceptance/host_directives_spec.ts @@ -1884,4 +1884,365 @@ describe('host directives', () => { expect(result).toBe(expected); }); }); + + describe('invalid usage validations', () => { + it('should throw an error if the metadata of a host directive cannot be resolved', () => { + class HostDir {} + + @Directive({selector: '[dir]', hostDirectives: [HostDir]} as HostDirectiveAny) + class Dir { + } + + @Component({template: '
'}) + class App { + } + + TestBed.configureTestingModule({declarations: [App, Dir]}); + + expect(() => TestBed.createComponent(App)) + .toThrowError( + 'NG0307: Could not resolve metadata for host directive HostDir. ' + + 'Make sure that the HostDir class is annotated with an @Directive decorator.'); + }); + + it('should throw an error if a host directive is not standalone', () => { + @Directive({standalone: false}) + class HostDir { + } + + @Directive({selector: '[dir]', hostDirectives: [HostDir]} as HostDirectiveAny) + class Dir { + } + + @Component({template: '
'}) + class App { + } + + TestBed.configureTestingModule({declarations: [App, Dir]}); + + expect(() => TestBed.createComponent(App)) + .toThrowError('NG0308: Host directive HostDir must be standalone.'); + }); + + it('should throw an error if a host directive matches multiple times in a template', () => { + @Directive({standalone: true, selector: '[dir]'}) + class HostDir { + } + + @Directive({ + selector: '[dir]', + hostDirectives: [HostDir], + standalone: true, + } as HostDirectiveAny) + class Dir { + } + + @Component({template: '
', standalone: true, imports: [HostDir, Dir]}) + class App { + } + + expect(() => TestBed.createComponent(App)) + .toThrowError( + 'NG0309: Directive HostDir matches multiple times on the same element. Directives can only match an element once.'); + }); + + it('should throw an error if a host directive appears multiple times in a chain', () => { + @Directive({standalone: true}) + class DuplicateHostDir { + } + + @Directive({standalone: true, hostDirectives: [DuplicateHostDir]} as HostDirectiveAny) + class HostDir { + } + + @Directive({ + selector: '[dir]', + hostDirectives: [HostDir, DuplicateHostDir], + } as HostDirectiveAny) + class Dir { + } + + @Component({template: '
'}) + class App { + } + + TestBed.configureTestingModule({declarations: [App, Dir]}); + + expect(() => TestBed.createComponent(App)) + .toThrowError( + 'NG0309: Directive DuplicateHostDir matches multiple times on the same element. Directives can only match an element once.'); + }); + + it('should throw an error if a host directive is a component', () => { + @Component({standalone: true, template: '', selector: 'host-comp'}) + class HostComp { + } + + @Directive({selector: '[dir]', hostDirectives: [HostComp]} as HostDirectiveAny) + class Dir { + } + + @Component({template: '
'}) + class App { + } + + TestBed.configureTestingModule({declarations: [App, Dir]}); + + expect(() => TestBed.createComponent(App)) + .toThrowError('NG0310: Host directive HostComp cannot be a component.'); + }); + + it('should throw an error if a host directive output does not exist', () => { + @Directive({standalone: true}) + class HostDir { + @Output() foo = new EventEmitter(); + } + + @Directive({ + selector: '[dir]', + hostDirectives: [{ + directive: HostDir, + outputs: ['doesNotExist'], + }] + } as HostDirectiveAny) + class Dir { + } + + @Component({template: '
'}) + class App { + } + + TestBed.configureTestingModule({declarations: [App, Dir]}); + + expect(() => TestBed.createComponent(App)) + .toThrowError( + 'NG0311: Directive HostDir does not have an output with a public name of doesNotExist.'); + }); + + it('should throw an error if a host directive output alias does not exist', () => { + @Directive({standalone: true}) + class HostDir { + @Output('alias') foo = new EventEmitter(); + } + + @Directive({ + selector: '[dir]', + hostDirectives: [{ + directive: HostDir, + outputs: ['foo'], + }] + } as HostDirectiveAny) + class Dir { + } + + @Component({template: '
'}) + class App { + } + + TestBed.configureTestingModule({declarations: [App, Dir]}); + + expect(() => TestBed.createComponent(App)) + .toThrowError( + 'NG0311: Directive HostDir does not have an output with a public name of foo.'); + }); + + it('should throw an error if a host directive input does not exist', () => { + @Directive({standalone: true}) + class HostDir { + @Input() foo: any; + } + + @Directive({ + selector: '[dir]', + hostDirectives: [{ + directive: HostDir, + inputs: ['doesNotExist'], + }] + } as HostDirectiveAny) + class Dir { + } + + @Component({template: '
'}) + class App { + } + + TestBed.configureTestingModule({declarations: [App, Dir]}); + + expect(() => TestBed.createComponent(App)) + .toThrowError( + 'NG0311: Directive HostDir does not have an input with a public name of doesNotExist.'); + }); + + it('should throw an error if a host directive input alias does not exist', () => { + @Directive({standalone: true}) + class HostDir { + @Input('alias') foo: any; + } + + @Directive({ + selector: '[dir]', + hostDirectives: [{directive: HostDir, inputs: ['foo']}], + } as HostDirectiveAny) + class Dir { + } + + @Component({template: '
'}) + class App { + } + + TestBed.configureTestingModule({declarations: [App, Dir]}); + + expect(() => TestBed.createComponent(App)) + .toThrowError( + 'NG0311: Directive HostDir does not have an input with a public name of foo.'); + }); + + it('should throw an error if a host directive tries to alias to an existing input', () => { + @Directive({selector: '[host-dir]', standalone: true}) + class HostDir { + @Input('colorAlias') color?: string; + @Input() buttonColor?: string; + } + + @Directive({ + selector: '[dir]', + hostDirectives: [{directive: HostDir, inputs: ['colorAlias: buttonColor']}] + } as HostDirectiveAny) + class Dir { + } + + @Component({imports: [Dir, HostDir], template: ''}) + class App { + } + + TestBed.configureTestingModule({declarations: [App, Dir]}); + + expect(() => { + const fixture = TestBed.createComponent(App); + fixture.detectChanges(); + }) + .toThrowError( + 'NG0312: Cannot alias input colorAlias of host directive HostDir ' + + 'to buttonColor, because it already has a different input with the same public name.'); + }); + + it('should throw an error if a host directive tries to alias to an existing input alias', () => { + @Directive({selector: '[host-dir]', standalone: true}) + class HostDir { + @Input('colorAlias') color?: string; + @Input('buttonColorAlias') buttonColor?: string; + } + + @Directive({ + selector: '[dir]', + hostDirectives: [{directive: HostDir, inputs: ['colorAlias: buttonColorAlias']}] + } as HostDirectiveAny) + class Dir { + } + + @Component({ + imports: [Dir, HostDir], + template: '', + }) + class App { + } + + TestBed.configureTestingModule({declarations: [App, Dir]}); + + expect(() => { + const fixture = TestBed.createComponent(App); + fixture.detectChanges(); + }) + .toThrowError( + 'NG0312: Cannot alias input colorAlias of host directive HostDir ' + + 'to buttonColorAlias, because it already has a different input with the same public name.'); + }); + + it('should not throw if a host directive input aliases to the same name', () => { + @Directive({selector: '[host-dir]', standalone: true}) + class HostDir { + @Input('color') color?: string; + } + + @Directive({ + selector: '[dir]', + hostDirectives: [{directive: HostDir, inputs: ['color: buttonColor']}] + } as HostDirectiveAny) + class Dir { + } + + @Component({imports: [Dir, HostDir], template: ''}) + class App { + } + + TestBed.configureTestingModule({declarations: [App, Dir]}); + + expect(() => { + const fixture = TestBed.createComponent(App); + fixture.detectChanges(); + }).not.toThrow(); + }); + + it('should throw an error if a host directive tries to alias to an existing output alias', () => { + @Directive({selector: '[host-dir]', standalone: true}) + class HostDir { + @Output('clickedAlias') clicked = new EventEmitter(); + @Output('tappedAlias') tapped = new EventEmitter(); + } + + @Directive({ + selector: '[dir]', + hostDirectives: [{directive: HostDir, outputs: ['clickedAlias: tappedAlias']}] + } as HostDirectiveAny) + class Dir { + } + + @Component({ + imports: [Dir, HostDir], + template: '', + }) + class App { + handleTap() {} + } + + TestBed.configureTestingModule({declarations: [App, Dir]}); + + expect(() => { + const fixture = TestBed.createComponent(App); + fixture.detectChanges(); + }) + .toThrowError( + 'NG0312: Cannot alias output clickedAlias of host directive HostDir ' + + 'to tappedAlias, because it already has a different output with the same public name.'); + }); + + it('should not throw if a host directive output aliases to the same name', () => { + @Directive({selector: '[host-dir]', standalone: true}) + class HostDir { + @Output('clicked') clicked = new EventEmitter(); + } + + @Directive({ + selector: '[dir]', + hostDirectives: [{directive: HostDir, outputs: ['clicked: wasClicked']}] + } as HostDirectiveAny) + class Dir { + } + + @Component({ + imports: [Dir, HostDir], + template: '', + }) + class App { + handleClick() {} + } + + TestBed.configureTestingModule({declarations: [App, Dir]}); + + expect(() => { + const fixture = TestBed.createComponent(App); + fixture.detectChanges(); + }).not.toThrow(); + }); + }); });