Skip to content

Commit

Permalink
feat(core): ChangeDetectorRef.detectChanges should detect bootstrap a…
Browse files Browse the repository at this point in the history
…nd onPush not dirty component

Close angular#36667

Calling `componentRef.detectChanges()` sometimes not work as expected.

```
@component({
  selector: 'dynamic',
  template: `{{name}}`,
  changeDetection: ChangeDetectionStrategy.OnPush
})
export class Dynamic  {
  name = 'initial name';
}

const componetRef = cfr.createComponent(Dynamic);
componentRef.changeDetectorRef // host view
componentRef.injector.get(ChangeDetectorRef) // component view
componentRef.instance.cdRef // componentView

componentRef.instance.name = 'something new';
// componentRef.instance.cdRef.markForCheck();
componentRef.detectChanges(); // does not work without `markForCheck`
componentRef.changeDetectorRef.detectChanges() // Users expect that Bar gets refreshed.
componentRef.injector.get(ChangeDetectorRef).detectChanges()
componentRef.instance.cdRef.detectChanges()

// Example: https://stackblitz.com/edit/angular-drqzhw?file=src%2Fapp%2Fapp.module.ts
ngOnInit() {
    const cmptRef = this._cfr
      .resolveComponentFactory(Dynamic)
      .create(this._injector);
    this._vcRef.insert(cmptRef.hostView);

    setTimeout(() => {
      cmptRef.instance.name = "changed name";
      console.log("changing name");
      cmptRef.changeDetectorRef.detectChanges(); // does not work
    });

    setTimeout(() => {
      cmptRef.instance.name = "second changed name";
      console.log("changing name");
      cmptRef.injector.get(ChangeDetectorRef).detectChanges(); // works
    }, 2000);
}
```

Since when the component is `boostrap` or `dynamic`, we created a hostView to host the component.
And the `componentRef.changeDetectorRef` references to that hostView instead of the componentView itself.
And when the component is `OnPush` and not dirty, calling `changeDetectorRef.detectChanges()` will bypass
the component which is confusing.

So this commit mark the root component dirty when calling `changeDetectorRef.detectChanges()` from the root hostView.
  • Loading branch information
JiaLiPassion committed Feb 15, 2021
1 parent 72a00dc commit f16ecd0
Show file tree
Hide file tree
Showing 2 changed files with 117 additions and 2 deletions.
20 changes: 20 additions & 0 deletions packages/core/src/render3/view_ref.ts
Expand Up @@ -16,6 +16,7 @@ import {CONTAINER_HEADER_OFFSET, VIEW_REFS} from './interfaces/container';
import {isLContainer} from './interfaces/type_checks';
import {CONTEXT, FLAGS, LView, LViewFlags, PARENT, TVIEW} from './interfaces/view';
import {destroyLView, detachView, renderDetachView} from './node_manipulation';
import {getComponentLViewByIndex} from './util/view_utils';



Expand Down Expand Up @@ -308,6 +309,25 @@ export class RootViewRef<T> extends ViewRef<T> {
}

detectChanges(): void {
// Mark all root children components dirty to ensure these components run change detection.
// The current RootViewRef is the hostView, when the component is
//
// 1. `bootstrap` or `dynamic`
// 2. Using `ChangeDetectionStrategy.OnPush`
// 3. Not dirty
//
// The change detection will bypass the root component.
// So we need to mark the component dirty.
const tView = this._view[TVIEW];
const components = tView.components;
if (components) {
for (let i = 0; i < components.length; i++) {
const componentView = getComponentLViewByIndex(components[i], this._view);
if (!(componentView[FLAGS] & (LViewFlags.CheckAlways | LViewFlags.Dirty))) {
componentView[FLAGS] |= LViewFlags.Dirty;
}
}
}
detectChangesInRootView(this._view);
}

Expand Down
99 changes: 97 additions & 2 deletions packages/core/test/render3/view_container_ref_spec.ts
Expand Up @@ -8,7 +8,7 @@

