Skip to content

Commit

Permalink
fix(core): execute template creation in non-reactive context (#49883)
Browse files Browse the repository at this point in the history
This fix assures that templates functions executed in the creation mode
are run outside of the reactive context. This avoids the situation where
signal reads in a directive constructor (executed as part of the creation
mode) would mark the host component as dirty.

Fixes #49871

PR Close #49883
  • Loading branch information
pkozlowski-opensource authored and thePunderWoman committed Apr 18, 2023
1 parent c985ed2 commit ef91a2e
Show file tree
Hide file tree
Showing 3 changed files with 90 additions and 4 deletions.
14 changes: 12 additions & 2 deletions packages/core/src/render3/instructions/shared.ts
Original file line number Diff line number Diff line change
Expand Up @@ -17,6 +17,7 @@ import {DoCheck, OnChanges, OnInit} from '../../interface/lifecycle_hooks';
import {SchemaMetadata} from '../../metadata/schema';
import {ViewEncapsulation} from '../../metadata/view';
import {validateAgainstEventAttributes, validateAgainstEventProperties} from '../../sanitization/sanitization';
import {setActiveConsumer} from '../../signals';
import {assertDefined, assertEqual, assertGreaterThan, assertGreaterThanOrEqual, assertIndexInRange, assertNotEqual, assertNotSame, assertSame, assertString} from '../../util/assert';
import {escapeCommentText} from '../../util/dom';
import {normalizeDebugBindingName, normalizeDebugBindingValue} from '../../util/ng_reflect';
Expand Down Expand Up @@ -507,9 +508,18 @@ function executeTemplate<T>(
const preHookType =
isUpdatePhase ? ProfilerEvent.TemplateUpdateStart : ProfilerEvent.TemplateCreateStart;
profiler(preHookType, context as unknown as {});
consumer.runInContext(templateFn, rf, context);
if (isUpdatePhase) {
consumer.runInContext(templateFn, rf, context);
} else {
const prevConsumer = setActiveConsumer(null);
try {
templateFn(rf, context);
} finally {
setActiveConsumer(prevConsumer);
}
}
} finally {
if (lView[REACTIVE_TEMPLATE_CONSUMER] === null) {
if (isUpdatePhase && lView[REACTIVE_TEMPLATE_CONSUMER] === null) {
commitLViewConsumerIfHasProducers(lView, REACTIVE_TEMPLATE_CONSUMER);
}
setSelectedIndex(prevSelectedIndex);
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -6,6 +6,7 @@
* found in the LICENSE file at https://angular.io/license
*/

import {NgIf} from '@angular/common';
import {ChangeDetectionStrategy, Component} from '@angular/core';
import {TestBed} from '@angular/core/testing';

Expand Down Expand Up @@ -92,6 +93,54 @@ describe('OnPush components with signals', () => {
expect(instance.value()).toBe('new');
});

it('should not mark components as dirty when signal is read in a constructor of a child component',
() => {
const state = signal('initial');

@Component({
selector: 'child',
template: `child`,
changeDetection: ChangeDetectionStrategy.OnPush,
standalone: true,
})
class ChildReadingSignalCmp {
constructor() {
state();
}
}

@Component({
template: `
{{incrementTemplateExecutions()}}
<!-- Template constructed to execute child component constructor in the update pass of a host component -->
<ng-template [ngIf]="true"><child></child></ng-template>
`,
changeDetection: ChangeDetectionStrategy.OnPush,
standalone: true,
imports: [NgIf, ChildReadingSignalCmp],
})
class OnPushCmp {
numTemplateExecutions = 0;
incrementTemplateExecutions() {
this.numTemplateExecutions++;
return '';
}
}

const fixture = TestBed.createComponent(OnPushCmp);
const instance = fixture.componentInstance;

fixture.detectChanges();
expect(instance.numTemplateExecutions).toBe(1);
expect(fixture.nativeElement.textContent.trim()).toEqual('child');

// The "state" signal is not accesses in the template's update function anywhere so it
// shouldn't mark components as dirty / impact change detection.
state.set('new');
fixture.detectChanges();
expect(instance.numTemplateExecutions).toBe(1);
});

it('can read a signal in a host binding', () => {
@Component({
template: `{{incrementTemplateExecutions()}}`,
Expand Down
31 changes: 29 additions & 2 deletions packages/core/test/render3/reactivity_spec.ts
Original file line number Diff line number Diff line change
Expand Up @@ -239,8 +239,7 @@ describe('effects', () => {
selector: 'test-cmp',
standalone: true,
imports: [WithInput],
template: `<with-input [in]="'A'" />|<with-input [in]="'B'" />
`,
template: `<with-input [in]="'A'" />|<with-input [in]="'B'" />`,
})
class Cmp {
}
Expand All @@ -249,4 +248,32 @@ describe('effects', () => {
fixture.detectChanges();
expect(fixture.nativeElement.textContent).toBe('A|B');
});

it('should allow writing to signals in a constructor', () => {
@Component({
selector: 'with-constructor',
standalone: true,
template: '{{state()}}',
})
class WithConstructor {
state = signal('property initializer');

constructor() {
this.state.set('constructor');
}
}

@Component({
selector: 'test-cmp',
standalone: true,
imports: [WithConstructor],
template: `<with-constructor />`,
})
class Cmp {
}

const fixture = TestBed.createComponent(Cmp);
fixture.detectChanges();
expect(fixture.nativeElement.textContent).toBe('constructor');
});
});

0 comments on commit ef91a2e

Please sign in to comment.