Skip to content

Commit

Permalink
perf(core): Update LView consumer to only mark component for check
Browse files Browse the repository at this point in the history
This commit updates the reactive template and host binding consumers to
only mark their declaration components for refresh, but not parents/ancestors.

This also updates the `AfterViewChecked` hook to run when a component is
refreshed during change detection but its host is not. It is reasonable
to expect that the `ngAfterViewChecked` lifecycle hook will run when a
signal updates and the component is refreshed. The hooks are typically
run when the host is refreshed so without this change, the update to
not mark ancestors dirty would have caused `ngAfterViewChecked` to not
run.

resolves angular#14628
resolves angular#22646

resolves angular#34347 - this is not the direct request of the issue but
generally forcing change detection to run is necessary only because a
value was updated that needs to be synced to the DOM. Values that use
signals will mark the component for check automatically so accessing the
`ChangeDetectorRef` of a child is not necessary. The other part of this
request was to avoid the need to "mark all views for checking since
it wouldn't affect anything but itself". This is directly addressed by
this commit - updating a signal that's read in the view's template
will not cause ancestors/"all views" to be refreshed.
  • Loading branch information
atscott committed Oct 20, 2023
1 parent 3278966 commit 3f8ca6d
Show file tree
Hide file tree
Showing 6 changed files with 478 additions and 171 deletions.
2 changes: 1 addition & 1 deletion packages/core/src/render3/hooks.ts
Original file line number Diff line number Diff line change
Expand Up @@ -245,7 +245,7 @@ function callHooks(
* - it is called in the non-reactive context;
* - profiling data are registered.
*/
function callHookInternal(directive: any, hook: () => void) {
export function callHookInternal(directive: any, hook: () => void) {
profiler(ProfilerEvent.LifecycleHookStart, directive, hook);
const prevConsumer = setActiveConsumer(null);
try {
Expand Down
15 changes: 13 additions & 2 deletions packages/core/src/render3/instructions/change_detection.ts
Original file line number Diff line number Diff line change
Expand Up @@ -9,10 +9,10 @@
import {assertDefined, assertEqual} from '../../util/assert';
import {assertLContainer} from '../assert';
import {getComponentViewByInstance} from '../context_discovery';
import {executeCheckHooks, executeInitAndCheckHooks, incrementInitPhaseFlags} from '../hooks';
import {callHookInternal, executeCheckHooks, executeInitAndCheckHooks, incrementInitPhaseFlags} from '../hooks';
import {CONTAINER_HEADER_OFFSET, HAS_CHILD_VIEWS_TO_REFRESH, HAS_TRANSPLANTED_VIEWS, LContainer, MOVED_VIEWS} from '../interfaces/container';
import {ComponentTemplate, RenderFlags} from '../interfaces/definition';
import {CONTEXT, ENVIRONMENT, FLAGS, InitPhaseState, LView, LViewFlags, PARENT, TVIEW, TView} from '../interfaces/view';
import {CONTEXT, ENVIRONMENT, FLAGS, InitPhaseState, LView, LViewFlags, PARENT, TVIEW, TView, TViewType} from '../interfaces/view';
import {enterView, isInCheckNoChangesMode, leaveView, setBindingIndex, setIsInCheckNoChangesMode} from '../state';
import {getFirstLContainer, getNextLContainer} from '../util/view_traversal_utils';
import {getComponentLViewByIndex, isCreationMode, markAncestorsForTraversal, markViewForRefresh, resetPreOrderHookFlags, viewAttachedToChangeDetector} from '../util/view_utils';
Expand Down Expand Up @@ -325,6 +325,17 @@ function detectChangesInView(lView: LView, mode: ChangeDetectionMode) {
mode === ChangeDetectionMode.Global) ||
flags & LViewFlags.RefreshView) {
refreshView(tView, lView, tView.template, lView[CONTEXT]);
// If we're running in targeted mode, that means the host view must not have been refreshed
// because when that happens, it descends into children in `Global` mode.
const isInCheckNoChangesPass = ngDevMode && isInCheckNoChangesMode();
if (!isInCheckNoChangesPass && mode === ChangeDetectionMode.Targeted &&
tView.type === TViewType.Component) {
// TODO: Is the component itself always the first item in the registry?
const hook = tView.directiveRegistry![0].type.prototype.ngAfterViewChecked;
if (hook !== undefined) {
callHookInternal(lView[CONTEXT], hook);
}
}
} else if (flags & LViewFlags.HasChildViewsToRefresh) {
detectChangesInEmbeddedViews(lView, ChangeDetectionMode.Targeted);
const components = tView.components;
Expand Down
3 changes: 2 additions & 1 deletion packages/core/src/render3/reactive_lview_consumer.ts
Original file line number Diff line number Diff line change
Expand Up @@ -12,6 +12,7 @@ import {assertDefined, assertEqual} from '../util/assert';

import {markViewDirty} from './instructions/mark_view_dirty';
import {LView, REACTIVE_HOST_BINDING_CONSUMER, REACTIVE_TEMPLATE_CONSUMER} from './interfaces/view';
import {markViewDirtyFromSignal} from './util/view_utils';

let currentConsumer: ReactiveLViewConsumer|null = null;
export interface ReactiveLViewConsumer extends ReactiveNode {
Expand Down Expand Up @@ -60,7 +61,7 @@ const REACTIVE_LVIEW_CONSUMER_NODE: ReactiveLViewConsumer = {
assertDefined(
node.lView,
'Updating a signal during template or host binding execution is not allowed.');
markViewDirty(node.lView!);
markViewDirtyFromSignal(node.lView!);
},
lView: null,
};
Expand Down
10 changes: 9 additions & 1 deletion packages/core/src/render3/util/view_utils.ts
Original file line number Diff line number Diff line change
Expand Up @@ -13,7 +13,7 @@ import {HAS_CHILD_VIEWS_TO_REFRESH, LContainer, TYPE} from '../interfaces/contai
import {TConstants, TNode} from '../interfaces/node';
import {RNode} from '../interfaces/renderer_dom';
import {isLContainer, isLView} from '../interfaces/type_checks';
import {DECLARATION_VIEW, FLAGS, HEADER_OFFSET, HOST, LView, LViewFlags, ON_DESTROY_HOOKS, PARENT, PREORDER_HOOK_FLAGS, PreOrderHookFlags, TData, TView} from '../interfaces/view';
import {DECLARATION_COMPONENT_VIEW, DECLARATION_VIEW, FLAGS, HEADER_OFFSET, HOST, LView, LViewFlags, ON_DESTROY_HOOKS, PARENT, PREORDER_HOOK_FLAGS, PreOrderHookFlags, TData, TView} from '../interfaces/view';



Expand Down Expand Up @@ -245,6 +245,14 @@ export function markAncestorsForTraversal(lView: LView) {
}
}

export function markViewDirtyFromSignal(lView: LView): void {
const declarationComponentView = lView[DECLARATION_COMPONENT_VIEW];
declarationComponentView[FLAGS] |= LViewFlags.RefreshView;
if (viewAttachedToChangeDetector(declarationComponentView)) {
markAncestorsForTraversal(declarationComponentView);
}
}

/**
* Stores a LView-specific destroy callback.
*/
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -6,8 +6,8 @@
* found in the LICENSE file at https://angular.io/license
*/

import {NgIf} from '@angular/common';
import {ChangeDetectionStrategy, Component, Input, signal, ViewChild} from '@angular/core';
import {NgIf, NgFor} from '@angular/common';
import {ChangeDetectionStrategy, Component, Input, signal, ChangeDetectorRef, ViewChild} from '@angular/core';
import {TestBed} from '@angular/core/testing';

describe('CheckAlways components', () => {
Expand Down Expand Up @@ -88,6 +88,7 @@ describe('CheckAlways components', () => {
fixture.detectChanges();
expect(fixture.nativeElement.textContent.trim()).toBe('new');
});

});


Expand Down Expand Up @@ -370,4 +371,200 @@ describe('OnPush components with signals', () => {
fixture.detectChanges();
expect(fixture.nativeElement.outerHTML).not.toContain('blue');
});

it('does not refresh view if signal marked dirty but did not change', () => {
const val = signal('initial', {equal: () => true});

@Component({
template: '{{val()}}{{incrementChecks()}}',
standalone: true,
changeDetection: ChangeDetectionStrategy.OnPush,
})
class App {
val = val;
templateExecutions = 0;
incrementChecks() {
this.templateExecutions++;
}
}

const fixture = TestBed.createComponent(App);
fixture.detectChanges();
expect(fixture.componentInstance.templateExecutions).toBe(1);
expect(fixture.nativeElement.innerText).toContain('initial');

val.set('new');
fixture.detectChanges();
expect(fixture.componentInstance.templateExecutions).toBe(1);
expect(fixture.nativeElement.innerText).toContain('initial');
});

describe('embedded views', () => {
it('with a single signal, single view', () => {
@Component({
selector: 'signal-component',
changeDetection: ChangeDetectionStrategy.OnPush,
standalone: true,
imports: [NgIf],
template: `<div *ngIf="true"> {{value()}} </div>`,
})
class SignalComponent {
value = signal('initial');
}

const fixture = TestBed.createComponent(SignalComponent);
fixture.detectChanges();
fixture.componentInstance.value.set('new');
fixture.detectChanges();
expect(trim(fixture.nativeElement.textContent)).toEqual('new');
});

it('with a single signal, multiple views', () => {
@Component({
selector: 'signal-component',
changeDetection: ChangeDetectionStrategy.OnPush,
standalone: true,
imports: [NgFor],
template: `<div *ngFor="let i of [1,2,3]"> {{value()}} </div>`,
})
class SignalComponent {
value = signal('initial');
}

const fixture = TestBed.createComponent(SignalComponent);
fixture.detectChanges();
fixture.componentInstance.value.set('new');
fixture.detectChanges();
expect(trim(fixture.nativeElement.textContent)).toEqual('new new new');
});


it('does not execute view template if signal not updated or marked dirty', () => {
@Component({
selector: 'signal-component',
changeDetection: ChangeDetectionStrategy.OnPush,
standalone: true,
imports: [NgIf],
template: `
{{componentSignal()}}
<div *ngIf="true"> {{incrementExecutions()}} </div>
`,
})
class SignalComponent {
embeddedViewExecutions = 0;
componentSignal = signal('initial');
incrementExecutions() {
this.embeddedViewExecutions++;
return '';
}
}

const fixture = TestBed.createComponent(SignalComponent);
fixture.detectChanges(false);
expect(fixture.componentInstance.embeddedViewExecutions).toEqual(1);

fixture.componentInstance.componentSignal.set('new');
fixture.detectChanges(false);
expect(trim(fixture.nativeElement.textContent)).toEqual('new');
// OnPush/Default components are checked as a whole so the embedded view is also checked again
expect(fixture.componentInstance.embeddedViewExecutions).toEqual(2);
});


it('re-executes deep embedded template if signal updates', () => {
@Component({
selector: 'signal-component',
standalone: true,
changeDetection: ChangeDetectionStrategy.OnPush,
imports: [NgIf],
template: `
<div *ngIf="true">
<div *ngIf="true">
<div *ngIf="true">
{{value()}}
</div>
</div>
</div>
`,
})
class SignalComponent {
value = signal('initial');
}

const fixture = TestBed.createComponent(SignalComponent);
fixture.detectChanges();

fixture.componentInstance.value.set('new')
fixture.detectChanges();
expect(trim(fixture.nativeElement.textContent)).toEqual('new');
});
});

describe('shielded by non-dirty OnPush', () => {
@Component({
selector: 'signal-component',
changeDetection: ChangeDetectionStrategy.OnPush,
standalone: true,
template: `{{value()}}`,
})
class SignalComponent {
value = signal('initial');
afterViewCheckedRuns = 0;
constructor(readonly cdr: ChangeDetectorRef) {}
ngAfterViewChecked() {
this.afterViewCheckedRuns++;
}
}

@Component({
selector: 'on-push-parent',
template: `<signal-component></signal-component>{{incrementChecks()}}`,
changeDetection: ChangeDetectionStrategy.OnPush,
standalone: true,
imports: [SignalComponent],
})
class OnPushParent {
@ViewChild(SignalComponent) signalChild!: SignalComponent;
viewExecutions = 0;

constructor(readonly cdr: ChangeDetectorRef) {}
incrementChecks() {
this.viewExecutions++;
}
}

it('refreshes when signal changes, but does not refresh non-dirty parent', () => {
const fixture = TestBed.createComponent(OnPushParent);
fixture.detectChanges();
expect(fixture.componentInstance.viewExecutions).toEqual(1);
fixture.componentInstance.signalChild.value.set('new');
fixture.detectChanges();
expect(fixture.componentInstance.viewExecutions).toEqual(1);
expect(trim(fixture.nativeElement.textContent)).toEqual('new');
});

it('does not refresh when detached', () => {
const fixture = TestBed.createComponent(OnPushParent);
fixture.detectChanges();
fixture.componentInstance.signalChild.value.set('new');
fixture.componentInstance.signalChild.cdr.detach();
fixture.detectChanges();
expect(trim(fixture.nativeElement.textContent)).toEqual('initial');
});

it('runs afterViewChecked hooks even though parent view was not dirty (those hooks are executed by the parent)',
() => {
const fixture = TestBed.createComponent(OnPushParent);
fixture.detectChanges();
fixture.componentInstance.signalChild.value.set('new');
fixture.detectChanges();
expect(trim(fixture.nativeElement.textContent)).toEqual('new');
expect(fixture.componentInstance.signalChild.afterViewCheckedRuns).toBe(2);
});
});
});


function trim(text: string|null): string {
return text ? text.replace(/[\s\n]+/gm, ' ').trim() : '';
}
Loading

0 comments on commit 3f8ca6d

Please sign in to comment.