From 9d1175e2b2b8bc8b282b15f56daac01e0537a5bf Mon Sep 17 00:00:00 2001 From: Andrew Kushnir Date: Tue, 10 Dec 2019 16:23:56 -0800 Subject: [PATCH] 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 --- packages/core/src/render3/bindings.ts | 8 +- packages/core/src/render3/errors.ts | 91 ++++++++-- .../core/src/render3/instructions/styling.ts | 11 +- .../test/acceptance/change_detection_spec.ts | 167 +++++++++++++++++- 4 files changed, 257 insertions(+), 20 deletions(-) diff --git a/packages/core/src/render3/bindings.ts b/packages/core/src/render3/bindings.ts index 989c410ba9..bb99394e49 100644 --- a/packages/core/src/render3/bindings.ts +++ b/packages/core/src/render3/bindings.ts @@ -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; diff --git a/packages/core/src/render3/errors.ts b/packages/core/src/render3/errors.ts index f0dff261cf..d92cb4ebc2 100644 --- a/packages/core/src/render3/errors.ts +++ b/packages/core/src/render3/errors.ts @@ -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}; +} \ No newline at end of file diff --git a/packages/core/src/render3/instructions/styling.ts b/packages/core/src/render3/instructions/styling.ts index 4fb6b0bacf..68c108bdcf 100644 --- a/packages/core/src/render3/instructions/styling.ts +++ b/packages/core/src/render3/instructions/styling.ts @@ -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 diff --git a/packages/core/test/acceptance/change_detection_spec.ts b/packages/core/test/acceptance/change_detection_spec.ts index b1046c9517..bed117e249 100644 --- a/packages/core/test/acceptance/change_detection_spec.ts +++ b/packages/core/test/acceptance/change_detection_spec.ts @@ -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 { + 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('
')) + .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( + '
')) + .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('
')) + .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( + '
')) + .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(` +
+ 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('
')) + .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('
')) + .toThrowError(new RegExp(message)); + }); + + it('should only display a value of an expression that was changed in text interpolation inside i18n block', + () => { + expect( + () => initWithTemplate('
Expression: {{ unstableStringExpression }}
')) + .toThrowError(/Previous value: '.*?initial'. Current value: '.*?changed'/); + }); + + it('should only display a value of an expression for interpolation inside an i18n property', + () => { + expect( + () => initWithTemplate( + '
')) + .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('
')) + // .not.toThrowError(); + // }); + // + // it('should not throw for class maps', () => { + // expect(() => initWithTemplate('
')) + // .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 {