Skip to content

Commit 83a3b85

Browse files
atscottalxhub
atscott
authored and
alxhub
committed
refactor(core): Do not refresh view if producers did not actually change (#52476)
Producers represent values which can deliver change notifications. When a producer value is changed, a change notification is propagated through the graph, notifying live consumers which depend on the producer of the potential update. Note here that this is a _potential_ update. A producer may not have actually "changed" based on its equality function. With this commit, before refreshing a view that is only marked for refresh because its consumer is dirty, we poll producers for change to see if they really have. If not, we can skip the refresh. The example test in this commit shows that a `computed` which depends on a `signal` that is updated but produces a value that is the same as before will _not_ cause the component's template to refresh. fixes #51797 PR Close #52476
1 parent 164cfc0 commit 83a3b85

File tree

18 files changed

+166
-174
lines changed

18 files changed

+166
-174
lines changed

‎goldens/public-api/core/primitives/signals/index.md‎

Lines changed: 3 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -13,6 +13,9 @@ export function consumerBeforeComputation(node: ReactiveNode | null): ReactiveNo
1313
// @public
1414
export function consumerDestroy(node: ReactiveNode): void;
1515

16+
// @public
17+
export function consumerPollProducersForChange(node: ReactiveNode): boolean;
18+
1619
// @public
1720
export function createComputed<T>(computation: () => T): ComputedGetter<T>;
1821

‎packages/core/src/render3/instructions/change_detection.ts‎

Lines changed: 5 additions & 8 deletions
Original file line numberDiff line numberDiff line change
@@ -15,12 +15,8 @@ import {getComponentViewByInstance} from '../context_discovery';
1515
import {executeCheckHooks, executeInitAndCheckHooks, incrementInitPhaseFlags} from '../hooks';
1616
import {CONTAINER_HEADER_OFFSET, HAS_CHILD_VIEWS_TO_REFRESH, HAS_TRANSPLANTED_VIEWS, LContainer, MOVED_VIEWS} from '../interfaces/container';
1717
import {ComponentTemplate, RenderFlags} from '../interfaces/definition';
18-
<<<<<<< HEAD
19-
import {CONTEXT, EFFECTS_TO_SCHEDULE, ENVIRONMENT, FLAGS, InitPhaseState, LView, LViewFlags, PARENT, TVIEW, TView, TViewType} from '../interfaces/view';
20-
=======
21-
import {CONTEXT, ENVIRONMENT, FLAGS, InitPhaseState, LView, LViewFlags, PARENT, REACTIVE_TEMPLATE_CONSUMER, TVIEW, TView, TViewType} from '../interfaces/view';
18+
import {CONTEXT, EFFECTS_TO_SCHEDULE, ENVIRONMENT, FLAGS, InitPhaseState, LView, LViewFlags, PARENT, REACTIVE_TEMPLATE_CONSUMER, TVIEW, TView, TViewType} from '../interfaces/view';
2219
import {getOrBorrowReactiveLViewConsumer, maybeReturnReactiveLViewConsumer, ReactiveLViewConsumer} from '../reactive_lview_consumer';
23-
>>>>>>> 48df4581bb (refactor(core): Update LView consumer to use only 1 consumer for a component)
2420
import {enterView, isInCheckNoChangesMode, leaveView, setBindingIndex, setIsInCheckNoChangesMode} from '../state';
2521
import {getFirstLContainer, getNextLContainer} from '../util/view_traversal_utils';
2622
import {getComponentLViewByIndex, isCreationMode, markAncestorsForTraversal, markViewForRefresh, resetPreOrderHookFlags, viewAttachedToChangeDetector} from '../util/view_utils';
@@ -405,14 +401,15 @@ function detectChangesInView(lView: LView, mode: ChangeDetectionMode) {
405401
// backwards views, it gives an opportunity for `OnPush` components to be marked `Dirty`
406402
// before the CheckNoChanges pass. We don't want existing errors that are hidden by the
407403
// current CheckNoChanges bug to surface when making unrelated changes.
408-
shouldRefreshView ||= !!(
409-
flags & LViewFlags.Dirty && mode === ChangeDetectionMode.Global && (!isInCheckNoChangesPass || RUN_IN_CHECK_NO_CHANGES_ANYWAY));
404+
shouldRefreshView ||=
405+
!!(flags & LViewFlags.Dirty && mode === ChangeDetectionMode.Global &&
406+
(!isInCheckNoChangesPass || RUN_IN_CHECK_NO_CHANGES_ANYWAY));
410407

411408
// Always refresh views marked for refresh, regardless of mode.
412409
shouldRefreshView ||= !!(flags & LViewFlags.RefreshView);
413410

414411
// Refresh views when they have a dirty reactive consumer, regardless of mode.
415-
shouldRefreshView ||= !!consumer?.dirty;
412+
shouldRefreshView ||= !!(consumer?.dirty && consumerPollProducersForChange(consumer));
416413

417414
// Mark the Flags and `ReactiveNode` as not dirty before refreshing the component, so that they
418415
// can be re-dirtied during the refresh process.

‎packages/core/src/render3/instructions/shared.ts‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -38,7 +38,7 @@ import {Renderer} from '../interfaces/renderer';
3838
import {RComment, RElement, RNode, RText} from '../interfaces/renderer_dom';
3939
import {SanitizerFn} from '../interfaces/sanitization';
4040
import {isComponentDef, isComponentHost, isContentQueryHost} from '../interfaces/type_checks';
41-
import {CHILD_HEAD, CHILD_TAIL, CLEANUP, CONTEXT, DECLARATION_COMPONENT_VIEW, DECLARATION_VIEW, EMBEDDED_VIEW_INJECTOR, ENVIRONMENT, FLAGS, HEADER_OFFSET, HOST, HostBindingOpCodes, HYDRATION, ID, INJECTOR, LView, LViewEnvironment, LViewFlags, NEXT, PARENT, REACTIVE_HOST_BINDING_CONSUMER, REACTIVE_TEMPLATE_CONSUMER, RENDERER, T_HOST, TData, TVIEW, TView, TViewType} from '../interfaces/view';
41+
import {CHILD_HEAD, CHILD_TAIL, CLEANUP, CONTEXT, DECLARATION_COMPONENT_VIEW, DECLARATION_VIEW, EMBEDDED_VIEW_INJECTOR, ENVIRONMENT, FLAGS, HEADER_OFFSET, HOST, HostBindingOpCodes, HYDRATION, ID, INJECTOR, LView, LViewEnvironment, LViewFlags, NEXT, PARENT, REACTIVE_TEMPLATE_CONSUMER, RENDERER, T_HOST, TData, TVIEW, TView, TViewType} from '../interfaces/view';
4242
import {assertPureTNodeType, assertTNodeType} from '../node_assert';
4343
import {clearElementContents, updateTextNode} from '../node_manipulation';
4444
import {isInlineTemplate, isNodeMatchingSelectorList} from '../node_selector_matcher';

‎packages/core/src/render3/interfaces/view.ts‎

Lines changed: 0 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -59,7 +59,6 @@ export const EMBEDDED_VIEW_INJECTOR = 20;
5959
export const ON_DESTROY_HOOKS = 21;
6060
export const EFFECTS_TO_SCHEDULE = 22;
6161
export const REACTIVE_TEMPLATE_CONSUMER = 23;
62-
export const REACTIVE_HOST_BINDING_CONSUMER = 24;
6362

6463
/**
6564
* Size of LView's header. Necessary to adjust for it when setting slots.
@@ -356,11 +355,6 @@ export interface LView<T = unknown> extends Array<any> {
356355
* if any signals were read.
357356
*/
358357
[REACTIVE_TEMPLATE_CONSUMER]: ReactiveLViewConsumer|null;
359-
360-
/**
361-
* Same as REACTIVE_TEMPLATE_CONSUMER, but for the host bindings of the LView.
362-
*/
363-
[REACTIVE_HOST_BINDING_CONSUMER]: ReactiveLViewConsumer|null;
364358
}
365359

366360
/**

‎packages/core/src/render3/node_manipulation.ts‎

Lines changed: 1 addition & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -26,7 +26,7 @@ import {TElementNode, TIcuContainerNode, TNode, TNodeFlags, TNodeType, TProjecti
2626
import {Renderer} from './interfaces/renderer';
2727
import {RComment, RElement, RNode, RTemplate, RText} from './interfaces/renderer_dom';
2828
import {isLContainer, isLView} from './interfaces/type_checks';
29-
import {CHILD_HEAD, CLEANUP, DECLARATION_COMPONENT_VIEW, DECLARATION_LCONTAINER, DestroyHookData, FLAGS, HookData, HookFn, HOST, LView, LViewFlags, NEXT, ON_DESTROY_HOOKS, PARENT, QUERIES, REACTIVE_HOST_BINDING_CONSUMER, REACTIVE_TEMPLATE_CONSUMER, RENDERER, T_HOST, TVIEW, TView, TViewType} from './interfaces/view';
29+
import {CHILD_HEAD, CLEANUP, DECLARATION_COMPONENT_VIEW, DECLARATION_LCONTAINER, DestroyHookData, FLAGS, HookData, HookFn, HOST, LView, LViewFlags, NEXT, ON_DESTROY_HOOKS, PARENT, QUERIES, REACTIVE_TEMPLATE_CONSUMER, RENDERER, T_HOST, TVIEW, TView, TViewType} from './interfaces/view';
3030
import {assertTNodeType} from './node_assert';
3131
import {profiler, ProfilerEvent} from './profiler';
3232
import {setUpAttributes} from './util/attrs_utils';
@@ -375,7 +375,6 @@ export function destroyLView(tView: TView, lView: LView) {
375375
const renderer = lView[RENDERER];
376376

377377
lView[REACTIVE_TEMPLATE_CONSUMER] && consumerDestroy(lView[REACTIVE_TEMPLATE_CONSUMER]);
378-
lView[REACTIVE_HOST_BINDING_CONSUMER] && consumerDestroy(lView[REACTIVE_HOST_BINDING_CONSUMER]);
379378

380379
if (renderer.destroyNode) {
381380
applyView(tView, lView, renderer, WalkTNodeTreeAction.Destroy, null, null);

‎packages/core/src/render3/reactive_lview_consumer.ts‎

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -8,13 +8,13 @@
88

99
import {REACTIVE_NODE, ReactiveNode} from '@angular/core/primitives/signals';
1010

11-
import {LView, REACTIVE_HOST_BINDING_CONSUMER, REACTIVE_TEMPLATE_CONSUMER} from './interfaces/view';
11+
import {LView, REACTIVE_TEMPLATE_CONSUMER} from './interfaces/view';
1212
import {markAncestorsForTraversal} from './util/view_utils';
1313

1414
let freeConsumers: ReactiveLViewConsumer[] = [];
1515
export interface ReactiveLViewConsumer extends ReactiveNode {
1616
lView: LView|null;
17-
slot: typeof REACTIVE_TEMPLATE_CONSUMER|typeof REACTIVE_HOST_BINDING_CONSUMER;
17+
slot: typeof REACTIVE_TEMPLATE_CONSUMER;
1818
}
1919

2020
/**

‎packages/core/test/acceptance/change_detection_signals_in_zones_spec.ts‎

Lines changed: 33 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -204,6 +204,38 @@ describe('OnPush components with signals', () => {
204204
expect(instance.value()).toBe('new');
205205
});
206206

207+
it('does not refresh a component when a signal notifies but isn\'t actually updated', () => {
208+
@Component({
209+
template: `{{memo()}}{{incrementTemplateExecutions()}}`,
210+
changeDetection: ChangeDetectionStrategy.OnPush,
211+
standalone: true,
212+
})
213+
class OnPushCmp {
214+
numTemplateExecutions = 0;
215+
value = signal({value: 'initial'});
216+
memo = computed(() => this.value().value, {equal: Object.is});
217+
incrementTemplateExecutions() {
218+
this.numTemplateExecutions++;
219+
return '';
220+
}
221+
}
222+
const fixture = TestBed.createComponent(OnPushCmp);
223+
const instance = fixture.componentInstance;
224+
225+
fixture.detectChanges();
226+
expect(instance.numTemplateExecutions).toBe(1);
227+
expect(fixture.nativeElement.textContent.trim()).toEqual('initial');
228+
229+
instance.value.update(v => ({...v}));
230+
fixture.detectChanges();
231+
expect(instance.numTemplateExecutions).toBe(1);
232+
233+
instance.value.update(v => ({value: 'new'}));
234+
fixture.detectChanges();
235+
expect(instance.numTemplateExecutions).toBe(2);
236+
expect(fixture.nativeElement.textContent.trim()).toEqual('new');
237+
});
238+
207239
it('should not mark components as dirty when signal is read in a constructor of a child component',
208240
() => {
209241
const state = signal('initial');
@@ -778,7 +810,7 @@ describe('OnPush components with signals', () => {
778810
}
779811

780812
@Component({
781-
template: '{{val()}}<child />',
813+
template: '{{val()}} <child />',
782814
imports: [Child],
783815
standalone: true,
784816
})

‎packages/core/test/bundling/animations-standalone/bundle.golden_symbols.json‎

Lines changed: 11 additions & 14 deletions
Original file line numberDiff line numberDiff line change
@@ -738,16 +738,10 @@
738738
"name": "connectableObservableDescriptor"
739739
},
740740
{
741-
"name": "consumerAfterComputation"
742-
},
743-
{
744-
"name": "consumerBeforeComputation"
745-
},
746-
{
747-
"name": "consumerDestroy"
741+
"name": "consumerIsLive"
748742
},
749743
{
750-
"name": "consumerIsLive"
744+
"name": "consumerPollProducersForChange"
751745
},
752746
{
753747
"name": "containsElement"
@@ -794,9 +788,6 @@
794788
{
795789
"name": "createTransitionInstruction"
796790
},
797-
{
798-
"name": "currentConsumer"
799-
},
800791
{
801792
"name": "dashCaseToCamelCase"
802793
},
@@ -845,6 +836,9 @@
845836
{
846837
"name": "enterView"
847838
},
839+
{
840+
"name": "epoch"
841+
},
848842
{
849843
"name": "eraseStyles"
850844
},
@@ -887,6 +881,9 @@
887881
{
888882
"name": "forwardRef"
889883
},
884+
{
885+
"name": "freeConsumers"
886+
},
890887
{
891888
"name": "from"
892889
},
@@ -1007,9 +1004,6 @@
10071004
{
10081005
"name": "getPromiseCtor"
10091006
},
1010-
{
1011-
"name": "getReactiveLViewConsumer"
1012-
},
10131007
{
10141008
"name": "getSimpleChangesStore"
10151009
},
@@ -1298,6 +1292,9 @@
12981292
{
12991293
"name": "producerRemoveLiveConsumerAtIndex"
13001294
},
1295+
{
1296+
"name": "producerUpdateValueVersion"
1297+
},
13011298
{
13021299
"name": "profiler"
13031300
},

‎packages/core/test/bundling/animations/bundle.golden_symbols.json‎

Lines changed: 11 additions & 14 deletions
Original file line numberDiff line numberDiff line change
@@ -795,16 +795,10 @@
795795
"name": "connectableObservableDescriptor"
796796
},
797797
{
798-
"name": "consumerAfterComputation"
799-
},
800-
{
801-
"name": "consumerBeforeComputation"
802-
},
803-
{
804-
"name": "consumerDestroy"
798+
"name": "consumerIsLive"
805799
},
806800
{
807-
"name": "consumerIsLive"
801+
"name": "consumerPollProducersForChange"
808802
},
809803
{
810804
"name": "containsElement"
@@ -857,9 +851,6 @@
857851
{
858852
"name": "createTransitionInstruction"
859853
},
860-
{
861-
"name": "currentConsumer"
862-
},
863854
{
864855
"name": "dashCaseToCamelCase"
865856
},
@@ -908,6 +899,9 @@
908899
{
909900
"name": "enterView"
910901
},
902+
{
903+
"name": "epoch"
904+
},
911905
{
912906
"name": "eraseStyles"
913907
},
@@ -950,6 +944,9 @@
950944
{
951945
"name": "forwardRef"
952946
},
947+
{
948+
"name": "freeConsumers"
949+
},
953950
{
954951
"name": "from"
955952
},
@@ -1073,9 +1070,6 @@
10731070
{
10741071
"name": "getPromiseCtor"
10751072
},
1076-
{
1077-
"name": "getReactiveLViewConsumer"
1078-
},
10791073
{
10801074
"name": "getSimpleChangesStore"
10811075
},
@@ -1373,6 +1367,9 @@
13731367
{
13741368
"name": "producerRemoveLiveConsumerAtIndex"
13751369
},
1370+
{
1371+
"name": "producerUpdateValueVersion"
1372+
},
13761373
{
13771374
"name": "profiler"
13781375
},

‎packages/core/test/bundling/cyclic_import/bundle.golden_symbols.json‎

Lines changed: 11 additions & 14 deletions
Original file line numberDiff line numberDiff line change
@@ -600,16 +600,10 @@
600600
"name": "connectableObservableDescriptor"
601601
},
602602
{
603-
"name": "consumerAfterComputation"
604-
},
605-
{
606-
"name": "consumerBeforeComputation"
607-
},
608-
{
609-
"name": "consumerDestroy"
603+
"name": "consumerIsLive"
610604
},
611605
{
612-
"name": "consumerIsLive"
606+
"name": "consumerPollProducersForChange"
613607
},
614608
{
615609
"name": "convertToBitFlags"
@@ -641,9 +635,6 @@
641635
{
642636
"name": "createTView"
643637
},
644-
{
645-
"name": "currentConsumer"
646-
},
647638
{
648639
"name": "deepForEach"
649640
},
@@ -686,6 +677,9 @@
686677
{
687678
"name": "enterView"
688679
},
680+
{
681+
"name": "epoch"
682+
},
689683
{
690684
"name": "executeCheckHooks"
691685
},
@@ -719,6 +713,9 @@
719713
{
720714
"name": "forwardRef"
721715
},
716+
{
717+
"name": "freeConsumers"
718+
},
722719
{
723720
"name": "from"
724721
},
@@ -836,9 +833,6 @@
836833
{
837834
"name": "getPromiseCtor"
838835
},
839-
{
840-
"name": "getReactiveLViewConsumer"
841-
},
842836
{
843837
"name": "getSimpleChangesStore"
844838
},
@@ -1094,6 +1088,9 @@
10941088
{
10951089
"name": "producerRemoveLiveConsumerAtIndex"
10961090
},
1091+
{
1092+
"name": "producerUpdateValueVersion"
1093+
},
10971094
{
10981095
"name": "profiler"
10991096
},

0 commit comments

Comments
 (0)