From 133b9eefe362ac46497d6bcfaa0115269dc3b236 Mon Sep 17 00:00:00 2001 From: Andrew Scott Date: Thu, 26 Oct 2023 16:40:19 -0700 Subject: [PATCH 1/3] refactor(core): Remove warning about signal set during template execution The significance of the combination of #51854 and #52302 went mostly unnoticed. The first removed a unidirectional data flow constraint for transplanted views and the second updated the signal implementation to share transplanted view logic. The result is that we automatically get behavior that (mostly) removes `ExpressionChangedAfterItWasCheckedError` when signals are used to drive application state to DOM synchronization. fixes #50320 --- .../render3/instructions/change_detection.ts | 28 ++--- .../core/src/render3/instructions/shared.ts | 4 - .../src/render3/reactive_lview_consumer.ts | 9 -- .../change_detection_signals_in_zones_spec.ts | 114 ++++++++++++------ 4 files changed, 89 insertions(+), 66 deletions(-) diff --git a/packages/core/src/render3/instructions/change_detection.ts b/packages/core/src/render3/instructions/change_detection.ts index 621653e0520b..99d8f8ea3b30 100644 --- a/packages/core/src/render3/instructions/change_detection.ts +++ b/packages/core/src/render3/instructions/change_detection.ts @@ -13,7 +13,7 @@ import {getComponentViewByInstance} from '../context_discovery'; import {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, EFFECTS_TO_SCHEDULE, ENVIRONMENT, FLAGS, InitPhaseState, LView, LViewFlags, PARENT, REACTIVE_TEMPLATE_CONSUMER, TVIEW, TView, TViewType} from '../interfaces/view'; +import {CONTEXT, EFFECTS_TO_SCHEDULE, 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'; @@ -160,23 +160,17 @@ export function refreshView( // execute pre-order hooks (OnInit, OnChanges, DoCheck) // PERF WARNING: do NOT extract this to a separate function without running benchmarks if (!isInCheckNoChangesPass) { - const consumer = lView[REACTIVE_TEMPLATE_CONSUMER]; - try { - consumer && (consumer.isRunning = true); - if (hooksInitPhaseCompleted) { - const preOrderCheckHooks = tView.preOrderCheckHooks; - if (preOrderCheckHooks !== null) { - executeCheckHooks(lView, preOrderCheckHooks, null); - } - } else { - const preOrderHooks = tView.preOrderHooks; - if (preOrderHooks !== null) { - executeInitAndCheckHooks(lView, preOrderHooks, InitPhaseState.OnInitHooksToBeRun, null); - } - incrementInitPhaseFlags(lView, InitPhaseState.OnInitHooksToBeRun); + if (hooksInitPhaseCompleted) { + const preOrderCheckHooks = tView.preOrderCheckHooks; + if (preOrderCheckHooks !== null) { + executeCheckHooks(lView, preOrderCheckHooks, null); + } + } else { + const preOrderHooks = tView.preOrderHooks; + if (preOrderHooks !== null) { + executeInitAndCheckHooks(lView, preOrderHooks, InitPhaseState.OnInitHooksToBeRun, null); } - } finally { - consumer && (consumer.isRunning = false); + incrementInitPhaseFlags(lView, InitPhaseState.OnInitHooksToBeRun); } } diff --git a/packages/core/src/render3/instructions/shared.ts b/packages/core/src/render3/instructions/shared.ts index 5339058590c2..c74c707c2ab9 100644 --- a/packages/core/src/render3/instructions/shared.ts +++ b/packages/core/src/render3/instructions/shared.ts @@ -82,13 +82,11 @@ export function processHostBindingOpCodes(tView: TView, lView: LView): void { setBindingRootForHostBindings(bindingRootIndx, directiveIdx); consumer.dirty = false; const prevConsumer = consumerBeforeComputation(consumer); - consumer.isRunning = true; try { const context = lView[directiveIdx]; hostBindingFn(RenderFlags.Update, context); } finally { consumerAfterComputation(consumer, prevConsumer); - consumer.isRunning = false; } } } @@ -274,12 +272,10 @@ export function executeTemplate( try { if (effectiveConsumer !== null) { effectiveConsumer.dirty = false; - effectiveConsumer.isRunning = true; } templateFn(rf, context); } finally { consumerAfterComputation(effectiveConsumer, prevConsumer); - effectiveConsumer && (effectiveConsumer.isRunning = false); } } finally { setSelectedIndex(prevSelectedIndex); diff --git a/packages/core/src/render3/reactive_lview_consumer.ts b/packages/core/src/render3/reactive_lview_consumer.ts index 2bb276a3de63..979b6ad50841 100644 --- a/packages/core/src/render3/reactive_lview_consumer.ts +++ b/packages/core/src/render3/reactive_lview_consumer.ts @@ -15,7 +15,6 @@ let currentConsumer: ReactiveLViewConsumer|null = null; export interface ReactiveLViewConsumer extends ReactiveNode { lView: LView; slot: typeof REACTIVE_TEMPLATE_CONSUMER|typeof REACTIVE_HOST_BINDING_CONSUMER; - isRunning: boolean; } /** @@ -33,13 +32,6 @@ const REACTIVE_LVIEW_CONSUMER_NODE: Omit ...REACTIVE_NODE, consumerIsAlwaysLive: true, consumerMarkedDirty: (node: ReactiveLViewConsumer) => { - if (ngDevMode && node.isRunning) { - console.warn( - `Angular detected a signal being set which makes the template for this component dirty` + - ` while it's being executed, which is not currently supported and will likely result` + - ` in ExpressionChangedAfterItHasBeenChecked errors or future updates not working` + - ` entirely.`); - } markViewDirtyFromSignal(node.lView); }, consumerOnSignalRead(this: ReactiveLViewConsumer): void { @@ -49,7 +41,6 @@ const REACTIVE_LVIEW_CONSUMER_NODE: Omit this.lView[this.slot] = currentConsumer; currentConsumer = null; }, - isRunning: false, }; function createLViewConsumer(): ReactiveLViewConsumer { diff --git a/packages/core/test/acceptance/change_detection_signals_in_zones_spec.ts b/packages/core/test/acceptance/change_detection_signals_in_zones_spec.ts index 6fe555c4e8e1..1303ba76c2d0 100644 --- a/packages/core/test/acceptance/change_detection_signals_in_zones_spec.ts +++ b/packages/core/test/acceptance/change_detection_signals_in_zones_spec.ts @@ -7,7 +7,7 @@ */ import {NgFor, NgIf} from '@angular/common'; -import {ChangeDetectionStrategy, ChangeDetectorRef, Component, Directive, Input, signal, untracked, ViewChild} from '@angular/core'; +import {ChangeDetectionStrategy,inject, ChangeDetectorRef, Component, Directive, Input, signal, untracked, ViewChild} from '@angular/core'; import {TestBed} from '@angular/core/testing'; describe('CheckAlways components', () => { @@ -483,7 +483,7 @@ describe('OnPush components with signals', () => { expect(fixture.nativeElement.outerHTML).not.toContain('blue'); }); - it('should warn when writing to signals during change-detecting a given template, in advance()', + it('should be able to write to signals during change-detecting a given template, in advance()', () => { const counter = signal(0); @@ -509,49 +509,63 @@ describe('OnPush components with signals', () => { counter = counter; } - const consoleWarnSpy = spyOn(console, 'warn').and.callThrough(); - const fixture = TestBed.createComponent(TestCmp); - fixture.detectChanges(false); - expect(consoleWarnSpy) - .toHaveBeenCalledWith(jasmine.stringMatching( - /will likely result in ExpressionChangedAfterItHasBeenChecked/)); + // CheckNoChanges should not throw ExpressionChanged error + // and signal value is updated to latest value with 1 `detectChanges` + fixture.detectChanges(); + expect(fixture.nativeElement.innerText).toContain('1'); + expect(fixture.nativeElement.innerText).toContain('force advance()'); }); - it('should warn when writing to signals during change-detecting a given template, at the end', - () => { - const counter = signal(0); + it('should allow writing to signals during change-detecting a given template, at the end', () => { + const counter = signal(0); - @Directive({ - standalone: true, - selector: '[misunderstood]', - }) - class MisunderstoodDir { - ngOnInit(): void { - counter.update((c) => c + 1); - } - } + @Directive({ + standalone: true, + selector: '[misunderstood]', + }) + class MisunderstoodDir { + ngOnInit(): void { + counter.update((c) => c + 1); + } + } - @Component({ - selector: 'test-component', - standalone: true, - imports: [MisunderstoodDir], - template: ` + @Component({ + selector: 'test-component', + standalone: true, + imports: [MisunderstoodDir], + template: ` {{counter()}}
`, - }) - class TestCmp { - counter = counter; - } + }) + class TestCmp { + counter = counter; + } - const consoleWarnSpy = spyOn(console, 'warn').and.callThrough(); + const fixture = TestBed.createComponent(TestCmp); + // CheckNoChanges should not throw ExpressionChanged error + // and signal value is updated to latest value with 1 `detectChanges` + fixture.detectChanges(); + expect(fixture.nativeElement.innerText).toBe('1'); + }); - const fixture = TestBed.createComponent(TestCmp); - fixture.detectChanges(false); - expect(consoleWarnSpy) - .toHaveBeenCalledWith(jasmine.stringMatching( - /will likely result in ExpressionChangedAfterItHasBeenChecked/)); - }); + it('should allow writing to signals in afterViewInit', () => { + @Component({ + template: '{{loading()}}', + standalone: true, + }) + class MyComp { + loading = signal(true); + // Classic example of what would have caused ExpressionChanged...Error + ngAfterViewInit() { + this.loading.set(false); + } + } + + const fixture = TestBed.createComponent(MyComp); + fixture.detectChanges(); + expect(fixture.nativeElement.innerText).toBe('false'); + }); it('does not refresh view if signal marked dirty but did not change', () => { const val = signal('initial', {equal: () => true}); @@ -749,6 +763,34 @@ describe('OnPush components with signals', () => { expect(fixture.componentInstance.signalChild.afterViewCheckedRuns).toBe(1); }); }); + + it('can refresh the root of change detection if updated after checked', () => { + const val = signal(1); + @Component({ + template: '', + selector: 'child', + standalone: true, + }) + class Child { + ngOnInit() { + val.set(2); + } + } + + @Component({ + template: '{{val()}}', + imports: [Child], + standalone: true, + }) + class SignalComponent { + val = val; + cdr = inject(ChangeDetectorRef); + } + + const fixture = TestBed.createComponent(SignalComponent); + fixture.componentInstance.cdr.detectChanges(); + expect(fixture.nativeElement.innerText).toEqual('2'); + }); }); From 808abb1f34d254f7d5a9722991688ce5dcee9b86 Mon Sep 17 00:00:00 2001 From: Andrew Scott Date: Tue, 24 Oct 2023 15:39:29 -0700 Subject: [PATCH 2/3] refactor(core): Update LView consumer to use only 1 consumer for a component This commit updates the reactive consumer used for `LView`s to be shared between a component and its embedded views. This allows us to use the consumer flag directly for a dirty indicator rather than needing to find a component view for updating its flags. In the future, this will also allow us to effectively poll producers to see if they really changed before refreshing a view. --- packages/core/primitives/signals/index.ts | 2 +- packages/core/src/defer/instructions.ts | 56 +++++---- .../render3/instructions/change_detection.ts | 82 ++++++++++-- .../src/render3/instructions/control_flow.ts | 118 ++++++++++-------- .../core/src/render3/instructions/shared.ts | 50 +++----- .../src/render3/reactive_lview_consumer.ts | 48 ++++--- packages/core/src/render3/util/view_utils.ts | 21 +--- .../change_detection_signals_in_zones_spec.ts | 2 +- 8 files changed, 213 insertions(+), 166 deletions(-) diff --git a/packages/core/primitives/signals/index.ts b/packages/core/primitives/signals/index.ts index b3d35a1f31eb..8d9ebd97b96b 100644 --- a/packages/core/primitives/signals/index.ts +++ b/packages/core/primitives/signals/index.ts @@ -9,7 +9,7 @@ export {createComputed} from './src/computed'; export {defaultEquals, ValueEqualityFn} from './src/equality'; export {setThrowInvalidWriteToSignalError} from './src/errors'; -export {consumerAfterComputation, consumerBeforeComputation, consumerDestroy, getActiveConsumer, isInNotificationPhase, isReactive, producerAccessed, producerNotifyConsumers, producerUpdatesAllowed, producerUpdateValueVersion, Reactive, REACTIVE_NODE, ReactiveNode, setActiveConsumer, SIGNAL} from './src/graph'; +export {consumerAfterComputation, consumerBeforeComputation, consumerDestroy, consumerPollProducersForChange, getActiveConsumer, isInNotificationPhase, isReactive, producerAccessed, producerNotifyConsumers, producerUpdatesAllowed, producerUpdateValueVersion, Reactive, REACTIVE_NODE, ReactiveNode, setActiveConsumer, SIGNAL} from './src/graph'; export {createSignal, setPostSignalSetFn, SignalGetter, signalMutateFn, SignalNode, signalSetFn, signalUpdateFn} from './src/signal'; export {createWatch, Watch, WatchCleanupFn, WatchCleanupRegisterFn} from './src/watch'; export {setAlternateWeakRefImpl} from './src/weak_ref'; diff --git a/packages/core/src/defer/instructions.ts b/packages/core/src/defer/instructions.ts index 5664dd975c4c..ad489700f309 100644 --- a/packages/core/src/defer/instructions.ts +++ b/packages/core/src/defer/instructions.ts @@ -6,6 +6,8 @@ * found in the LICENSE file at https://angular.io/license */ +import {setActiveConsumer} from '@angular/core/primitives/signals'; + import {InjectionToken, Injector} from '../di'; import {RuntimeError, RuntimeErrorCode} from '../errors'; import {findMatchingDehydratedView} from '../hydration/views'; @@ -179,21 +181,26 @@ export function ɵɵdeferWhen(rawValue: unknown) { const lView = getLView(); const bindingIndex = nextBindingIndex(); if (bindingUpdated(lView, bindingIndex, rawValue)) { - const value = Boolean(rawValue); // handle truthy or falsy values - const tNode = getSelectedTNode(); - const lDetails = getLDeferBlockDetails(lView, tNode); - const renderedState = lDetails[DEFER_BLOCK_STATE]; - if (value === false && renderedState === DeferBlockInternalState.Initial) { - // If nothing is rendered yet, render a placeholder (if defined). - renderPlaceholder(lView, tNode); - } else if ( - value === true && - (renderedState === DeferBlockInternalState.Initial || - renderedState === DeferBlockState.Placeholder)) { - // The `when` condition has changed to `true`, trigger defer block loading - // if the block is either in initial (nothing is rendered) or a placeholder - // state. - triggerDeferBlock(lView, tNode); + const prevConsumer = setActiveConsumer(null); + try { + const value = Boolean(rawValue); // handle truthy or falsy values + const tNode = getSelectedTNode(); + const lDetails = getLDeferBlockDetails(lView, tNode); + const renderedState = lDetails[DEFER_BLOCK_STATE]; + if (value === false && renderedState === DeferBlockInternalState.Initial) { + // If nothing is rendered yet, render a placeholder (if defined). + renderPlaceholder(lView, tNode); + } else if ( + value === true && + (renderedState === DeferBlockInternalState.Initial || + renderedState === DeferBlockState.Placeholder)) { + // The `when` condition has changed to `true`, trigger defer block loading + // if the block is either in initial (nothing is rendered) or a placeholder + // state. + triggerDeferBlock(lView, tNode); + } + } finally { + setActiveConsumer(prevConsumer); } } } @@ -207,13 +214,18 @@ export function ɵɵdeferPrefetchWhen(rawValue: unknown) { const bindingIndex = nextBindingIndex(); if (bindingUpdated(lView, bindingIndex, rawValue)) { - const value = Boolean(rawValue); // handle truthy or falsy values - const tView = lView[TVIEW]; - const tNode = getSelectedTNode(); - const tDetails = getTDeferBlockDetails(tView, tNode); - if (value === true && tDetails.loadingState === DeferDependenciesLoadingState.NOT_STARTED) { - // If loading has not been started yet, trigger it now. - triggerPrefetching(tDetails, lView, tNode); + const prevConsumer = setActiveConsumer(null); + try { + const value = Boolean(rawValue); // handle truthy or falsy values + const tView = lView[TVIEW]; + const tNode = getSelectedTNode(); + const tDetails = getTDeferBlockDetails(tView, tNode); + if (value === true && tDetails.loadingState === DeferDependenciesLoadingState.NOT_STARTED) { + // If loading has not been started yet, trigger it now. + triggerPrefetching(tDetails, lView, tNode); + } + } finally { + setActiveConsumer(prevConsumer); } } } diff --git a/packages/core/src/render3/instructions/change_detection.ts b/packages/core/src/render3/instructions/change_detection.ts index 99d8f8ea3b30..d87640677bee 100644 --- a/packages/core/src/render3/instructions/change_detection.ts +++ b/packages/core/src/render3/instructions/change_detection.ts @@ -6,6 +6,8 @@ * found in the LICENSE file at https://angular.io/license */ +import {consumerAfterComputation, consumerBeforeComputation, consumerPollProducersForChange, ReactiveNode} from '@angular/core/primitives/signals'; + import {RuntimeError, RuntimeErrorCode} from '../../errors'; import {assertDefined, assertEqual} from '../../util/assert'; import {assertLContainer} from '../assert'; @@ -13,7 +15,12 @@ import {getComponentViewByInstance} from '../context_discovery'; import {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'; +<<<<<<< HEAD import {CONTEXT, EFFECTS_TO_SCHEDULE, ENVIRONMENT, FLAGS, InitPhaseState, LView, LViewFlags, PARENT, TVIEW, TView, TViewType} from '../interfaces/view'; +======= +import {CONTEXT, ENVIRONMENT, FLAGS, InitPhaseState, LView, LViewFlags, PARENT, REACTIVE_TEMPLATE_CONSUMER, TVIEW, TView, TViewType} from '../interfaces/view'; +import {getOrBorrowReactiveLViewConsumer, maybeReturnReactiveLViewConsumer, ReactiveLViewConsumer} from '../reactive_lview_consumer'; +>>>>>>> 48df4581bb (refactor(core): Update LView consumer to use only 1 consumer for a component) 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'; @@ -49,7 +56,8 @@ export function detectChangesInternal( // descendants views that need to be refreshed due to re-dirtying during the change detection // run, detect changes on the view again. We run change detection in `Targeted` mode to only // refresh views with the `RefreshView` flag. - while (lView[FLAGS] & (LViewFlags.RefreshView | LViewFlags.HasChildViewsToRefresh)) { + while (lView[FLAGS] & (LViewFlags.RefreshView | LViewFlags.HasChildViewsToRefresh) || + lView[REACTIVE_TEMPLATE_CONSUMER]?.dirty) { if (retries === MAXIMUM_REFRESH_RERUNS) { throw new RuntimeError( RuntimeErrorCode.INFINITE_CHANGE_DETECTION, @@ -145,7 +153,18 @@ export function refreshView( !isInCheckNoChangesPass && lView[ENVIRONMENT].inlineEffectRunner?.flush(); + + // Start component reactive context + // - We might already be in a reactive context if this is an embedded view of the host. + // - We might be descending into a view that needs a consumer. enterView(lView); + let prevConsumer: ReactiveNode|null = null; + let currentConsumer: ReactiveLViewConsumer|null = null; + if (!isInCheckNoChangesPass && viewShouldHaveReactiveConsumer(tView)) { + currentConsumer = getOrBorrowReactiveLViewConsumer(lView); + prevConsumer = consumerBeforeComputation(currentConsumer); + } + try { resetPreOrderHookFlags(lView); @@ -273,10 +292,32 @@ export function refreshView( markAncestorsForTraversal(lView); throw e; } finally { + if (currentConsumer !== null) { + consumerAfterComputation(currentConsumer, prevConsumer); + maybeReturnReactiveLViewConsumer(currentConsumer); + } leaveView(); } } +/** + * Indicates if the view should get its own reactive consumer node. + * + * In the current design, all embedded views share a consumer with the component view. This allows + * us to refresh at the component level rather than at a per-view level. In addition, root views get + * their own reactive node because root component will have a host view that executes the + * component's host bindings. This needs to be tracked in a consumer as well. + * + * To get a more granular change detection than per-component, all we would just need to update the + * condition here so that a given view gets a reactive consumer which can become dirty independently + * from its parent component. For example embedded views for signal components could be created with + * a new type "SignalEmbeddedView" and the condition here wouldn't even need updating in order to + * get granular per-view change detection for signal components. + */ +function viewShouldHaveReactiveConsumer(tView: TView) { + return tView.type !== TViewType.Embedded; +} + /** * Goes over embedded views (ones created through ViewContainerRef APIs) and refreshes * them by executing an associated template function. @@ -352,20 +393,35 @@ function detectChangesInView(lView: LView, mode: ChangeDetectionMode) { const isInCheckNoChangesPass = ngDevMode && isInCheckNoChangesMode(); const tView = lView[TVIEW]; const flags = lView[FLAGS]; - - // Flag cleared before change detection runs so that the view can be re-marked for traversal if - // necessary. + const consumer = lView[REACTIVE_TEMPLATE_CONSUMER]; + + // Refresh CheckAlways views in Global mode. + let shouldRefreshView: boolean = + !!(mode === ChangeDetectionMode.Global && flags & LViewFlags.CheckAlways); + + // Refresh Dirty views in Global mode, as long as we're not in checkNoChanges. + // CheckNoChanges never worked with `OnPush` components because the `Dirty` flag was + // cleared before checkNoChanges ran. Because there is now a loop for to check for + // backwards views, it gives an opportunity for `OnPush` components to be marked `Dirty` + // before the CheckNoChanges pass. We don't want existing errors that are hidden by the + // current CheckNoChanges bug to surface when making unrelated changes. + shouldRefreshView ||= !!( + flags & LViewFlags.Dirty && mode === ChangeDetectionMode.Global && (!isInCheckNoChangesPass || RUN_IN_CHECK_NO_CHANGES_ANYWAY)); + + // Always refresh views marked for refresh, regardless of mode. + shouldRefreshView ||= !!(flags & LViewFlags.RefreshView); + + // Refresh views when they have a dirty reactive consumer, regardless of mode. + shouldRefreshView ||= !!consumer?.dirty; + + // Mark the Flags and `ReactiveNode` as not dirty before refreshing the component, so that they + // can be re-dirtied during the refresh process. + if (consumer) { + consumer.dirty = false; + } lView[FLAGS] &= ~(LViewFlags.HasChildViewsToRefresh | LViewFlags.RefreshView); - if ((flags & LViewFlags.CheckAlways && mode === ChangeDetectionMode.Global) || - (flags & LViewFlags.Dirty && mode === ChangeDetectionMode.Global && - // CheckNoChanges never worked with `OnPush` components because the `Dirty` flag was cleared - // before checkNoChanges ran. Because there is now a loop for to check for backwards views, - // it gives an opportunity for `OnPush` components to be marked `Dirty` before the - // CheckNoChanges pass. We don't want existing errors that are hidden by the current - // CheckNoChanges bug to surface when making unrelated changes. - (!isInCheckNoChangesPass || RUN_IN_CHECK_NO_CHANGES_ANYWAY)) || - flags & LViewFlags.RefreshView) { + if (shouldRefreshView) { refreshView(tView, lView, tView.template, lView[CONTEXT]); } else if (flags & LViewFlags.HasChildViewsToRefresh) { detectChangesInEmbeddedViews(lView, ChangeDetectionMode.Targeted); diff --git a/packages/core/src/render3/instructions/control_flow.ts b/packages/core/src/render3/instructions/control_flow.ts index 2c44e9ad8fcb..bfb02fcf5d36 100644 --- a/packages/core/src/render3/instructions/control_flow.ts +++ b/packages/core/src/render3/instructions/control_flow.ts @@ -6,6 +6,8 @@ * found in the LICENSE file at https://angular.io/license */ +import {setActiveConsumer} from '@angular/core/primitives/signals'; + import {TrackByFunction} from '../../change_detection'; import {DehydratedContainerView} from '../../hydration/interfaces'; import {findMatchingDehydratedView} from '../../hydration/views'; @@ -48,22 +50,27 @@ export function ɵɵconditional(containerIndex: number, matchingTemplateIndex const viewInContainerIdx = 0; if (bindingUpdated(hostLView, bindingIndex, matchingTemplateIndex)) { - // The index of the view to show changed - remove the previously displayed one - // (it is a noop if there are no active views in a container). - removeLViewFromLContainer(lContainer, viewInContainerIdx); - - // Index -1 is a special case where none of the conditions evaluates to - // a truthy value and as the consequence we've got no view to show. - if (matchingTemplateIndex !== -1) { - const templateTNode = getExistingTNode(hostLView[TVIEW], matchingTemplateIndex); - - const dehydratedView = findMatchingDehydratedView(lContainer, templateTNode.tView!.ssrId); - const embeddedLView = - createAndRenderEmbeddedLView(hostLView, templateTNode, value, {dehydratedView}); - - addLViewToLContainer( - lContainer, embeddedLView, viewInContainerIdx, - shouldAddViewToDom(templateTNode, dehydratedView)); + const prevConsumer = setActiveConsumer(null); + try { + // The index of the view to show changed - remove the previously displayed one + // (it is a noop if there are no active views in a container). + removeLViewFromLContainer(lContainer, viewInContainerIdx); + + // Index -1 is a special case where none of the conditions evaluates to + // a truthy value and as the consequence we've got no view to show. + if (matchingTemplateIndex !== -1) { + const templateTNode = getExistingTNode(hostLView[TVIEW], matchingTemplateIndex); + + const dehydratedView = findMatchingDehydratedView(lContainer, templateTNode.tView!.ssrId); + const embeddedLView = + createAndRenderEmbeddedLView(hostLView, templateTNode, value, {dehydratedView}); + + addLViewToLContainer( + lContainer, embeddedLView, viewInContainerIdx, + shouldAddViewToDom(templateTNode, dehydratedView)); + } + } finally { + setActiveConsumer(prevConsumer); } } else { // We might keep displaying the same template but the actual value of the expression could have @@ -238,46 +245,51 @@ class LiveCollectionLContainerImpl extends */ export function ɵɵrepeater( metadataSlotIdx: number, collection: Iterable|undefined|null): void { - const hostLView = getLView(); - const hostTView = hostLView[TVIEW]; - const metadata = hostLView[HEADER_OFFSET + metadataSlotIdx] as RepeaterMetadata; - - if (metadata.liveCollection === undefined) { - const containerIndex = metadataSlotIdx + 1; - const lContainer = getLContainer(hostLView, HEADER_OFFSET + containerIndex); - const itemTemplateTNode = getExistingTNode(hostTView, containerIndex); - metadata.liveCollection = - new LiveCollectionLContainerImpl(lContainer, hostLView, itemTemplateTNode); - } else { - metadata.liveCollection.reset(); - } + const prevConsumer = setActiveConsumer(null); + try { + const hostLView = getLView(); + const hostTView = hostLView[TVIEW]; + const metadata = hostLView[HEADER_OFFSET + metadataSlotIdx] as RepeaterMetadata; + + if (metadata.liveCollection === undefined) { + const containerIndex = metadataSlotIdx + 1; + const lContainer = getLContainer(hostLView, HEADER_OFFSET + containerIndex); + const itemTemplateTNode = getExistingTNode(hostTView, containerIndex); + metadata.liveCollection = + new LiveCollectionLContainerImpl(lContainer, hostLView, itemTemplateTNode); + } else { + metadata.liveCollection.reset(); + } - const liveCollection = metadata.liveCollection; - reconcile(liveCollection, collection, metadata.trackByFn); - - // moves in the container might caused context's index to get out of order, re-adjust if needed - liveCollection.updateIndexes(); - - // handle empty blocks - if (metadata.hasEmptyBlock) { - const bindingIndex = nextBindingIndex(); - const isCollectionEmpty = liveCollection.length === 0; - if (bindingUpdated(hostLView, bindingIndex, isCollectionEmpty)) { - const emptyTemplateIndex = metadataSlotIdx + 2; - const lContainerForEmpty = getLContainer(hostLView, HEADER_OFFSET + emptyTemplateIndex); - if (isCollectionEmpty) { - const emptyTemplateTNode = getExistingTNode(hostTView, emptyTemplateIndex); - const dehydratedView = - findMatchingDehydratedView(lContainerForEmpty, emptyTemplateTNode.tView!.ssrId); - const embeddedLView = createAndRenderEmbeddedLView( - hostLView, emptyTemplateTNode, undefined, {dehydratedView}); - addLViewToLContainer( - lContainerForEmpty, embeddedLView, 0, - shouldAddViewToDom(emptyTemplateTNode, dehydratedView)); - } else { - removeLViewFromLContainer(lContainerForEmpty, 0); + const liveCollection = metadata.liveCollection; + reconcile(liveCollection, collection, metadata.trackByFn); + + // moves in the container might caused context's index to get out of order, re-adjust if needed + liveCollection.updateIndexes(); + + // handle empty blocks + if (metadata.hasEmptyBlock) { + const bindingIndex = nextBindingIndex(); + const isCollectionEmpty = liveCollection.length === 0; + if (bindingUpdated(hostLView, bindingIndex, isCollectionEmpty)) { + const emptyTemplateIndex = metadataSlotIdx + 2; + const lContainerForEmpty = getLContainer(hostLView, HEADER_OFFSET + emptyTemplateIndex); + if (isCollectionEmpty) { + const emptyTemplateTNode = getExistingTNode(hostTView, emptyTemplateIndex); + const dehydratedView = + findMatchingDehydratedView(lContainerForEmpty, emptyTemplateTNode.tView!.ssrId); + const embeddedLView = createAndRenderEmbeddedLView( + hostLView, emptyTemplateTNode, undefined, {dehydratedView}); + addLViewToLContainer( + lContainerForEmpty, embeddedLView, 0, + shouldAddViewToDom(emptyTemplateTNode, dehydratedView)); + } else { + removeLViewFromLContainer(lContainerForEmpty, 0); + } } } + } finally { + setActiveConsumer(prevConsumer); } } diff --git a/packages/core/src/render3/instructions/shared.ts b/packages/core/src/render3/instructions/shared.ts index c74c707c2ab9..d9f874ab7e9c 100644 --- a/packages/core/src/render3/instructions/shared.ts +++ b/packages/core/src/render3/instructions/shared.ts @@ -43,7 +43,6 @@ import {assertPureTNodeType, assertTNodeType} from '../node_assert'; import {clearElementContents, updateTextNode} from '../node_manipulation'; import {isInlineTemplate, isNodeMatchingSelectorList} from '../node_selector_matcher'; import {profiler, ProfilerEvent} from '../profiler'; -import {getReactiveLViewConsumer} from '../reactive_lview_consumer'; import {getBindingsEnabled, getCurrentDirectiveIndex, getCurrentParentTNode, getCurrentTNodePlaceholderOk, getSelectedIndex, isCurrentTNodeParent, isInCheckNoChangesMode, isInI18nBlock, isInSkipHydrationBlock, setBindingRootForHostBindings, setCurrentDirectiveIndex, setCurrentQueryIndex, setCurrentTNode, setSelectedIndex} from '../state'; import {NO_CHANGE} from '../tokens'; import {mergeHostAttrs} from '../util/attrs_utils'; @@ -67,7 +66,6 @@ import {handleUnknownPropertyError, isPropertyValid, matchingSchemas} from './el export function processHostBindingOpCodes(tView: TView, lView: LView): void { const hostBindingOpCodes = tView.hostBindingOpCodes; if (hostBindingOpCodes === null) return; - const consumer = getReactiveLViewConsumer(lView, REACTIVE_HOST_BINDING_CONSUMER); try { for (let i = 0; i < hostBindingOpCodes.length; i++) { const opCode = hostBindingOpCodes[i] as number; @@ -80,14 +78,8 @@ export function processHostBindingOpCodes(tView: TView, lView: LView): void { const bindingRootIndx = hostBindingOpCodes[++i] as number; const hostBindingFn = hostBindingOpCodes[++i] as HostBindingsFunction; setBindingRootForHostBindings(bindingRootIndx, directiveIdx); - consumer.dirty = false; - const prevConsumer = consumerBeforeComputation(consumer); - try { - const context = lView[directiveIdx]; - hostBindingFn(RenderFlags.Update, context); - } finally { - consumerAfterComputation(consumer, prevConsumer); - } + const context = lView[directiveIdx]; + hostBindingFn(RenderFlags.Update, context); } } } finally { @@ -253,7 +245,6 @@ export function allocExpando( export function executeTemplate( tView: TView, lView: LView, templateFn: ComponentTemplate, rf: RenderFlags, context: T) { - const consumer = getReactiveLViewConsumer(lView, REACTIVE_TEMPLATE_CONSUMER); const prevSelectedIndex = getSelectedIndex(); const isUpdatePhase = rf & RenderFlags.Update; try { @@ -267,16 +258,7 @@ export function executeTemplate( const preHookType = isUpdatePhase ? ProfilerEvent.TemplateUpdateStart : ProfilerEvent.TemplateCreateStart; profiler(preHookType, context as unknown as {}); - const effectiveConsumer = isUpdatePhase ? consumer : null; - const prevConsumer = consumerBeforeComputation(effectiveConsumer); - try { - if (effectiveConsumer !== null) { - effectiveConsumer.dirty = false; - } - templateFn(rf, context); - } finally { - consumerAfterComputation(effectiveConsumer, prevConsumer); - } + templateFn(rf, context); } finally { setSelectedIndex(prevSelectedIndex); @@ -1426,17 +1408,23 @@ export function createLContainer( export function refreshContentQueries(tView: TView, lView: LView): void { const contentQueries = tView.contentQueries; if (contentQueries !== null) { - for (let i = 0; i < contentQueries.length; i += 2) { - const queryStartIdx = contentQueries[i]; - const directiveDefIdx = contentQueries[i + 1]; - if (directiveDefIdx !== -1) { - const directiveDef = tView.data[directiveDefIdx] as DirectiveDef; - ngDevMode && assertDefined(directiveDef, 'DirectiveDef not found.'); - ngDevMode && - assertDefined(directiveDef.contentQueries, 'contentQueries function should be defined'); - setCurrentQueryIndex(queryStartIdx); - directiveDef.contentQueries!(RenderFlags.Update, lView[directiveDefIdx], directiveDefIdx); + const prevConsumer = setActiveConsumer(null); + try { + for (let i = 0; i < contentQueries.length; i += 2) { + const queryStartIdx = contentQueries[i]; + const directiveDefIdx = contentQueries[i + 1]; + if (directiveDefIdx !== -1) { + const directiveDef = tView.data[directiveDefIdx] as DirectiveDef; + ngDevMode && assertDefined(directiveDef, 'DirectiveDef not found.'); + ngDevMode && + assertDefined( + directiveDef.contentQueries, 'contentQueries function should be defined'); + setCurrentQueryIndex(queryStartIdx); + directiveDef.contentQueries!(RenderFlags.Update, lView[directiveDefIdx], directiveDefIdx); + } } + } finally { + setActiveConsumer(prevConsumer); } } } diff --git a/packages/core/src/render3/reactive_lview_consumer.ts b/packages/core/src/render3/reactive_lview_consumer.ts index 979b6ad50841..30bd0dc812fc 100644 --- a/packages/core/src/render3/reactive_lview_consumer.ts +++ b/packages/core/src/render3/reactive_lview_consumer.ts @@ -9,11 +9,11 @@ import {REACTIVE_NODE, ReactiveNode} from '@angular/core/primitives/signals'; import {LView, REACTIVE_HOST_BINDING_CONSUMER, REACTIVE_TEMPLATE_CONSUMER} from './interfaces/view'; -import {markViewDirtyFromSignal} from './util/view_utils'; +import {markAncestorsForTraversal} from './util/view_utils'; -let currentConsumer: ReactiveLViewConsumer|null = null; +let freeConsumers: ReactiveLViewConsumer[] = []; export interface ReactiveLViewConsumer extends ReactiveNode { - lView: LView; + lView: LView|null; slot: typeof REACTIVE_TEMPLATE_CONSUMER|typeof REACTIVE_HOST_BINDING_CONSUMER; } @@ -22,35 +22,33 @@ export interface ReactiveLViewConsumer extends ReactiveNode { * Sometimes, a previously created consumer may be reused, in order to save on allocations. In that * case, the LView will be updated. */ -export function getReactiveLViewConsumer( - lView: LView, slot: typeof REACTIVE_TEMPLATE_CONSUMER|typeof REACTIVE_HOST_BINDING_CONSUMER): - ReactiveLViewConsumer { - return lView[slot] ?? getOrCreateCurrentLViewConsumer(lView, slot); +export function getOrBorrowReactiveLViewConsumer(lView: LView): ReactiveLViewConsumer { + return lView[REACTIVE_TEMPLATE_CONSUMER] ?? borrowReactiveLViewConsumer(lView); +} + +function borrowReactiveLViewConsumer(lView: LView): ReactiveLViewConsumer { + const consumer: ReactiveLViewConsumer = + freeConsumers.pop() ?? Object.create(REACTIVE_LVIEW_CONSUMER_NODE); + consumer.lView = lView; + return consumer; +} + +export function maybeReturnReactiveLViewConsumer(consumer: ReactiveLViewConsumer): void { + if (consumer.lView![REACTIVE_TEMPLATE_CONSUMER] === consumer) { + // The consumer got committed. + return; + } + consumer.lView = null; + freeConsumers.push(consumer); } const REACTIVE_LVIEW_CONSUMER_NODE: Omit = { ...REACTIVE_NODE, consumerIsAlwaysLive: true, consumerMarkedDirty: (node: ReactiveLViewConsumer) => { - markViewDirtyFromSignal(node.lView); + markAncestorsForTraversal(node.lView!); }, consumerOnSignalRead(this: ReactiveLViewConsumer): void { - if (currentConsumer !== this) { - return; - } - this.lView[this.slot] = currentConsumer; - currentConsumer = null; + this.lView![REACTIVE_TEMPLATE_CONSUMER] = this; }, }; - -function createLViewConsumer(): ReactiveLViewConsumer { - return Object.create(REACTIVE_LVIEW_CONSUMER_NODE); -} - -function getOrCreateCurrentLViewConsumer( - lView: LView, slot: typeof REACTIVE_TEMPLATE_CONSUMER|typeof REACTIVE_HOST_BINDING_CONSUMER) { - currentConsumer ??= createLViewConsumer(); - currentConsumer.lView = lView; - currentConsumer.slot = slot; - return currentConsumer; -} diff --git a/packages/core/src/render3/util/view_utils.ts b/packages/core/src/render3/util/view_utils.ts index da88a183d115..8e792824f4b1 100644 --- a/packages/core/src/render3/util/view_utils.ts +++ b/packages/core/src/render3/util/view_utils.ts @@ -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_COMPONENT_VIEW, DECLARATION_VIEW, FLAGS, HEADER_OFFSET, HOST, LView, LViewFlags, ON_DESTROY_HOOKS, PARENT, PREORDER_HOOK_FLAGS, PreOrderHookFlags, TData, TView} from '../interfaces/view'; +import {DECLARATION_VIEW, FLAGS, HEADER_OFFSET, HOST, LView, LViewFlags, ON_DESTROY_HOOKS, PARENT, PREORDER_HOOK_FLAGS, PreOrderHookFlags, TData, TView} from '../interfaces/view'; @@ -241,25 +241,6 @@ export function markAncestorsForTraversal(lView: LView) { } } -/** - * Marks the component or root view of an LView for refresh. - * - * This function locates the declaration component view of a given LView and marks it for refresh. - * With this, we get component-level change detection granularity. Marking the `LView` itself for - * refresh would be view-level granularity. - * - * Note that when an LView is a root view, the DECLARATION_COMPONENT_VIEW will be the root view - * itself. This is a bit confusing since the TView.type is `Root`, rather than `Component`, but this - * is actually what we need for host bindings in a root view. - */ -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. */ diff --git a/packages/core/test/acceptance/change_detection_signals_in_zones_spec.ts b/packages/core/test/acceptance/change_detection_signals_in_zones_spec.ts index 1303ba76c2d0..d97c011d8c9c 100644 --- a/packages/core/test/acceptance/change_detection_signals_in_zones_spec.ts +++ b/packages/core/test/acceptance/change_detection_signals_in_zones_spec.ts @@ -7,7 +7,7 @@ */ import {NgFor, NgIf} from '@angular/common'; -import {ChangeDetectionStrategy,inject, ChangeDetectorRef, Component, Directive, Input, signal, untracked, ViewChild} from '@angular/core'; +import {ChangeDetectionStrategy, ChangeDetectorRef, Component, computed, Directive, inject, Input, signal, ViewChild} from '@angular/core'; import {TestBed} from '@angular/core/testing'; describe('CheckAlways components', () => { From 7bd221f93279c082c19bede02e22bd25f1a15b3e Mon Sep 17 00:00:00 2001 From: Andrew Scott Date: Thu, 26 Oct 2023 15:55:37 -0700 Subject: [PATCH 3/3] refactor(core): Do not refresh view if producers did not actually change 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 --- .../core/primitives/signals/index.md | 3 ++ .../render3/instructions/change_detection.ts | 13 +++---- .../core/src/render3/instructions/shared.ts | 2 +- packages/core/src/render3/interfaces/view.ts | 6 ---- .../core/src/render3/node_manipulation.ts | 3 +- .../src/render3/reactive_lview_consumer.ts | 4 +-- .../change_detection_signals_in_zones_spec.ts | 34 ++++++++++++++++++- .../bundle.golden_symbols.json | 25 ++++++-------- .../animations/bundle.golden_symbols.json | 25 ++++++-------- .../cyclic_import/bundle.golden_symbols.json | 25 ++++++-------- .../bundling/defer/bundle.golden_symbols.json | 25 ++++++-------- .../forms_reactive/bundle.golden_symbols.json | 25 ++++++-------- .../bundle.golden_symbols.json | 25 ++++++-------- .../hello_world/bundle.golden_symbols.json | 25 ++++++-------- .../hydration/bundle.golden_symbols.json | 25 ++++++-------- .../router/bundle.golden_symbols.json | 25 ++++++-------- .../bundle.golden_symbols.json | 25 ++++++-------- .../bundling/todo/bundle.golden_symbols.json | 25 ++++++-------- 18 files changed, 166 insertions(+), 174 deletions(-) diff --git a/goldens/public-api/core/primitives/signals/index.md b/goldens/public-api/core/primitives/signals/index.md index 352cee5631e9..9d6988759fee 100644 --- a/goldens/public-api/core/primitives/signals/index.md +++ b/goldens/public-api/core/primitives/signals/index.md @@ -13,6 +13,9 @@ export function consumerBeforeComputation(node: ReactiveNode | null): ReactiveNo // @public export function consumerDestroy(node: ReactiveNode): void; +// @public +export function consumerPollProducersForChange(node: ReactiveNode): boolean; + // @public export function createComputed(computation: () => T): ComputedGetter; diff --git a/packages/core/src/render3/instructions/change_detection.ts b/packages/core/src/render3/instructions/change_detection.ts index d87640677bee..5be778958d27 100644 --- a/packages/core/src/render3/instructions/change_detection.ts +++ b/packages/core/src/render3/instructions/change_detection.ts @@ -15,12 +15,8 @@ import {getComponentViewByInstance} from '../context_discovery'; import {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'; -<<<<<<< HEAD -import {CONTEXT, EFFECTS_TO_SCHEDULE, ENVIRONMENT, FLAGS, InitPhaseState, LView, LViewFlags, PARENT, TVIEW, TView, TViewType} from '../interfaces/view'; -======= -import {CONTEXT, ENVIRONMENT, FLAGS, InitPhaseState, LView, LViewFlags, PARENT, REACTIVE_TEMPLATE_CONSUMER, TVIEW, TView, TViewType} from '../interfaces/view'; +import {CONTEXT, EFFECTS_TO_SCHEDULE, ENVIRONMENT, FLAGS, InitPhaseState, LView, LViewFlags, PARENT, REACTIVE_TEMPLATE_CONSUMER, TVIEW, TView, TViewType} from '../interfaces/view'; import {getOrBorrowReactiveLViewConsumer, maybeReturnReactiveLViewConsumer, ReactiveLViewConsumer} from '../reactive_lview_consumer'; ->>>>>>> 48df4581bb (refactor(core): Update LView consumer to use only 1 consumer for a component) 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'; @@ -405,14 +401,15 @@ function detectChangesInView(lView: LView, mode: ChangeDetectionMode) { // backwards views, it gives an opportunity for `OnPush` components to be marked `Dirty` // before the CheckNoChanges pass. We don't want existing errors that are hidden by the // current CheckNoChanges bug to surface when making unrelated changes. - shouldRefreshView ||= !!( - flags & LViewFlags.Dirty && mode === ChangeDetectionMode.Global && (!isInCheckNoChangesPass || RUN_IN_CHECK_NO_CHANGES_ANYWAY)); + shouldRefreshView ||= + !!(flags & LViewFlags.Dirty && mode === ChangeDetectionMode.Global && + (!isInCheckNoChangesPass || RUN_IN_CHECK_NO_CHANGES_ANYWAY)); // Always refresh views marked for refresh, regardless of mode. shouldRefreshView ||= !!(flags & LViewFlags.RefreshView); // Refresh views when they have a dirty reactive consumer, regardless of mode. - shouldRefreshView ||= !!consumer?.dirty; + shouldRefreshView ||= !!(consumer?.dirty && consumerPollProducersForChange(consumer)); // Mark the Flags and `ReactiveNode` as not dirty before refreshing the component, so that they // can be re-dirtied during the refresh process. diff --git a/packages/core/src/render3/instructions/shared.ts b/packages/core/src/render3/instructions/shared.ts index d9f874ab7e9c..bf72fb514148 100644 --- a/packages/core/src/render3/instructions/shared.ts +++ b/packages/core/src/render3/instructions/shared.ts @@ -38,7 +38,7 @@ import {Renderer} from '../interfaces/renderer'; import {RComment, RElement, RNode, RText} from '../interfaces/renderer_dom'; import {SanitizerFn} from '../interfaces/sanitization'; import {isComponentDef, isComponentHost, isContentQueryHost} from '../interfaces/type_checks'; -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'; +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'; import {assertPureTNodeType, assertTNodeType} from '../node_assert'; import {clearElementContents, updateTextNode} from '../node_manipulation'; import {isInlineTemplate, isNodeMatchingSelectorList} from '../node_selector_matcher'; diff --git a/packages/core/src/render3/interfaces/view.ts b/packages/core/src/render3/interfaces/view.ts index dc6048ba7d04..31fcdfc9b2e9 100644 --- a/packages/core/src/render3/interfaces/view.ts +++ b/packages/core/src/render3/interfaces/view.ts @@ -59,7 +59,6 @@ export const EMBEDDED_VIEW_INJECTOR = 20; export const ON_DESTROY_HOOKS = 21; export const EFFECTS_TO_SCHEDULE = 22; export const REACTIVE_TEMPLATE_CONSUMER = 23; -export const REACTIVE_HOST_BINDING_CONSUMER = 24; /** * Size of LView's header. Necessary to adjust for it when setting slots. @@ -356,11 +355,6 @@ export interface LView extends Array { * if any signals were read. */ [REACTIVE_TEMPLATE_CONSUMER]: ReactiveLViewConsumer|null; - - /** - * Same as REACTIVE_TEMPLATE_CONSUMER, but for the host bindings of the LView. - */ - [REACTIVE_HOST_BINDING_CONSUMER]: ReactiveLViewConsumer|null; } /** diff --git a/packages/core/src/render3/node_manipulation.ts b/packages/core/src/render3/node_manipulation.ts index 9f317b78261a..cd778a2c529e 100644 --- a/packages/core/src/render3/node_manipulation.ts +++ b/packages/core/src/render3/node_manipulation.ts @@ -26,7 +26,7 @@ import {TElementNode, TIcuContainerNode, TNode, TNodeFlags, TNodeType, TProjecti import {Renderer} from './interfaces/renderer'; import {RComment, RElement, RNode, RTemplate, RText} from './interfaces/renderer_dom'; import {isLContainer, isLView} from './interfaces/type_checks'; -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'; +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'; import {assertTNodeType} from './node_assert'; import {profiler, ProfilerEvent} from './profiler'; import {setUpAttributes} from './util/attrs_utils'; @@ -375,7 +375,6 @@ export function destroyLView(tView: TView, lView: LView) { const renderer = lView[RENDERER]; lView[REACTIVE_TEMPLATE_CONSUMER] && consumerDestroy(lView[REACTIVE_TEMPLATE_CONSUMER]); - lView[REACTIVE_HOST_BINDING_CONSUMER] && consumerDestroy(lView[REACTIVE_HOST_BINDING_CONSUMER]); if (renderer.destroyNode) { applyView(tView, lView, renderer, WalkTNodeTreeAction.Destroy, null, null); diff --git a/packages/core/src/render3/reactive_lview_consumer.ts b/packages/core/src/render3/reactive_lview_consumer.ts index 30bd0dc812fc..aae10bc9835a 100644 --- a/packages/core/src/render3/reactive_lview_consumer.ts +++ b/packages/core/src/render3/reactive_lview_consumer.ts @@ -8,13 +8,13 @@ import {REACTIVE_NODE, ReactiveNode} from '@angular/core/primitives/signals'; -import {LView, REACTIVE_HOST_BINDING_CONSUMER, REACTIVE_TEMPLATE_CONSUMER} from './interfaces/view'; +import {LView, REACTIVE_TEMPLATE_CONSUMER} from './interfaces/view'; import {markAncestorsForTraversal} from './util/view_utils'; let freeConsumers: ReactiveLViewConsumer[] = []; export interface ReactiveLViewConsumer extends ReactiveNode { lView: LView|null; - slot: typeof REACTIVE_TEMPLATE_CONSUMER|typeof REACTIVE_HOST_BINDING_CONSUMER; + slot: typeof REACTIVE_TEMPLATE_CONSUMER; } /** diff --git a/packages/core/test/acceptance/change_detection_signals_in_zones_spec.ts b/packages/core/test/acceptance/change_detection_signals_in_zones_spec.ts index d97c011d8c9c..f580278d2171 100644 --- a/packages/core/test/acceptance/change_detection_signals_in_zones_spec.ts +++ b/packages/core/test/acceptance/change_detection_signals_in_zones_spec.ts @@ -204,6 +204,38 @@ describe('OnPush components with signals', () => { expect(instance.value()).toBe('new'); }); + it('does not refresh a component when a signal notifies but isn\'t actually updated', () => { + @Component({ + template: `{{memo()}}{{incrementTemplateExecutions()}}`, + changeDetection: ChangeDetectionStrategy.OnPush, + standalone: true, + }) + class OnPushCmp { + numTemplateExecutions = 0; + value = signal({value: 'initial'}); + memo = computed(() => this.value().value, {equal: Object.is}); + 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('initial'); + + instance.value.update(v => ({...v})); + fixture.detectChanges(); + expect(instance.numTemplateExecutions).toBe(1); + + instance.value.update(v => ({value: 'new'})); + fixture.detectChanges(); + expect(instance.numTemplateExecutions).toBe(2); + expect(fixture.nativeElement.textContent.trim()).toEqual('new'); + }); + it('should not mark components as dirty when signal is read in a constructor of a child component', () => { const state = signal('initial'); @@ -778,7 +810,7 @@ describe('OnPush components with signals', () => { } @Component({ - template: '{{val()}}', + template: '{{val()}} ', imports: [Child], standalone: true, }) diff --git a/packages/core/test/bundling/animations-standalone/bundle.golden_symbols.json b/packages/core/test/bundling/animations-standalone/bundle.golden_symbols.json index e7f432226bb0..4ef519fa3f22 100644 --- a/packages/core/test/bundling/animations-standalone/bundle.golden_symbols.json +++ b/packages/core/test/bundling/animations-standalone/bundle.golden_symbols.json @@ -738,16 +738,10 @@ "name": "connectableObservableDescriptor" }, { - "name": "consumerAfterComputation" - }, - { - "name": "consumerBeforeComputation" - }, - { - "name": "consumerDestroy" + "name": "consumerIsLive" }, { - "name": "consumerIsLive" + "name": "consumerPollProducersForChange" }, { "name": "containsElement" @@ -794,9 +788,6 @@ { "name": "createTransitionInstruction" }, - { - "name": "currentConsumer" - }, { "name": "dashCaseToCamelCase" }, @@ -845,6 +836,9 @@ { "name": "enterView" }, + { + "name": "epoch" + }, { "name": "eraseStyles" }, @@ -887,6 +881,9 @@ { "name": "forwardRef" }, + { + "name": "freeConsumers" + }, { "name": "from" }, @@ -1007,9 +1004,6 @@ { "name": "getPromiseCtor" }, - { - "name": "getReactiveLViewConsumer" - }, { "name": "getSimpleChangesStore" }, @@ -1298,6 +1292,9 @@ { "name": "producerRemoveLiveConsumerAtIndex" }, + { + "name": "producerUpdateValueVersion" + }, { "name": "profiler" }, diff --git a/packages/core/test/bundling/animations/bundle.golden_symbols.json b/packages/core/test/bundling/animations/bundle.golden_symbols.json index 09b6902a01a1..f10f1fc6a7ab 100644 --- a/packages/core/test/bundling/animations/bundle.golden_symbols.json +++ b/packages/core/test/bundling/animations/bundle.golden_symbols.json @@ -795,16 +795,10 @@ "name": "connectableObservableDescriptor" }, { - "name": "consumerAfterComputation" - }, - { - "name": "consumerBeforeComputation" - }, - { - "name": "consumerDestroy" + "name": "consumerIsLive" }, { - "name": "consumerIsLive" + "name": "consumerPollProducersForChange" }, { "name": "containsElement" @@ -857,9 +851,6 @@ { "name": "createTransitionInstruction" }, - { - "name": "currentConsumer" - }, { "name": "dashCaseToCamelCase" }, @@ -908,6 +899,9 @@ { "name": "enterView" }, + { + "name": "epoch" + }, { "name": "eraseStyles" }, @@ -950,6 +944,9 @@ { "name": "forwardRef" }, + { + "name": "freeConsumers" + }, { "name": "from" }, @@ -1073,9 +1070,6 @@ { "name": "getPromiseCtor" }, - { - "name": "getReactiveLViewConsumer" - }, { "name": "getSimpleChangesStore" }, @@ -1373,6 +1367,9 @@ { "name": "producerRemoveLiveConsumerAtIndex" }, + { + "name": "producerUpdateValueVersion" + }, { "name": "profiler" }, diff --git a/packages/core/test/bundling/cyclic_import/bundle.golden_symbols.json b/packages/core/test/bundling/cyclic_import/bundle.golden_symbols.json index 40a252cfb8d6..1ed3c3cb3556 100644 --- a/packages/core/test/bundling/cyclic_import/bundle.golden_symbols.json +++ b/packages/core/test/bundling/cyclic_import/bundle.golden_symbols.json @@ -600,16 +600,10 @@ "name": "connectableObservableDescriptor" }, { - "name": "consumerAfterComputation" - }, - { - "name": "consumerBeforeComputation" - }, - { - "name": "consumerDestroy" + "name": "consumerIsLive" }, { - "name": "consumerIsLive" + "name": "consumerPollProducersForChange" }, { "name": "convertToBitFlags" @@ -641,9 +635,6 @@ { "name": "createTView" }, - { - "name": "currentConsumer" - }, { "name": "deepForEach" }, @@ -686,6 +677,9 @@ { "name": "enterView" }, + { + "name": "epoch" + }, { "name": "executeCheckHooks" }, @@ -719,6 +713,9 @@ { "name": "forwardRef" }, + { + "name": "freeConsumers" + }, { "name": "from" }, @@ -836,9 +833,6 @@ { "name": "getPromiseCtor" }, - { - "name": "getReactiveLViewConsumer" - }, { "name": "getSimpleChangesStore" }, @@ -1094,6 +1088,9 @@ { "name": "producerRemoveLiveConsumerAtIndex" }, + { + "name": "producerUpdateValueVersion" + }, { "name": "profiler" }, diff --git a/packages/core/test/bundling/defer/bundle.golden_symbols.json b/packages/core/test/bundling/defer/bundle.golden_symbols.json index fc3dbab8d774..74f54e3673d6 100644 --- a/packages/core/test/bundling/defer/bundle.golden_symbols.json +++ b/packages/core/test/bundling/defer/bundle.golden_symbols.json @@ -681,16 +681,10 @@ "name": "connectableObservableDescriptor" }, { - "name": "consumerAfterComputation" - }, - { - "name": "consumerBeforeComputation" - }, - { - "name": "consumerDestroy" + "name": "consumerIsLive" }, { - "name": "consumerIsLive" + "name": "consumerPollProducersForChange" }, { "name": "convertToBitFlags" @@ -722,9 +716,6 @@ { "name": "createTView" }, - { - "name": "currentConsumer" - }, { "name": "deepForEach" }, @@ -773,6 +764,9 @@ { "name": "enterView" }, + { + "name": "epoch" + }, { "name": "executeCheckHooks" }, @@ -806,6 +800,9 @@ { "name": "forwardRef" }, + { + "name": "freeConsumers" + }, { "name": "from" }, @@ -938,9 +935,6 @@ { "name": "getPromiseCtor" }, - { - "name": "getReactiveLViewConsumer" - }, { "name": "getSelectedIndex" }, @@ -2219,6 +2213,9 @@ { "name": "producerRemoveLiveConsumerAtIndex" }, + { + "name": "producerUpdateValueVersion" + }, { "name": "profiler" }, diff --git a/packages/core/test/bundling/forms_reactive/bundle.golden_symbols.json b/packages/core/test/bundling/forms_reactive/bundle.golden_symbols.json index 7ede144dc40f..0eaab905f8bb 100644 --- a/packages/core/test/bundling/forms_reactive/bundle.golden_symbols.json +++ b/packages/core/test/bundling/forms_reactive/bundle.golden_symbols.json @@ -822,16 +822,10 @@ "name": "connectableObservableDescriptor" }, { - "name": "consumerAfterComputation" - }, - { - "name": "consumerBeforeComputation" - }, - { - "name": "consumerDestroy" + "name": "consumerIsLive" }, { - "name": "consumerIsLive" + "name": "consumerPollProducersForChange" }, { "name": "controlNameBinding" @@ -875,9 +869,6 @@ { "name": "createTView" }, - { - "name": "currentConsumer" - }, { "name": "deepForEach" }, @@ -929,6 +920,9 @@ { "name": "enterView" }, + { + "name": "epoch" + }, { "name": "executeCheckHooks" }, @@ -986,6 +980,9 @@ { "name": "forwardRef" }, + { + "name": "freeConsumers" + }, { "name": "from" }, @@ -1127,9 +1124,6 @@ { "name": "getPromiseCtor" }, - { - "name": "getReactiveLViewConsumer" - }, { "name": "getSelectedIndex" }, @@ -1520,6 +1514,9 @@ { "name": "producerRemoveLiveConsumerAtIndex" }, + { + "name": "producerUpdateValueVersion" + }, { "name": "profiler" }, diff --git a/packages/core/test/bundling/forms_template_driven/bundle.golden_symbols.json b/packages/core/test/bundling/forms_template_driven/bundle.golden_symbols.json index 52c2528d3d1f..da0892f52ebd 100644 --- a/packages/core/test/bundling/forms_template_driven/bundle.golden_symbols.json +++ b/packages/core/test/bundling/forms_template_driven/bundle.golden_symbols.json @@ -795,16 +795,10 @@ "name": "connectableObservableDescriptor" }, { - "name": "consumerAfterComputation" - }, - { - "name": "consumerBeforeComputation" - }, - { - "name": "consumerDestroy" + "name": "consumerIsLive" }, { - "name": "consumerIsLive" + "name": "consumerPollProducersForChange" }, { "name": "controlPath" @@ -845,9 +839,6 @@ { "name": "createTView" }, - { - "name": "currentConsumer" - }, { "name": "deepForEach" }, @@ -899,6 +890,9 @@ { "name": "enterView" }, + { + "name": "epoch" + }, { "name": "executeCheckHooks" }, @@ -953,6 +947,9 @@ { "name": "forwardRef" }, + { + "name": "freeConsumers" + }, { "name": "from" }, @@ -1088,9 +1085,6 @@ { "name": "getPromiseCtor" }, - { - "name": "getReactiveLViewConsumer" - }, { "name": "getSelectedIndex" }, @@ -1484,6 +1478,9 @@ { "name": "producerRemoveLiveConsumerAtIndex" }, + { + "name": "producerUpdateValueVersion" + }, { "name": "profiler" }, diff --git a/packages/core/test/bundling/hello_world/bundle.golden_symbols.json b/packages/core/test/bundling/hello_world/bundle.golden_symbols.json index 1581f1b704a7..2cda661aa131 100644 --- a/packages/core/test/bundling/hello_world/bundle.golden_symbols.json +++ b/packages/core/test/bundling/hello_world/bundle.golden_symbols.json @@ -462,16 +462,10 @@ "name": "connectableObservableDescriptor" }, { - "name": "consumerAfterComputation" - }, - { - "name": "consumerBeforeComputation" - }, - { - "name": "consumerDestroy" + "name": "consumerIsLive" }, { - "name": "consumerIsLive" + "name": "consumerPollProducersForChange" }, { "name": "convertToBitFlags" @@ -500,9 +494,6 @@ { "name": "createTView" }, - { - "name": "currentConsumer" - }, { "name": "deepForEach" }, @@ -545,6 +536,9 @@ { "name": "enterView" }, + { + "name": "epoch" + }, { "name": "executeCheckHooks" }, @@ -566,6 +560,9 @@ { "name": "forwardRef" }, + { + "name": "freeConsumers" + }, { "name": "from" }, @@ -665,9 +662,6 @@ { "name": "getPromiseCtor" }, - { - "name": "getReactiveLViewConsumer" - }, { "name": "getSimpleChangesStore" }, @@ -866,6 +860,9 @@ { "name": "producerRemoveLiveConsumerAtIndex" }, + { + "name": "producerUpdateValueVersion" + }, { "name": "profiler" }, diff --git a/packages/core/test/bundling/hydration/bundle.golden_symbols.json b/packages/core/test/bundling/hydration/bundle.golden_symbols.json index 0cb24e7aab59..31a6088f1d0c 100644 --- a/packages/core/test/bundling/hydration/bundle.golden_symbols.json +++ b/packages/core/test/bundling/hydration/bundle.golden_symbols.json @@ -684,16 +684,10 @@ "name": "connectableObservableDescriptor" }, { - "name": "consumerAfterComputation" - }, - { - "name": "consumerBeforeComputation" - }, - { - "name": "consumerDestroy" + "name": "consumerIsLive" }, { - "name": "consumerIsLive" + "name": "consumerPollProducersForChange" }, { "name": "convertToBitFlags" @@ -722,9 +716,6 @@ { "name": "createTextNode" }, - { - "name": "currentConsumer" - }, { "name": "deepForEach" }, @@ -773,6 +764,9 @@ { "name": "enterView" }, + { + "name": "epoch" + }, { "name": "executeCheckHooks" }, @@ -803,6 +797,9 @@ { "name": "forwardRef" }, + { + "name": "freeConsumers" + }, { "name": "from" }, @@ -920,9 +917,6 @@ { "name": "getPromiseCtor" }, - { - "name": "getReactiveLViewConsumer" - }, { "name": "getSegmentHead" }, @@ -1193,6 +1187,9 @@ { "name": "producerRemoveLiveConsumerAtIndex" }, + { + "name": "producerUpdateValueVersion" + }, { "name": "profiler" }, diff --git a/packages/core/test/bundling/router/bundle.golden_symbols.json b/packages/core/test/bundling/router/bundle.golden_symbols.json index cbda420a4ae5..08c3928266a9 100644 --- a/packages/core/test/bundling/router/bundle.golden_symbols.json +++ b/packages/core/test/bundling/router/bundle.golden_symbols.json @@ -1035,16 +1035,10 @@ "name": "connectableObservableDescriptor" }, { - "name": "consumerAfterComputation" - }, - { - "name": "consumerBeforeComputation" - }, - { - "name": "consumerDestroy" + "name": "consumerIsLive" }, { - "name": "consumerIsLive" + "name": "consumerPollProducersForChange" }, { "name": "containsSegmentGroup" @@ -1124,9 +1118,6 @@ { "name": "createWildcardMatchResult" }, - { - "name": "currentConsumer" - }, { "name": "deactivateRouteAndItsChildren" }, @@ -1217,6 +1208,9 @@ { "name": "enterView" }, + { + "name": "epoch" + }, { "name": "equalArraysOrString" }, @@ -1283,6 +1277,9 @@ { "name": "forwardRef" }, + { + "name": "freeConsumers" + }, { "name": "from" }, @@ -1448,9 +1445,6 @@ { "name": "getPromiseCtor" }, - { - "name": "getReactiveLViewConsumer" - }, { "name": "getSanitizer" }, @@ -1847,6 +1841,9 @@ { "name": "producerRemoveLiveConsumerAtIndex" }, + { + "name": "producerUpdateValueVersion" + }, { "name": "profiler" }, diff --git a/packages/core/test/bundling/standalone_bootstrap/bundle.golden_symbols.json b/packages/core/test/bundling/standalone_bootstrap/bundle.golden_symbols.json index 53f4a0f630ea..a7988d5bc335 100644 --- a/packages/core/test/bundling/standalone_bootstrap/bundle.golden_symbols.json +++ b/packages/core/test/bundling/standalone_bootstrap/bundle.golden_symbols.json @@ -540,16 +540,10 @@ "name": "connectableObservableDescriptor" }, { - "name": "consumerAfterComputation" - }, - { - "name": "consumerBeforeComputation" - }, - { - "name": "consumerDestroy" + "name": "consumerIsLive" }, { - "name": "consumerIsLive" + "name": "consumerPollProducersForChange" }, { "name": "convertToBitFlags" @@ -572,9 +566,6 @@ { "name": "createTView" }, - { - "name": "currentConsumer" - }, { "name": "deepForEach" }, @@ -617,6 +608,9 @@ { "name": "enterView" }, + { + "name": "epoch" + }, { "name": "executeCheckHooks" }, @@ -644,6 +638,9 @@ { "name": "forwardRef" }, + { + "name": "freeConsumers" + }, { "name": "from" }, @@ -749,9 +746,6 @@ { "name": "getPromiseCtor" }, - { - "name": "getReactiveLViewConsumer" - }, { "name": "getSimpleChangesStore" }, @@ -959,6 +953,9 @@ { "name": "producerRemoveLiveConsumerAtIndex" }, + { + "name": "producerUpdateValueVersion" + }, { "name": "profiler" }, diff --git a/packages/core/test/bundling/todo/bundle.golden_symbols.json b/packages/core/test/bundling/todo/bundle.golden_symbols.json index 6e88fdcd4208..8f476765fece 100644 --- a/packages/core/test/bundling/todo/bundle.golden_symbols.json +++ b/packages/core/test/bundling/todo/bundle.golden_symbols.json @@ -714,16 +714,10 @@ "name": "connectableObservableDescriptor" }, { - "name": "consumerAfterComputation" - }, - { - "name": "consumerBeforeComputation" - }, - { - "name": "consumerDestroy" + "name": "consumerIsLive" }, { - "name": "consumerIsLive" + "name": "consumerPollProducersForChange" }, { "name": "convertToBitFlags" @@ -761,9 +755,6 @@ { "name": "createTView" }, - { - "name": "currentConsumer" - }, { "name": "deepForEach" }, @@ -815,6 +806,9 @@ { "name": "enterView" }, + { + "name": "epoch" + }, { "name": "executeCheckHooks" }, @@ -854,6 +848,9 @@ { "name": "forwardRef" }, + { + "name": "freeConsumers" + }, { "name": "from" }, @@ -989,9 +986,6 @@ { "name": "getPromiseCtor" }, - { - "name": "getReactiveLViewConsumer" - }, { "name": "getSelectedIndex" }, @@ -1313,6 +1307,9 @@ { "name": "producerRemoveLiveConsumerAtIndex" }, + { + "name": "producerUpdateValueVersion" + }, { "name": "profiler" },