Skip to content

Commit

Permalink
fix(core): host directive validation not picking up duplicate directi…
Browse files Browse the repository at this point in the history
…ves on component node (#52073)

Fixes that, depending on the matching and import order, in some cases we weren't throwing the error saying that a directive matched multiple times on the same element.

Fixes #52072.

PR Close #52073
  • Loading branch information
crisbeto authored and atscott committed Oct 9, 2023
1 parent acac9ea commit 7368b8a
Show file tree
Hide file tree
Showing 5 changed files with 65 additions and 13 deletions.
19 changes: 19 additions & 0 deletions packages/core/src/render3/assert.ts
Expand Up @@ -6,6 +6,7 @@
* found in the LICENSE file at https://angular.io/license
*/

import {RuntimeError, RuntimeErrorCode} from '../errors';
import {assertDefined, assertEqual, assertNumber, throwError} from '../util/assert';

import {getComponentDef, getNgModuleDef} from './definition';
Expand Down Expand Up @@ -140,6 +141,24 @@ export function assertParentView(lView: LView|null, errMessage?: string) {
errMessage || 'Component views should always have a parent view (component\'s host view)');
}

export function assertNoDuplicateDirectives(directives: DirectiveDef<unknown>[]): void {
// The array needs at least two elements in order to have duplicates.
if (directives.length < 2) {
return;
}

const seenDirectives = new Set<DirectiveDef<unknown>>();

for (const current of directives) {
if (seenDirectives.has(current)) {
throw new RuntimeError(
RuntimeErrorCode.DUPLICATE_DIRECTITVE,
`Directive ${current.type.name} matches multiple times on the same element. ` +
`Directives can only match an element once.`);
}
seenDirectives.add(current);
}
}

/**
* This is a basic sanity check that the `injectorIndex` seems to point to what looks like a
Expand Down
3 changes: 2 additions & 1 deletion packages/core/src/render3/component_ref.ts
Expand Up @@ -27,7 +27,7 @@ import {VERSION} from '../version';
import {NOT_FOUND_CHECK_ONLY_ELEMENT_INJECTOR} from '../view/provider_flags';

import {AfterRenderEventManager} from './after_render_hooks';
import {assertComponentType} from './assert';
import {assertComponentType, assertNoDuplicateDirectives} from './assert';
import {attachPatchData} from './context_discovery';
import {getComponentDef} from './definition';
import {getNodeInjectable, NodeInjector} from './di';
Expand Down Expand Up @@ -246,6 +246,7 @@ export class ComponentFactory<T> extends AbstractComponentFactory<T> {
hostDirectiveDefs = new Map();
rootComponentDef.findHostDirectiveDefs(rootComponentDef, rootDirectives, hostDirectiveDefs);
rootDirectives.push(rootComponentDef);
ngDevMode && assertNoDuplicateDirectives(rootDirectives);
} else {
rootDirectives = [rootComponentDef];
}
Expand Down
14 changes: 3 additions & 11 deletions packages/core/src/render3/features/host_directives_feature.ts
Expand Up @@ -65,7 +65,7 @@ function findHostDirectiveDefs(
const hostDirectiveDef = getDirectiveDef(hostDirectiveConfig.directive)!;

if (typeof ngDevMode === 'undefined' || ngDevMode) {
validateHostDirective(hostDirectiveConfig, hostDirectiveDef, matchedDefs);
validateHostDirective(hostDirectiveConfig, hostDirectiveDef);
}

// We need to patch the `declaredInputs` so that
Expand Down Expand Up @@ -144,11 +144,10 @@ function patchDeclaredInputs(
* 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<unknown>, directiveDef: DirectiveDef<any>|null,
matchedDefs: DirectiveDef<unknown>[]): asserts directiveDef is DirectiveDef<unknown> {
hostDirectiveConfig: HostDirectiveDef<unknown>,
directiveDef: DirectiveDef<any>|null): asserts directiveDef is DirectiveDef<unknown> {
const type = hostDirectiveConfig.directive;

if (directiveDef === null) {
Expand All @@ -170,13 +169,6 @@ function validateHostDirective(
`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);
}
Expand Down
3 changes: 2 additions & 1 deletion packages/core/src/render3/instructions/shared.ts
Expand Up @@ -24,7 +24,7 @@ import {assertDefined, assertEqual, assertGreaterThan, assertGreaterThanOrEqual,
import {escapeCommentText} from '../../util/dom';
import {normalizeDebugBindingName, normalizeDebugBindingValue} from '../../util/ng_reflect';
import {stringify} from '../../util/stringify';
import {assertFirstCreatePass, assertFirstUpdatePass, assertLView, assertTNodeForLView, assertTNodeForTView} from '../assert';
import {assertFirstCreatePass, assertFirstUpdatePass, assertLView, assertNoDuplicateDirectives, assertTNodeForLView, assertTNodeForTView} from '../assert';
import {attachPatchData} from '../context_discovery';
import {getFactoryDef} from '../definition_factory';
import {diPublicInInjector, getNodeInjectable, getOrCreateNodeInjectorForNode} from '../di';
Expand Down Expand Up @@ -1131,6 +1131,7 @@ function findDirectiveDefMatches(
}
}
}
ngDevMode && matches !== null && assertNoDuplicateDirectives(matches);
return matches === null ? null : [matches, hostDirectiveDefs];
}

Expand Down
39 changes: 39 additions & 0 deletions packages/core/test/acceptance/host_directives_spec.ts
Expand Up @@ -2921,6 +2921,45 @@ describe('host directives', () => {
'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 matches multiple times on a component', () => {
@Directive({standalone: true, selector: '[dir]'})
class HostDir {
}

@Component({
selector: 'comp',
hostDirectives: [HostDir],
standalone: true,
template: '',
})
class Comp {
}

const baseAppMetadata = {
template: '<comp dir></comp>',
standalone: true,
};

const expectedError =
'NG0309: Directive HostDir matches multiple times on the same element. Directives can only match an element once.';

// Note: the definition order in `imports` seems to affect the
// directive matching order so we test both scenarios.
expect(() => {
@Component({...baseAppMetadata, imports: [Comp, HostDir]})
class App {
}
TestBed.createComponent(App);
}).toThrowError(expectedError);

expect(() => {
@Component({...baseAppMetadata, imports: [HostDir, Comp]})
class App {
}
TestBed.createComponent(App);
}).toThrowError(expectedError);
});

it('should throw an error if a host directive appears multiple times in a chain', () => {
@Directive({standalone: true})
class DuplicateHostDir {
Expand Down

0 comments on commit 7368b8a

Please sign in to comment.