Skip to content
Permalink
Browse files

fix(ivy): improve ExpressionChangedAfterChecked error (#34381)

Prior to this change, the ExpressionChangedAfterChecked error thrown in Ivy was missing useful information that was available in View Engine, specifically: missing property name for proprty bindings and also the content of the entire property interpolation (only a changed value was displayed) if one of expressions was changed unexpectedly. This commit improves the error message by including the mentioned information into the error text.

PR Close #34381
  • Loading branch information
AndrewKushnir authored and kara committed Dec 11, 2019
1 parent 82dce68 commit 7ea39849ff3d2528f8aec8c1152ed153b3a4d9f5
@@ -8,7 +8,8 @@

import {devModeEqual} from '../change_detection/change_detection_util';
import {assertDataInRange, assertLessThan, assertNotSame} from '../util/assert';
import {throwErrorIfNoChangesMode} from './errors';

import {getExpressionChangedErrorDetails, throwErrorIfNoChangesMode} from './errors';
import {LView} from './interfaces/view';
import {getCheckNoChangesMode} from './state';
import {NO_CHANGE} from './tokens';
@@ -44,7 +45,10 @@ export function bindingUpdated(lView: LView, bindingIndex: number, value: any):
// (before the change detection was run).
const oldValueToCompare = oldValue !== NO_CHANGE ? oldValue : undefined;
if (!devModeEqual(oldValueToCompare, value)) {
throwErrorIfNoChangesMode(oldValue === NO_CHANGE, oldValueToCompare, value);
const details =
getExpressionChangedErrorDetails(lView, bindingIndex, oldValueToCompare, value);
throwErrorIfNoChangesMode(
oldValue === NO_CHANGE, details.oldValue, details.newValue, details.propName);
}
}
lView[bindingIndex] = value;
@@ -10,6 +10,9 @@ import {InjectorType} from '../di/interface/defs';
import {stringify} from '../util/stringify';

import {TNode} from './interfaces/node';
import {LView, TVIEW} from './interfaces/view';
import {INTERPOLATION_DELIMITER} from './util/misc_utils';



/** Called when directives inject each other (creating a circular dependency) */
@@ -22,20 +25,6 @@ export function throwMultipleComponentError(tNode: TNode): never {
throw new Error(`Multiple components match node with tagname ${tNode.tagName}`);
}

/** Throws an ExpressionChangedAfterChecked error if checkNoChanges mode is on. */
export function throwErrorIfNoChangesMode(
creationMode: boolean, oldValue: any, currValue: any): never|void {
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);
}

export function throwMixedMultiProviderError() {
throw new Error(`Cannot mix multi providers and regular providers`);
}
@@ -52,3 +41,77 @@ export function throwInvalidProviderError(
throw new Error(
`Invalid provider for the NgModule '${stringify(ngModuleType)}'` + ngModuleDetail);
}

