Skip to content

[Patch port] fix(core): resolve forward-referenced host directives during directive matching #58500

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Closed
wants to merge 1 commit into from
Closed

[Patch port] fix(core): resolve forward-referenced host directives during directive matching #58500

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
87 changes: 50 additions & 37 deletions packages/core/src/render3/features/host_directives_feature.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -7,27 +7,18 @@
*/
import {resolveForwardRef} from '../../di';
import {RuntimeError, RuntimeErrorCode} from '../../errors';
import {Type} from '../../interface/type';
import {assertEqual} from '../../util/assert';
import {EMPTY_OBJ} from '../../util/empty';
import {getComponentDef, getDirectiveDef} from '../definition';
import {
import type {
DirectiveDef,
DirectiveDefFeature,
HostDirectiveBindingMap,
HostDirectiveConfig,
HostDirectiveDef,
HostDirectiveDefs,
} from '../interfaces/definition';

/** Values that can be used to define a host directive through the `HostDirectivesFeature`. */
type HostDirectiveConfig =
| Type<unknown>
| {
directive: Type<unknown>;
inputs?: string[];
outputs?: string[];
};

/**
* This feature adds the host directives behavior to a directive definition by patching a
* function onto it. The expectation is that the runtime will invoke the function during
Expand All @@ -52,22 +43,17 @@ export function ɵɵHostDirectivesFeature(
rawHostDirectives: HostDirectiveConfig[] | (() => HostDirectiveConfig[]),
) {
const feature: DirectiveDefFeature = (definition: DirectiveDef<unknown>) => {
const resolved = (
Array.isArray(rawHostDirectives) ? rawHostDirectives : rawHostDirectives()
).map((dir) => {
return typeof dir === 'function'
? {directive: resolveForwardRef(dir), inputs: EMPTY_OBJ, outputs: EMPTY_OBJ}
: {
directive: resolveForwardRef(dir.directive),
inputs: bindingArrayToMap(dir.inputs),
outputs: bindingArrayToMap(dir.outputs),
};
});
const isEager = Array.isArray(rawHostDirectives);

if (definition.hostDirectives === null) {
definition.findHostDirectiveDefs = findHostDirectiveDefs;
definition.hostDirectives = resolved;
definition.hostDirectives = isEager
? rawHostDirectives.map(createHostDirectiveDef)
: [rawHostDirectives];
} else if (isEager) {
definition.hostDirectives.unshift(...rawHostDirectives.map(createHostDirectiveDef));
} else {
definition.hostDirectives.unshift(...resolved);
definition.hostDirectives.unshift(rawHostDirectives);
}
};
feature.ngInherit = true;
Expand All @@ -80,23 +66,50 @@ function findHostDirectiveDefs(
hostDirectiveDefs: HostDirectiveDefs,
): void {
if (currentDef.hostDirectives !== null) {
for (const hostDirectiveConfig of currentDef.hostDirectives) {
const hostDirectiveDef = getDirectiveDef(hostDirectiveConfig.directive)!;

if (typeof ngDevMode === 'undefined' || ngDevMode) {
validateHostDirective(hostDirectiveConfig, hostDirectiveDef);
for (const configOrFn of currentDef.hostDirectives) {
if (typeof configOrFn === 'function') {
const resolved = configOrFn();
for (const config of resolved) {
trackHostDirectiveDef(createHostDirectiveDef(config), matchedDefs, hostDirectiveDefs);
}
} else {
trackHostDirectiveDef(configOrFn, matchedDefs, hostDirectiveDefs);
}
}
}
}

// We need to patch the `declaredInputs` so that
// `ngOnChanges` can map the properties correctly.
patchDeclaredInputs(hostDirectiveDef.declaredInputs, hostDirectiveConfig.inputs);
/** Tracks a single host directive during directive matching. */
function trackHostDirectiveDef(
def: HostDirectiveDef,
matchedDefs: DirectiveDef<unknown>[],
hostDirectiveDefs: HostDirectiveDefs,
) {
const hostDirectiveDef = getDirectiveDef(def.directive)!;

// Host directives execute before the host so that its host bindings can be overwritten.
findHostDirectiveDefs(hostDirectiveDef, matchedDefs, hostDirectiveDefs);
hostDirectiveDefs.set(hostDirectiveDef, hostDirectiveConfig);
matchedDefs.push(hostDirectiveDef);
}
if (typeof ngDevMode === 'undefined' || ngDevMode) {
validateHostDirective(def, hostDirectiveDef);
}

// We need to patch the `declaredInputs` so that
// `ngOnChanges` can map the properties correctly.
patchDeclaredInputs(hostDirectiveDef.declaredInputs, def.inputs);

// Host directives execute before the host so that its host bindings can be overwritten.
findHostDirectiveDefs(hostDirectiveDef, matchedDefs, hostDirectiveDefs);
hostDirectiveDefs.set(hostDirectiveDef, def);
matchedDefs.push(hostDirectiveDef);
}

/** Creates a `HostDirectiveDef` from a used-defined host directive configuration. */
function createHostDirectiveDef(config: HostDirectiveConfig): HostDirectiveDef {
return typeof config === 'function'
? {directive: resolveForwardRef(config), inputs: EMPTY_OBJ, outputs: EMPTY_OBJ}
: {
directive: resolveForwardRef(config.directive),
inputs: bindingArrayToMap(config.inputs),
outputs: bindingArrayToMap(config.outputs),
};
}

