Skip to content

Commit

Permalink
fix(core): unable to inject ChangeDetectorRef inside host directives (#…
Browse files Browse the repository at this point in the history
…48355)

When injecting the `ChangeDetectorRef` into a node that matches a component, we create a new ref using the component's LView. This breaks down for host directives, because they run before the component's LView has been created.

These changes resolve the issue by creating the LView before creating the node injector for the directives.

Fixes #48249.

PR Close #48355
  • Loading branch information
crisbeto authored and AndrewKushnir committed Dec 7, 2022
1 parent c0d0417 commit e4dcaa5
Show file tree
Hide file tree
Showing 8 changed files with 54 additions and 28 deletions.
21 changes: 12 additions & 9 deletions packages/core/src/render3/instructions/shared.ts
Original file line number Diff line number Diff line change
Expand Up @@ -1142,6 +1142,16 @@ function instantiateAllDirectives(
tView: TView, lView: LView, tNode: TDirectiveHostNode, native: RNode) {
const start = tNode.directiveStart;
const end = tNode.directiveEnd;

// The component view needs to be created before creating the node injector
// since it is used to inject some special symbols like `ChangeDetectorRef`.
if (isComponentHost(tNode)) {
ngDevMode && assertTNodeType(tNode, TNodeType.AnyRNode);
addComponentLogic(
lView, tNode as TElementNode,
tView.data[start + tNode.componentOffset] as ComponentDef<unknown>);
}

if (!tView.firstCreatePass) {
getOrCreateNodeInjectorForNode(tNode, lView);
}
Expand All @@ -1151,23 +1161,16 @@ function instantiateAllDirectives(
const initialInputs = tNode.initialInputs;
for (let i = start; i < end; i++) {
const def = tView.data[i] as DirectiveDef<any>;
const isComponent = isComponentDef(def);

if (isComponent) {
ngDevMode && assertTNodeType(tNode, TNodeType.AnyRNode);
addComponentLogic(lView, tNode as TElementNode, def as ComponentDef<any>);
}

const directive = getNodeInjectable(lView, tView, i, tNode);
attachPatchData(directive, lView);

if (initialInputs !== null) {
setInputsFromAttrs(lView, i - start, directive, def, tNode, initialInputs!);
}

if (isComponent) {
if (isComponentDef(def)) {
const componentView = getComponentLViewByIndex(tNode.index, lView);
componentView[CONTEXT] = directive;
componentView[CONTEXT] = getNodeInjectable(lView, tView, i, tNode);
}
}
}
Expand Down
40 changes: 39 additions & 1 deletion packages/core/test/acceptance/host_directives_spec.ts
Original file line number Diff line number Diff line change
Expand Up @@ -6,7 +6,7 @@
* found in the LICENSE file at https://angular.io/license
*/

import {AfterViewChecked, AfterViewInit, Component, Directive, ElementRef, EventEmitter, forwardRef, inject, Inject, InjectionToken, Input, OnChanges, OnInit, Output, SimpleChanges, Type, ViewChild, ViewContainerRef} from '@angular/core';
import {AfterViewChecked, AfterViewInit, ChangeDetectorRef, Component, Directive, ElementRef, EventEmitter, forwardRef, inject, Inject, InjectionToken, Input, OnChanges, OnInit, Output, SimpleChanges, Type, ViewChild, ViewContainerRef} from '@angular/core';
import {TestBed} from '@angular/core/testing';
import {By} from '@angular/platform-browser';

Expand Down Expand Up @@ -919,6 +919,44 @@ describe('host directives', () => {
expect(() => TestBed.createComponent(App))
.toThrowError(/NG0200: Circular dependency in DI detected for HostDir/);
});

it('should inject a valid ChangeDetectorRef when attached to a component', () => {
type InternalChangeDetectorRef = ChangeDetectorRef&{_lView: unknown};

@Directive({standalone: true})
class HostDir {
changeDetectorRef = inject(ChangeDetectorRef) as InternalChangeDetectorRef;
}

@Component({selector: 'my-comp', hostDirectives: [HostDir], template: ''})
class Comp {
changeDetectorRef = inject(ChangeDetectorRef) as InternalChangeDetectorRef;
}

@Component({template: '<my-comp></my-comp>'})
class App {
@ViewChild(HostDir) hostDir!: HostDir;
@ViewChild(Comp) comp!: Comp;
}

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

const hostDirectiveCdr = fixture.componentInstance.hostDir.changeDetectorRef;
const componentCdr = fixture.componentInstance.comp.changeDetectorRef;

// We can't assert that the change detectors are the same by comparing
// them directly, because a new one is created each time. Instead of we
// compare that they're associated with the same LView.
expect(hostDirectiveCdr._lView).toBeTruthy();
expect(componentCdr._lView).toBeTruthy();
expect(hostDirectiveCdr._lView).toBe(componentCdr._lView);
expect(() => {
hostDirectiveCdr.markForCheck();
hostDirectiveCdr.detectChanges();
}).not.toThrow();
});
});

describe('outputs', () => {
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -548,9 +548,6 @@
{
"name": "addClass"
},
{
"name": "addComponentLogic"
},
{
"name": "addPropertyAlias"
},
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -386,9 +386,6 @@
{
"name": "_wrapInTimeout"
},
{
"name": "addComponentLogic"
},
{
"name": "addPropertyAlias"
},
Expand Down Expand Up @@ -743,6 +740,9 @@
{
"name": "isComponentDef"
},
{
"name": "isComponentHost"
},
{
"name": "isContentQueryHost"
},
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -551,9 +551,6 @@
{
"name": "_wrapInTimeout"
},
{
"name": "addComponentLogic"
},
{
"name": "addPropertyAlias"
},
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -539,9 +539,6 @@
{
"name": "_wrapInTimeout"
},
{
"name": "addComponentLogic"
},
{
"name": "addPropertyAlias"
},
Expand Down
3 changes: 0 additions & 3 deletions packages/core/test/bundling/router/bundle.golden_symbols.json
Original file line number Diff line number Diff line change
Expand Up @@ -707,9 +707,6 @@
{
"name": "absoluteRedirect"
},
{
"name": "addComponentLogic"
},
{
"name": "addPropertyAlias"
},
Expand Down
3 changes: 0 additions & 3 deletions packages/core/test/bundling/todo/bundle.golden_symbols.json
Original file line number Diff line number Diff line change
Expand Up @@ -464,9 +464,6 @@
{
"name": "_wrapInTimeout"
},
{
"name": "addComponentLogic"
},
{
"name": "addPropertyAlias"
},
Expand Down

0 comments on commit e4dcaa5

Please sign in to comment.