/** Throws an ExpressionChangedAfterChecked error if checkNoChanges mode is on. */
export function throwErrorIfNoChangesMode(
creationMode: boolean, oldValue: any, currValue: any, propName?: string): never|void {
const field = propName ? ` for '${propName}'` : '';
let msg =
`ExpressionChangedAfterItHasBeenCheckedError: Expression has changed after it was checked. Previous value${field}: '${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, see `viewDebugError` function in
// `packages/core/src/view/errors.ts` for reference.
throw new Error(msg);
}

function constructDetailsForInterpolation(
lView: LView, rootIndex: number, expressionIndex: number, meta: string, changedValue: any) {
const [propName, prefix, ...chunks] = meta.split(INTERPOLATION_DELIMITER);
let oldValue = prefix, newValue = prefix;
for (let i = 0; i < chunks.length; i++) {
const slotIdx = rootIndex + i;
oldValue += `${lView[slotIdx]}${chunks[i]}`;
newValue += `${slotIdx === expressionIndex ? changedValue : lView[slotIdx]}${chunks[i]}`;
}
return {propName, oldValue, newValue};
}

/**
* Constructs an object that contains details for the ExpressionChangedAfterItHasBeenCheckedError:
* - property name (for property bindings or interpolations)
* - old and new values, enriched using information from metadata
*
* More information on the metadata storage format can be found in `storePropertyBindingMetadata`
* function description.
*/
export function getExpressionChangedErrorDetails(
lView: LView, bindingIndex: number, oldValue: any,
newValue: any): {propName?: string, oldValue: any, newValue: any} {
const tData = lView[TVIEW].data;
const metadata = tData[bindingIndex];

if (typeof metadata === 'string') {
// metadata for property interpolation
if (metadata.indexOf(INTERPOLATION_DELIMITER) > -1) {
return constructDetailsForInterpolation(
lView, bindingIndex, bindingIndex, metadata, newValue);
}
// metadata for property binding
return {propName: metadata, oldValue, newValue};
}

// metadata is not available for this expression, check if this expression is a part of the
// property interpolation by going from the current binding index left and look for a string that
// contains INTERPOLATION_DELIMITER, the layout in tView.data for this case will look like this:
// [..., 'id�Prefix � and � suffix', null, null, null, ...]
if (metadata === null) {
let idx = bindingIndex - 1;
while (typeof tData[idx] !== 'string' && tData[idx + 1] === null) {
idx--;
}
const meta = tData[idx];
if (typeof meta === 'string') {
const matches = meta.match(new RegExp(INTERPOLATION_DELIMITER, 'g'));
// first interpolation delimiter separates property name from interpolation parts (in case of
// property interpolations), so we subtract one from total number of found delimiters
if (matches && (matches.length - 1) > bindingIndex - idx) {
return constructDetailsForInterpolation(lView, idx, bindingIndex, meta, newValue);
}
}
}
return {propName: undefined, oldValue, newValue};
}
@@ -20,7 +20,7 @@ import {activateStylingMapFeature} from '../styling/map_based_bindings';
import {attachStylingDebugObject} from '../styling/styling_debug';
import {NO_CHANGE} from '../tokens';
import {renderStringify} from '../util/misc_utils';
import {addItemToStylingMap, allocStylingMapArray, allocTStylingContext, allowDirectStyling, concatString, forceClassesAsString, forceStylesAsString, getInitialStylingValue, getStylingMapArray, getValue, hasClassInput, hasStyleInput, hasValueChanged, hasValueChangedUnwrapSafeValue, isHostStylingActive, isStylingContext, isStylingValueDefined, normalizeIntoStylingMap, patchConfig, selectClassBasedInputName, setValue, stylingMapToString} from '../util/styling_utils';
import {addItemToStylingMap, allocStylingMapArray, allocTStylingContext, allowDirectStyling, concatString, forceClassesAsString, forceStylesAsString, getInitialStylingValue, getStylingMapArray, getValue, hasClassInput, hasStyleInput, hasValueChanged, hasValueChangedUnwrapSafeValue, isHostStylingActive, isStylingContext, isStylingMapArray, isStylingValueDefined, normalizeIntoStylingMap, patchConfig, selectClassBasedInputName, setValue, stylingMapToString} from '../util/styling_utils';
import {getNativeByTNode, getTNode} from '../util/view_utils';


@@ -191,7 +191,8 @@ function stylingProp(
if (ngDevMode && getCheckNoChangesMode()) {
const oldValue = getValue(lView, bindingIndex);
if (hasValueChangedUnwrapSafeValue(oldValue, value)) {
throwErrorIfNoChangesMode(false, oldValue, value);
const field = isClassBased ? `class.${prop}` : `style.${prop}`;
throwErrorIfNoChangesMode(false, oldValue, value, field);
}
}

@@ -369,7 +370,11 @@ function stylingMap(
// For this reason, the checkNoChanges situation must also be handled here
// as well.
if (ngDevMode && valueHasChanged && getCheckNoChangesMode()) {
throwErrorIfNoChangesMode(false, oldValue, value);
// check if the value is a StylingMapArray, in which case take the first value (which stores raw
// value) from the array
const previousValue =
isStylingMapArray(oldValue) ? oldValue[StylingMapArrayIndex.RawValuePosition] : oldValue;
throwErrorIfNoChangesMode(false, previousValue, value);
}

// Direct Apply Case: bypass context resolution and apply the
@@ -12,7 +12,7 @@ import {ApplicationRef, ChangeDetectionStrategy, ChangeDetectorRef, Component, C
import {AfterContentChecked, AfterViewChecked} from '@angular/core/src/core';
import {ComponentFixture, TestBed} from '@angular/core/testing';
import {expect} from '@angular/platform-browser/testing/src/matchers';
import {ivyEnabled} from '@angular/private/testing';
import {ivyEnabled, onlyInIvy} from '@angular/private/testing';
import {BehaviorSubject} from 'rxjs';

describe('change detection', () => {
@@ -1248,6 +1248,171 @@ describe('change detection', () => {
expect(trim(fixture.nativeElement.textContent)).toEqual('InsertComp(Hello) Hello Angular!');
});
});

describe('ExpressionChangedAfterItHasBeenCheckedError', () => {
@Component({template: '...'})
class MyApp {
a: string = 'a';
b: string = 'b';
c: string = 'c';
unstableBooleanExpression: boolean = true;
unstableStringExpression: string = 'initial';
unstableColorExpression: string = 'red';
unstableStyleMapExpression: {[key: string]: string;} = {'color': 'red', 'margin': '10px'};
unstableClassMapExpression: {[key: string]: boolean;} = {'classA': true, 'classB': false};

ngAfterViewChecked() {
this.unstableBooleanExpression = false;
this.unstableStringExpression = 'changed';
this.unstableColorExpression = 'green';
this.unstableStyleMapExpression = {'color': 'green', 'margin': '20px'};
this.unstableClassMapExpression = {'classA': false, 'classB': true};
}
}

function initComponent(overrides: {[key: string]: any}): ComponentFixture<MyApp> {
TestBed.configureTestingModule({declarations: [MyApp]});
TestBed.overrideComponent(MyApp, {set: overrides});
const fixture = TestBed.createComponent(MyApp);
fixture.detectChanges();
return fixture;
}

function initWithTemplate(template: string) { return initComponent({template}); }
function initWithHostBindings(bindings: {[key: string]: string}) {
return initComponent({host: bindings});
}

it('should include field name in case of property binding', () => {
const message = ivyEnabled ? `Previous value for 'id': 'initial'. Current value: 'changed'` :
`Previous value: 'id: initial'. Current value: 'id: changed'`;
expect(() => initWithTemplate('<div [id]="unstableStringExpression"></div>'))
.toThrowError(new RegExp(message));
});

it('should include field name in case of property interpolation', () => {
const message = ivyEnabled ?
`Previous value for 'id': 'Expressions: a and initial!'. Current value: 'Expressions: a and changed!'` :
`Previous value: 'id: Expressions: a and initial!'. Current value: 'id: Expressions: a and changed!'`;
expect(
() => initWithTemplate(
'<div id="Expressions: {{ a }} and {{ unstableStringExpression }}!"></div>'))
.toThrowError(new RegExp(message));
});

it('should include field name in case of attribute binding', () => {
// TODO(akushnir): improve error message and include attr name in Ivy
const message = ivyEnabled ? `Previous value: 'initial'. Current value: 'changed'` :
`Previous value: 'id: initial'. Current value: 'id: changed'`;
expect(() => initWithTemplate('<div [attr.id]="unstableStringExpression"></div>'))
.toThrowError(new RegExp(message));
});

it('should include field name in case of attribute interpolation', () => {
// TODO(akushnir): improve error message and include attr name and entire expression in Ivy
const message = ivyEnabled ?
`Previous value: 'initial'. Current value: 'changed'` :
`Previous value: 'id: Expressions: a and initial!'. Current value: 'id: Expressions: a and changed!'`;
expect(
() => initWithTemplate(
'<div attr.id="Expressions: {{ a }} and {{ unstableStringExpression }}!"></div>'))
.toThrowError(new RegExp(message));
});

it('should only display a value of an expression that was changed in text interpolation',
() => {
expect(() => initWithTemplate('Expressions: {{ a }} and {{ unstableStringExpression }}!'))
.toThrowError(/Previous value: '.*?initial'. Current value: '.*?changed'/);
});

it('should only display a value of an expression that was changed in text interpolation ' +
'that follows an element with property interpolation',
() => {
expect(() => {
initWithTemplate(`
<div id="Prop interpolation: {{ aVal }}"></div>
Text interpolation: {{ unstableStringExpression }}.
`);
}).toThrowError(/Previous value: '.*?initial'. Current value: '.*?changed'/);
});

it('should include style prop name in case of style binding', () => {
const message = ivyEnabled ?
`Previous value for 'style.color': 'red'. Current value: 'green'` :
`Previous value: 'color: red'. Current value: 'color: green'`;
expect(() => initWithTemplate('<div [style.color]="unstableColorExpression"></div>'))
.toThrowError(new RegExp(message));
});

it('should include class name in case of class binding', () => {
const message = ivyEnabled ?
`Previous value for 'class.someClass': 'true'. Current value: 'false'` :
`Previous value: 'someClass: true'. Current value: 'someClass: false'`;
expect(() => initWithTemplate('<div [class.someClass]="unstableBooleanExpression"></div>'))
.toThrowError(new RegExp(message));
});

it('should only display a value of an expression that was changed in text interpolation inside i18n block',
() => {
expect(
() => initWithTemplate('<div i18n>Expression: {{ unstableStringExpression }}</div>'))
.toThrowError(/Previous value: '.*?initial'. Current value: '.*?changed'/);
});

it('should only display a value of an expression for interpolation inside an i18n property',
() => {
expect(
() => initWithTemplate(
'<div i18n-title title="Expression: {{ unstableStringExpression }}"></div>'))
.toThrowError(/Previous value: '.*?initial'. Current value: '.*?changed'/);
});

it('should include field name in case of host property binding', () => {
const message = ivyEnabled ? `Previous value for 'id': 'initial'. Current value: 'changed'` :
`Previous value: 'id: initial'. Current value: 'id: changed'`;
expect(() => initWithHostBindings({'[id]': 'unstableStringExpression'}))
.toThrowError(new RegExp(message));
});

it('should include style prop name in case of host style bindings', () => {
const message = ivyEnabled ?
`Previous value for 'style.color': 'red'. Current value: 'green'` :
`Previous value: 'color: red'. Current value: 'color: green'`;
expect(() => initWithHostBindings({'[style.color]': 'unstableColorExpression'}))
.toThrowError(new RegExp(message));
});

it('should include class name in case of host class bindings', () => {
const message = ivyEnabled ?
`Previous value for 'class.someClass': 'true'. Current value: 'false'` :
`Previous value: 'someClass: true'. Current value: 'someClass: false'`;
expect(() => initWithHostBindings({'[class.someClass]': 'unstableBooleanExpression'}))
.toThrowError(new RegExp(message));
});

// Note: the tests below currently fail in Ivy, but not in VE. VE behavior is correct and Ivy's
// logic should be fixed by the upcoming styling refactor, we keep these tests to verify that.
//
// it('should not throw for style maps', () => {
// expect(() => initWithTemplate('<div [style]="unstableStyleMapExpression"></div>'))
// .not.toThrowError();
// });
//
// it('should not throw for class maps', () => {
// expect(() => initWithTemplate('<div [class]="unstableClassMapExpression"></div>'))
// .not.toThrowError();
// });
//
// it('should not throw for style maps as host bindings', () => {
// expect(() => initWithHostBindings({'[style]': 'unstableStyleMapExpression'}))
// .not.toThrowError();
// });
//
// it('should not throw for class maps as host binding', () => {
// expect(() => initWithHostBindings({'[class]': 'unstableClassMapExpression'}))
// .not.toThrowError();
// });
});
});

function trim(text: string | null): string {

0 comments on commit 7ea3984

Please sign in to comment.
You can’t perform that action at this time.