Skip to content

Commit

Permalink
refactor(core): remove ManualOnPush change detection (#45943)
Browse files Browse the repository at this point in the history 
We've had a TODO to expose ManualOnPush for a long time, but it hasn't moved since then. These changes remove it since it would be easy to re-introduce if we wanted to, it frees up an extra space in the flags bitmap and it removes some `render3` tests that we won't have to migrate to `TestBed`.

PR Close #45943
  • Loading branch information
@crisbeto @AndrewKushnir
crisbeto authored and AndrewKushnir committed May 10, 2022
1 parent d48cfbc commit 74321a4
Show file tree
Hide file tree
Showing 3 changed files with 11 additions and 248 deletions.
8 changes: 2 additions & 6 deletions packages/core/src/render3/instructions/listener.ts
View file
Expand Up @@ -13,7 +13,7 @@ import {PropertyAliasValue, TNode, TNodeFlags, TNodeType} from '../interfaces/no
import {GlobalTargetResolver, isProceduralRenderer, Renderer3} from '../interfaces/renderer';
import {RElement} from '../interfaces/renderer_dom';
import {isDirectiveHost} from '../interfaces/type_checks';
import {CLEANUP, CONTEXT, FLAGS, LView, LViewFlags, RENDERER, TView} from '../interfaces/view';
import {CLEANUP, CONTEXT, LView, RENDERER, TView} from '../interfaces/view';
import {assertTNodeType} from '../node_assert';
import {profiler, ProfilerEvent} from '../profiler';
import {getCurrentDirectiveDef, getCurrentTNode, getLView, getTView} from '../state';
Expand Down Expand Up @@ -266,11 +266,7 @@ function wrapListener(
const startView = tNode.flags & TNodeFlags.isComponentHost ?
getComponentLViewByIndex(tNode.index, lView) :
lView;

// See interfaces/view.ts for more on LViewFlags.ManualOnPush
if ((lView[FLAGS] & LViewFlags.ManualOnPush) === 0) {
markViewDirty(startView);
}
markViewDirty(startView);

let result = executeListenerWithErrorHandling(lView, context, listenerFn, e);
// A just-invoked listener function might have coalesced listeners so we need to check for
Expand Down
35 changes: 9 additions & 26 deletions packages/core/src/render3/interfaces/view.ts
View file
Expand Up @@ -368,50 +368,33 @@ export const enum LViewFlags {
/** Whether this view has default change detection strategy (checks always) or onPush */
CheckAlways = 0b00000010000,

/**
* Whether or not manual change detection is turned on for onPush components.
*
* This is a special mode that only marks components dirty in two cases:
* 1) There has been a change to an @Input property
* 2) `markDirty()` has been called manually by the user
*
* Note that in this mode, the firing of events does NOT mark components
* dirty automatically.
*
* Manual mode is turned off by default for backwards compatibility, as events
* automatically mark OnPush components dirty in View Engine.
*
* TODO: Add a public API to ChangeDetectionStrategy to turn this mode on
*/
ManualOnPush = 0b00000100000,

/** Whether or not this view is currently dirty (needing check) */
Dirty = 0b000001000000,
Dirty = 0b00000100000,

/** Whether or not this view is currently attached to change detection tree. */
Attached = 0b000010000000,
Attached = 0b000001000000,

/** Whether or not this view is destroyed. */
Destroyed = 0b000100000000,
Destroyed = 0b000010000000,

/** Whether or not this view is the root view */
IsRoot = 0b001000000000,
IsRoot = 0b000100000000,

/**
* Whether this moved LView was needs to be refreshed at the insertion location because the
* declaration was dirty.
*/
RefreshTransplantedView = 0b0010000000000,
RefreshTransplantedView = 0b001000000000,

/** Indicates that the view **or any of its ancestors** have an embedded view injector. */
HasEmbeddedViewInjector = 0b0100000000000,
HasEmbeddedViewInjector = 0b0010000000000,

/**
* Index of the current init phase on last 21 bits
*/
IndexWithinInitPhaseIncrementer = 0b01000000000000,
IndexWithinInitPhaseShift = 12,
IndexWithinInitPhaseReset = 0b00111111111111,
IndexWithinInitPhaseIncrementer = 0b0100000000000,
IndexWithinInitPhaseShift = 11,
IndexWithinInitPhaseReset = 0b0011111111111,
}

/**
Expand Down
216 changes: 0 additions & 216 deletions packages/core/test/render3/change_detection_spec.ts
View file
Expand Up @@ -190,222 +190,6 @@ describe('change detection', () => {
}));
});

describe('onPush', () => {
let comp: MyComponent;

class MyComponent implements DoCheck {
/* @Input() */
name = 'Nancy';
doCheckCount = 0;

ngDoCheck(): void {
this.doCheckCount++;
}

onClick() {}

static ɵfac = () => comp = new MyComponent();
static ɵcmp = ɵɵdefineComponent({
type: MyComponent,
selectors: [['my-comp']],
decls: 2,
vars: 2,
/**
* {{ doCheckCount }} - {{ name }}
* <button (click)="onClick()"></button>
*/
template:
(rf: RenderFlags, ctx: MyComponent) => {
if (rf & RenderFlags.Create) {
ɵɵtext(0);
ɵɵelementStart(1, 'button');
{
ɵɵlistener('click', () => {
ctx.onClick();
});
}
ɵɵelementEnd();
}
if (rf & RenderFlags.Update) {
ɵɵtextInterpolate2('', ctx.doCheckCount, ' - ', ctx.name, '');
}
},
changeDetection: ChangeDetectionStrategy.OnPush,
inputs: {name: 'name'}
});
}

describe('Manual mode', () => {
class ManualComponent implements DoCheck {
/* @Input() */
name = 'Nancy';
doCheckCount = 0;

ngDoCheck(): void {
this.doCheckCount++;
}

onClick() {}

static ɵfac = () => comp = new ManualComponent();
static ɵcmp = ɵɵdefineComponent({
type: ManualComponent,
selectors: [['manual-comp']],
decls: 2,
vars: 2,
/**
* {{ doCheckCount }} - {{ name }}
* <button (click)="onClick()"></button>
*/
template:
(rf: RenderFlags, ctx: ManualComponent) => {
if (rf & RenderFlags.Create) {
// This is temporarily the only way to turn on manual change detection
// because public API has not yet been added.
const view = ɵɵgetCurrentView() as any;
view[FLAGS] |= LViewFlags.ManualOnPush;

ɵɵtext(0);
ɵɵelementStart(1, 'button');
{
ɵɵlistener('click', () => {
ctx.onClick();
});
}
ɵɵelementEnd();
}
if (rf & RenderFlags.Update) {
ɵɵtextInterpolate2('', ctx.doCheckCount, ' - ', ctx.name, '');
}
},
changeDetection: ChangeDetectionStrategy.OnPush,
inputs: {name: 'name'}
});
}

class ManualApp {
name: string = 'Nancy';

static ɵfac = () => new ManualApp();
static ɵcmp = ɵɵdefineComponent({
type: ManualApp,
selectors: [['manual-app']],
decls: 1,
vars: 1,
/** <manual-comp [name]="name"></manual-comp> */
template:
(rf: RenderFlags, ctx: ManualApp) => {
if (rf & RenderFlags.Create) {
ɵɵelement(0, 'manual-comp');
}
if (rf & RenderFlags.Update) {
ɵɵproperty('name', ctx.name);
}
},
dependencies: () => [ManualComponent]
});
}


it('should not check OnPush components in update mode when component events occur, unless marked dirty',
() => {
const myApp = renderComponent(ManualApp);
expect(comp.doCheckCount).toEqual(1);
expect(getRenderedText(myApp)).toEqual('1 - Nancy');

const button = containerEl.querySelector('button')!;
button.click();
requestAnimationFrame.flush();
// No ticks should have been scheduled.
expect(comp.doCheckCount).toEqual(1);
expect(getRenderedText(myApp)).toEqual('1 - Nancy');

tick(myApp);
// The comp should still be clean. So doCheck will run, but the view should display 1.
expect(comp.doCheckCount).toEqual(2);
expect(getRenderedText(myApp)).toEqual('1 - Nancy');

markDirty(comp);
requestAnimationFrame.flush();
// Now that markDirty has been manually called, the view should be dirty and a tick
// should be scheduled to check the view.
expect(comp.doCheckCount).toEqual(3);
expect(getRenderedText(myApp)).toEqual('3 - Nancy');
});

it('should not check parent OnPush components in update mode when child events occur, unless marked dirty',
() => {
let parent: ButtonParent;

class ButtonParent implements DoCheck {
doCheckCount = 0;
ngDoCheck(): void {
this.doCheckCount++;
}

static ɵfac = () => parent = new ButtonParent();
static ɵcmp = ɵɵdefineComponent({
type: ButtonParent,
selectors: [['button-parent']],
decls: 2,
vars: 1,
/** {{ doCheckCount }} - <manual-comp></manual-comp> */
template:
(rf: RenderFlags, ctx: ButtonParent) => {
if (rf & RenderFlags.Create) {
ɵɵtext(0);
ɵɵelement(1, 'manual-comp');
}
if (rf & RenderFlags.Update) {
ɵɵtextInterpolate1('', ctx.doCheckCount, ' - ');
}
},
dependencies: () => [ManualComponent],
changeDetection: ChangeDetectionStrategy.OnPush
});
}

const MyButtonApp = createComponent('my-button-app', function(rf: RenderFlags) {
if (rf & RenderFlags.Create) {
ɵɵelement(0, 'button-parent');
}
}, 1, 0, [ButtonParent]);

const myButtonApp = renderComponent(MyButtonApp);
expect(parent!.doCheckCount).toEqual(1);
expect(comp!.doCheckCount).toEqual(1);
expect(getRenderedText(myButtonApp)).toEqual('1 - 1 - Nancy');

tick(myButtonApp);
expect(parent!.doCheckCount).toEqual(2);
// parent isn't checked, so child doCheck won't run
expect(comp!.doCheckCount).toEqual(1);
expect(getRenderedText(myButtonApp)).toEqual('1 - 1 - Nancy');

const button = containerEl.querySelector('button');
button!.click();
requestAnimationFrame.flush();
// No ticks should have been scheduled.
expect(parent!.doCheckCount).toEqual(2);
expect(comp!.doCheckCount).toEqual(1);

tick(myButtonApp);
expect(parent!.doCheckCount).toEqual(3);
// parent isn't checked, so child doCheck won't run
expect(comp!.doCheckCount).toEqual(1);
expect(getRenderedText(myButtonApp)).toEqual('1 - 1 - Nancy');

markDirty(comp);
requestAnimationFrame.flush();
// Now that markDirty has been manually called, both views should be dirty and a tick
// should be scheduled to check the view.
expect(parent!.doCheckCount).toEqual(4);
expect(comp!.doCheckCount).toEqual(2);
expect(getRenderedText(myButtonApp)).toEqual('4 - 2 - Nancy');
});
});
});

it('should call begin and end when the renderer factory implements them', () => {
const log: string[] = [];

Expand Down

0 comments on commit 74321a4

Please sign in to comment.