Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

feat(ivy): support checkNoChanges #22710

Closed
wants to merge 2 commits into from
Closed
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
72 changes: 60 additions & 12 deletions packages/core/src/render3/instructions.ts
Original file line number Diff line number Diff line change
Expand Up @@ -151,6 +151,13 @@ let bindingIndex: number;
*/
let cleanup: any[]|null;

/**
* In this mode, any changes in bindings will throw an ExpressionChangedAfterChecked error.
*
* Necessary to support ChangeDetectorRef.checkNoChanges().
*/
let checkNoChangesMode = false;

const enum BindingDirection {
Input,
Output,
Expand Down Expand Up @@ -194,9 +201,11 @@ export function enterView(newView: LView, host: LElementNode | LViewNode | null)
* the direction of traversal (up or down the view tree) a bit clearer.
*/
export function leaveView(newView: LView): void {
executeHooks(
currentView.data, currentView.tView.viewHooks, currentView.tView.viewCheckHooks,
creationMode);
if (!checkNoChangesMode) {
executeHooks(
currentView.data, currentView.tView.viewHooks, currentView.tView.viewCheckHooks,
creationMode);
}
// Views should be clean and in update mode after being checked, so these bits are cleared
currentView.flags &= ~(LViewFlags.CreationMode | LViewFlags.Dirty);
currentView.lifecycleStage = LifecycleStage.INIT;
Expand Down Expand Up @@ -1135,9 +1144,11 @@ export function containerRefreshStart(index: number): void {
(previousOrParentNode as LContainerNode).native, undefined,
`the container's native element should not have been set yet.`);

// We need to execute init hooks here so ngOnInit hooks are called in top level views
// before they are called in embedded views (for backwards compatibility).
executeInitHooks(currentView, currentView.tView, creationMode);
if (!checkNoChangesMode) {
// We need to execute init hooks here so ngOnInit hooks are called in top level views
// before they are called in embedded views (for backwards compatibility).
executeInitHooks(currentView, currentView.tView, creationMode);
}
}

/**
Expand Down Expand Up @@ -1270,8 +1281,10 @@ export function embeddedViewEnd(): void {
* @param elementIndex
*/
export function directiveRefresh<T>(directiveIndex: number, elementIndex: number): void {
executeInitHooks(currentView, currentView.tView, creationMode);
executeContentHooks(currentView, currentView.tView, creationMode);
if (!checkNoChangesMode) {
executeInitHooks(currentView, currentView.tView, creationMode);
executeContentHooks(currentView, currentView.tView, creationMode);
}
const template = (tData[directiveIndex] as ComponentDef<T>).template;
if (template != null) {
ngDevMode && assertDataInRange(elementIndex);
Expand Down Expand Up @@ -1594,6 +1607,37 @@ export function detectChanges<T>(component: T): void {
}


/**
* Checks the change detector and its children, and throws if any changes are detected.
*
* This is used in development mode to verify that running change detection doesn't
* introduce other changes.
*/
export function checkNoChanges<T>(component: T): void {
checkNoChangesMode = true;
try {
detectChanges(component);
} finally {
checkNoChangesMode = false;
}
}

/** Throws an ExpressionChangedAfterChecked error if checkNoChanges mode is on. */
function throwErrorIfNoChangesMode(oldValue: any, currValue: any): never|void {
if (checkNoChangesMode) {
let msg =
`ExpressionChangedAfterItHasBeenCheckedError: Expression has changed after it was checked. Previous value: '${oldValue}'. Current value: '${currValue}'.`;
if (creationMode) {
msg +=
` It seems like the view has been created after its parent and its children have been dirty checked.` +
` Has it been created in a change detection hook ?`;
}
// TODO: include debug context
throw new Error(msg);
}
}


/** Checks the view of the component provided. Does not gate on dirty checks or execute doCheck. */
function detectChangesInternal<T>(hostView: LView, hostNode: LElementNode, component: T) {
const componentIndex = hostNode.flags >> LNodeFlags.INDX_SHIFT;
Expand Down Expand Up @@ -1672,6 +1716,7 @@ export function bind<T>(value: T | NO_CHANGE): T|NO_CHANGE {

const changed: boolean = value !== NO_CHANGE && isDifferent(data[bindingIndex], value);
if (changed) {
throwErrorIfNoChangesMode(data[bindingIndex], value);
data[bindingIndex] = value;
}
bindingIndex++;
Expand Down Expand Up @@ -1841,14 +1886,17 @@ export function consumeBinding(): any {
export function bindingUpdated(value: any): boolean {
ngDevMode && assertNotEqual(value, NO_CHANGE, 'Incoming value should never be NO_CHANGE.');

if (creationMode || isDifferent(data[bindingIndex], value)) {
creationMode && initBindings();
data[bindingIndex++] = value;
return true;
if (creationMode) {
initBindings();
} else if (isDifferent(data[bindingIndex], value)) {
throwErrorIfNoChangesMode(data[bindingIndex], value);
} else {
bindingIndex++;
return false;
}

data[bindingIndex++] = value;
return true;
}

/** Updates binding if changed, then returns the latest value. */
Expand Down
10 changes: 8 additions & 2 deletions packages/core/src/render3/view_ref.ts
Original file line number Diff line number Diff line change
Expand Up @@ -8,7 +8,7 @@

import {EmbeddedViewRef as viewEngine_EmbeddedViewRef} from '../linker/view_ref';

import {detectChanges, markViewDirty} from './instructions';
import {checkNoChanges, detectChanges, markViewDirty} from './instructions';
import {ComponentTemplate} from './interfaces/definition';
import {LViewNode} from './interfaces/node';
import {LView, LViewFlags} from './interfaces/view';
Expand Down Expand Up @@ -195,7 +195,13 @@ export class ViewRef<T> implements viewEngine_EmbeddedViewRef<T> {
*/
detectChanges(): void { detectChanges(this.context); }

checkNoChanges(): void { notImplemented(); }
/**
* Checks the change detector and its children, and throws if any changes are detected.
*
* This is used in development mode to verify that running change detection doesn't
* introduce other changes.
*/
checkNoChanges(): void { checkNoChanges(this.context); }
}


Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -35,6 +35,9 @@
{
"name": "canInsertNativeNode"
},
{
"name": "checkNoChangesMode"
},
{
"name": "createLNode"
},
Expand Down
156 changes: 156 additions & 0 deletions packages/core/test/render3/change_detection_spec.ts
Original file line number Diff line number Diff line change
Expand Up @@ -911,6 +911,162 @@ describe('change detection', () => {
// TODO(kara): add test for dynamic views once bug fix is in
});

describe('checkNoChanges', () => {
let comp: NoChangesComp;

class NoChangesComp {
value = 1;
doCheckCount = 0;
contentCheckCount = 0;
viewCheckCount = 0;

ngDoCheck() { this.doCheckCount++; }

ngAfterContentChecked() { this.contentCheckCount++; }

ngAfterViewChecked() { this.viewCheckCount++; }

constructor(public cdr: ChangeDetectorRef) {}

static ngComponentDef = defineComponent({
type: NoChangesComp,
tag: 'no-changes-comp',
factory: () => comp = new NoChangesComp(injectChangeDetectorRef()),
template: (ctx: NoChangesComp, cm: boolean) => {
if (cm) {
text(0);
}
textBinding(0, bind(ctx.value));
}
});
}

class AppComp {
value = 1;

constructor(public cdr: ChangeDetectorRef) {}

static ngComponentDef = defineComponent({
type: AppComp,
tag: 'app-comp',
factory: () => new AppComp(injectChangeDetectorRef()),
/**
* {{ value }} -
* <no-changes-comp></no-changes-comp>
*/
template: (ctx: AppComp, cm: boolean) => {
if (cm) {
text(0);
elementStart(1, NoChangesComp);
elementEnd();
}
textBinding(0, interpolation1('', ctx.value, ' - '));
NoChangesComp.ngComponentDef.h(2, 1);
directiveRefresh(2, 1);
}
});
}

it('should throw if bindings in current view have changed', () => {
const comp = renderComponent(NoChangesComp);

expect(() => comp.cdr.checkNoChanges()).not.toThrow();

comp.value = 2;
expect(() => comp.cdr.checkNoChanges())
.toThrowError(
/ExpressionChangedAfterItHasBeenCheckedError: .+ Previous value: '1'. Current value: '2'/gi);
});

it('should throw if interpolations in current view have changed', () => {
const app = renderComponent(AppComp);

expect(() => app.cdr.checkNoChanges()).not.toThrow();

app.value = 2;
expect(() => app.cdr.checkNoChanges())
.toThrowError(
/ExpressionChangedAfterItHasBeenCheckedError: .+ Previous value: '1'. Current value: '2'/gi);
});

it('should throw if bindings in children of current view have changed', () => {
const app = renderComponent(AppComp);

expect(() => app.cdr.checkNoChanges()).not.toThrow();

comp.value = 2;
expect(() => app.cdr.checkNoChanges())
.toThrowError(
/ExpressionChangedAfterItHasBeenCheckedError: .+ Previous value: '1'. Current value: '2'/gi);
});

it('should throw if bindings in embedded view have changed', () => {
class EmbeddedViewApp {
value = 1;
showing = true;

constructor(public cdr: ChangeDetectorRef) {}

static ngComponentDef = defineComponent({
type: EmbeddedViewApp,
tag: 'embedded-view-app',
factory: () => new EmbeddedViewApp(injectChangeDetectorRef()),
/**
* % if (showing) {
* {{ value }}
* %}
*/
template: (ctx: EmbeddedViewApp, cm: boolean) => {
if (cm) {
container(0);
}
containerRefreshStart(0);
{
if (ctx.showing) {
if (embeddedViewStart(0)) {
text(0);
}
textBinding(0, bind(ctx.value));
embeddedViewEnd();
}
}
containerRefreshEnd();
}
});
}

const app = renderComponent(EmbeddedViewApp);

expect(() => app.cdr.checkNoChanges()).not.toThrow();

app.value = 2;
expect(() => app.cdr.checkNoChanges())
.toThrowError(
/ExpressionChangedAfterItHasBeenCheckedError: .+ Previous value: '1'. Current value: '2'/gi);
});

it('should NOT call lifecycle hooks', () => {
const app = renderComponent(AppComp);
expect(comp.doCheckCount).toEqual(1);
expect(comp.contentCheckCount).toEqual(1);
expect(comp.viewCheckCount).toEqual(1);

comp.value = 2;
expect(() => app.cdr.checkNoChanges()).toThrow();
expect(comp.doCheckCount).toEqual(1);
expect(comp.contentCheckCount).toEqual(1);
expect(comp.viewCheckCount).toEqual(1);
});

it('should NOT throw if bindings in ancestors of current view have changed', () => {
const app = renderComponent(AppComp);

app.value = 2;
expect(() => comp.cdr.checkNoChanges()).not.toThrow();
});

});

});

});