import {QueryFlags} from '@angular/core/src/render3/interfaces/query';
import {HEADER_OFFSET} from '@angular/core/src/render3/interfaces/view';
import {ChangeDetectorRef, Component as _Component, ComponentFactoryResolver, ElementRef, QueryList, TemplateRef, ViewContainerRef, ViewRef} from '../../src/core';
import {ChangeDetectionStrategy, ChangeDetectorRef, Component as _Component, ComponentFactoryResolver, ElementRef, QueryList, TemplateRef, ViewContainerRef, ViewRef} from '../../src/core';
import {ViewEncapsulation} from '../../src/metadata';
import {injectComponentFactoryResolver, ɵɵdefineComponent, ɵɵdefineDirective, ɵɵlistener, ɵɵloadQuery, ɵɵqueryRefresh, ɵɵviewQuery} from '../../src/render3/index';
import {ɵɵdirectiveInject, ɵɵelement, ɵɵelementEnd, ɵɵelementStart, ɵɵtemplate, ɵɵtext} from '../../src/render3/instructions/all';
Expand Down Expand Up @@ -163,6 +163,7 @@ describe('ViewContainerRef', () => {

class DynamicComp {
doCheckCount = 0;
doRenderCount = 0;

ngDoCheck() {
this.doCheckCount++;
Expand All @@ -175,7 +176,10 @@ describe('ViewContainerRef', () => {
selectors: [['dynamic-comp']],
decls: 0,
vars: 0,
template: (rf: RenderFlags, cmp: DynamicComp) => {}
template:
(rf: RenderFlags, cmp: DynamicComp) => {
cmp.doRenderCount++;
}
});
}

Expand All @@ -186,11 +190,13 @@ describe('ViewContainerRef', () => {
const ref = fixture.component.vcr.createComponent(dynamicCompFactory);
fixture.update();
expect(dynamicComp.doCheckCount).toEqual(1);
expect(dynamicComp.doRenderCount).toEqual(2);

// The change detector ref should be attached to the root view that contains
// DynamicComp, so the doCheck hook for DynamicComp should run upon ref.detectChanges().
ref.changeDetectorRef.detectChanges();
expect(dynamicComp.doCheckCount).toEqual(2);
expect(dynamicComp.doRenderCount).toEqual(3);
expect((ref.changeDetectorRef as any).context).toBeNull();
});

Expand All @@ -202,12 +208,14 @@ describe('ViewContainerRef', () => {
const ref = fixture.component.vcr.createComponent(dynamicCompFactory);
fixture.update();
expect(dynamicComp.doCheckCount).toEqual(1);
expect(dynamicComp.doRenderCount).toEqual(2);

// The injector should retrieve the change detector ref for DynamicComp. As such,
// the doCheck hook for DynamicComp should NOT run upon ref.detectChanges().
const changeDetector = ref.injector.get(ChangeDetectorRef);
changeDetector.detectChanges();
expect(dynamicComp.doCheckCount).toEqual(1);
expect(dynamicComp.doRenderCount).toEqual(3);
expect((changeDetector as any).context).toEqual(dynamicComp);
});

Expand All @@ -227,6 +235,93 @@ describe('ViewContainerRef', () => {
});
});

describe('createComponent OnPush', () => {
describe('ComponentRef', () => {
let dynamicComp!: DynamicComp;

class AppComp {
constructor(public vcr: ViewContainerRef, public cfr: ComponentFactoryResolver) {}

static ɵfac =
() => {
return new AppComp(
ɵɵdirectiveInject(ViewContainerRef as any), injectComponentFactoryResolver());
}

static ɵcmp = ɵɵdefineComponent({
type: AppComp,
selectors: [['app-comp']],
decls: 0,
vars: 0,
template: (rf: RenderFlags, cmp: AppComp) => {}
});
}

class DynamicComp {
doCheckCount = 0;
doRenderCount = 0;

ngDoCheck() {
this.doCheckCount++;
}

static ɵfac = () => dynamicComp = new DynamicComp();

static ɵcmp = ɵɵdefineComponent({
type: DynamicComp,
selectors: [['dynamic-comp']],
decls: 0,
vars: 0,
changeDetection: ChangeDetectionStrategy.OnPush,
template:
(rf: RenderFlags, cmp: DynamicComp) => {
cmp.doRenderCount++;
}
});
}

it('should return ComponentRef with ChangeDetectorRef attached to root view', () => {
const fixture = new ComponentFixture(AppComp);

const dynamicCompFactory = fixture.component.cfr.resolveComponentFactory(DynamicComp);
const ref = fixture.component.vcr.createComponent(dynamicCompFactory);
fixture.update();
expect(dynamicComp.doCheckCount).toEqual(1);
expect(dynamicComp.doRenderCount).toEqual(2);

// The change detector ref should be attached to the root view that contains
// DynamicComp, so the doCheck hook for DynamicComp should run upon ref.detectChanges().
ref.changeDetectorRef.detectChanges();
expect(dynamicComp.doCheckCount).toEqual(2);
// The DynamicComp is OnPush, and not dirty, the template function should also be
// invoked.
expect(dynamicComp.doRenderCount).toEqual(3);
expect((ref.changeDetectorRef as any).context).toBeNull();
});

it('should return ComponentRef that can retrieve component ChangeDetectorRef through its injector',
() => {
const fixture = new ComponentFixture(AppComp);

const dynamicCompFactory = fixture.component.cfr.resolveComponentFactory(DynamicComp);
const ref = fixture.component.vcr.createComponent(dynamicCompFactory);
fixture.update();
expect(dynamicComp.doCheckCount).toEqual(1);
expect(dynamicComp.doRenderCount).toEqual(2);

// The injector should retrieve the change detector ref for DynamicComp. As such,
// the doCheck hook for DynamicComp should NOT run upon ref.detectChanges().
const changeDetector = ref.injector.get(ChangeDetectorRef);
changeDetector.detectChanges();
expect(dynamicComp.doCheckCount).toEqual(1);
// Since the change detector ref is for the DynamicComp, so the template function
// should be invoked.
expect(dynamicComp.doRenderCount).toEqual(3);
expect((changeDetector as any).context).toEqual(dynamicComp);
});
});
});

describe('getters', () => {
it('should work on elements', () => {
function createTemplate() {
Expand Down

0 comments on commit f16ecd0

Please sign in to comment.