/**
Expand Down
22 changes: 20 additions & 2 deletions packages/core/src/render3/interfaces/definition.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -260,8 +260,17 @@ export interface DirectiveDef<T> {
) => void)
| null;

/** Additional directives to be applied whenever the directive has been matched. */
hostDirectives: HostDirectiveDef[] | null;
/**
* Additional directives to be applied whenever the directive has been matched.
*
* `HostDirectiveConfig` objects represent a host directive that can be resolved eagerly and were
* already pre-processed when the definition was created. A function needs to be resolved lazily
* during directive matching, because it's a forward reference.
*
* **Note:** we can't `HostDirectiveConfig` in the array, because there's no way to distinguish if
* a function in the array is a `Type` or a `() => HostDirectiveConfig[]`.
*/
hostDirectives: (HostDirectiveDef | (() => HostDirectiveConfig[]))[] | null;

setInput:
| (<U extends T>(
Expand Down Expand Up @@ -493,6 +502,15 @@ export type HostDirectiveBindingMap = {
*/
export type HostDirectiveDefs = Map<DirectiveDef<unknown>, HostDirectiveDef>;

/** Value that can be used to configure a host directive. */
export type HostDirectiveConfig =
| Type<unknown>
| {
directive: Type<unknown>;
inputs?: string[];
outputs?: string[];
};

export interface ComponentDefFeature {
<T>(componentDef: ComponentDef<T>): void;
/**
Expand Down
87 changes: 78 additions & 9 deletions packages/core/test/acceptance/host_directives_spec.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -14,7 +14,6 @@ import {
Directive,
ElementRef,
EventEmitter,
forwardRef,
inject,
Inject,
InjectionToken,
Expand All @@ -26,6 +25,8 @@ import {
Type,
ViewChild,
ViewContainerRef,
ɵɵdefineDirective,
ɵɵHostDirectivesFeature,
} from '@angular/core';
import {TestBed} from '@angular/core/testing';
import {By} from '@angular/platform-browser';
Expand Down Expand Up @@ -74,38 +75,106 @@ describe('host directives', () => {
it('should apply a host directive referenced through a forwardRef', () => {
const logs: string[] = [];

@Directive({
selector: '[dir]',
hostDirectives: [forwardRef(() => HostDir), {directive: forwardRef(() => OtherHostDir)}],
})
// This directive was "compiled" manually, because our tests are JIT-compiled and the JIT
// compiler doesn't produce the callback-based variant of the `ɵɵHostDirectivesFeature`.
// This represents the following metadata:
// @Directive({
// selector: '[dir]',
// hostDirectives: [forwardRef(() => HostDir), {directive: forwardRef(() => OtherHostDir)}],
// standalone: false,
// })
class Dir {
static ɵfac = () => new Dir();
static ɵdir = ɵɵdefineDirective({
type: Dir,
selectors: [['', 'dir', '']],
standalone: false,
features: [ɵɵHostDirectivesFeature(() => [HostDir, {directive: OtherHostDir}])],
});

constructor() {
logs.push('Dir');
}
}

@Directive({standalone: true})
class OtherHostDir {
constructor() {
logs.push('OtherHostDir');
}
}

@Directive({standalone: true})
class HostDir {
constructor() {
logs.push('HostDir');
}
}

@Directive({standalone: true})
@Component({
template: '<div dir></div>',
standalone: false,
})
class App {}

TestBed.configureTestingModule({declarations: [App, Dir]});
const fixture = TestBed.createComponent(App);
fixture.detectChanges();

expect(logs).toEqual(['HostDir', 'OtherHostDir', 'Dir']);
});

it('should apply a directive that references host directives through a forwardRef and is injected by its host directives', () => {
// This directive was "compiled" manually, because our tests are JIT-compiled and the JIT
// compiler doesn't produce the callback-based variant of the `ɵɵHostDirectivesFeature`.
// This represents the following metadata:
// @Directive({
// selector: '[dir]',
// hostDirectives: [forwardRef(() => HostDir), {directive: forwardRef(() => OtherHostDir)}],
// standalone: false,
// host: {'one': 'override', 'two': 'override'}
// })
class Dir {
static ɵfac = () => new Dir();
static ɵdir = ɵɵdefineDirective({
type: Dir,
selectors: [['', 'dir', '']],
standalone: false,
hostAttrs: ['one', 'override', 'two', 'override'],
features: [ɵɵHostDirectivesFeature(() => [HostDir, {directive: OtherHostDir}])],
});
}

@Directive({standalone: true, host: {'one': 'base'}})
class OtherHostDir {
constructor() {
logs.push('OtherHostDir');
inject(Dir);
}
}

@Component({template: '<div dir></div>'})
@Directive({standalone: true, host: {'two': 'base'}})
class HostDir {
constructor() {
inject(Dir);
}
}

@Component({
template: '<div dir></div>',
standalone: false,
})
class App {}

TestBed.configureTestingModule({declarations: [App, Dir]});
const fixture = TestBed.createComponent(App);
fixture.detectChanges();

expect(logs).toEqual(['HostDir', 'OtherHostDir', 'Dir']);
// Note: we can't use the constructor call order here to determine the initialization order,
// because the act of injecting `Dir` will cause it to be created earlier than its host bindings
// will be invoked. Instead we check that the host bindings apply in the right order.
const host = fixture.nativeElement.querySelector('[dir]');
expect(host.getAttribute('one')).toBe('override');
expect(host.getAttribute('two')).toBe('override');
});

it('should apply a chain of host directives', () => {
Expand Down