From f45c43188fa48d3655fb95405286b114e426b51d Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Matias=20Niemel=C3=A4?= Date: Fri, 11 Oct 2019 17:31:26 +0200 Subject: [PATCH] fix(ivy): ensure errors are thrown during checkNoChanges for style/class bindings (#33103) Prior to this fix, all style/class bindings (e.g. `[style]` and `[class.foo]`) would quietly update a binding value if and when the current binding value changes during checkNoChanges. With this patch, all styling instructions will properly check to see if the value has changed during the second pass of detectChanges() if checkNoChanges is active. PR Close #33103 --- aio/src/app/app.component.spec.ts | 5 +- .../doc-viewer/doc-viewer.component.spec.ts | 30 ++++++----- .../layout/doc-viewer/doc-viewer.component.ts | 5 +- .../core/src/render3/instructions/styling.ts | 38 ++++++++++---- .../core/src/render3/interfaces/styling.ts | 7 ++- .../core/src/render3/util/styling_utils.ts | 15 +++--- packages/core/test/acceptance/styling_spec.ts | 51 +++++++++++++++++++ .../styling_next/map_based_bindings_spec.ts | 2 +- 8 files changed, 116 insertions(+), 37 deletions(-) diff --git a/aio/src/app/app.component.spec.ts b/aio/src/app/app.component.spec.ts index 2c15a491c4..1c630d6f01 100644 --- a/aio/src/app/app.component.spec.ts +++ b/aio/src/app/app.component.spec.ts @@ -1,5 +1,5 @@ import { NO_ERRORS_SCHEMA, DebugElement } from '@angular/core'; -import { inject, ComponentFixture, TestBed, fakeAsync, tick } from '@angular/core/testing'; +import { inject, ComponentFixture, TestBed, fakeAsync, flushMicrotasks, tick } from '@angular/core/testing'; import { Title } from '@angular/platform-browser'; import { APP_BASE_HREF } from '@angular/common'; import { HttpClient } from '@angular/common/http'; @@ -529,7 +529,8 @@ describe('AppComponent', () => { it('should call `scrollAfterRender` (via `onDocInserted`) when navigate to a new Doc', fakeAsync(() => { locationService.go('guide/pipes'); tick(1); // triggers the HTTP response for the document - fixture.detectChanges(); // triggers the event that calls `onDocInserted` + fixture.detectChanges(); // passes the new doc to the `DocViewer` + flushMicrotasks(); // triggers the `DocViewer` event that calls `onDocInserted` expect(scrollAfterRenderSpy).toHaveBeenCalledWith(scrollDelay); diff --git a/aio/src/app/layout/doc-viewer/doc-viewer.component.spec.ts b/aio/src/app/layout/doc-viewer/doc-viewer.component.spec.ts index 905129b43e..e3edc9ea33 100644 --- a/aio/src/app/layout/doc-viewer/doc-viewer.component.spec.ts +++ b/aio/src/app/layout/doc-viewer/doc-viewer.component.spec.ts @@ -1,7 +1,7 @@ import { ComponentFixture, TestBed } from '@angular/core/testing'; import { Meta, Title } from '@angular/platform-browser'; -import { Observable, of } from 'rxjs'; +import { Observable, asapScheduler, of } from 'rxjs'; import { FILE_NOT_FOUND_ID, FETCHING_ERROR_ID } from 'app/documents/document.service'; import { Logger } from 'app/shared/logger.service'; @@ -21,6 +21,8 @@ describe('DocViewerComponent', () => { let docViewerEl: HTMLElement; let docViewer: TestDocViewerComponent; + const safeFlushAsapScheduler = () => asapScheduler.actions.length && asapScheduler.flush(); + beforeEach(() => { TestBed.configureTestingModule({ imports: [CustomElementsModule, TestModule], @@ -42,19 +44,20 @@ describe('DocViewerComponent', () => { describe('#doc', () => { let renderSpy: jasmine.Spy; - const setCurrentDoc = (contents: string|null, id = 'fizz/buzz') => { - parentComponent.currentDoc = {contents, id}; - parentFixture.detectChanges(); + const setCurrentDoc = (newDoc: TestParentComponent['currentDoc']) => { + parentComponent.currentDoc = newDoc && {id: 'fizz/buzz', ...newDoc}; + parentFixture.detectChanges(); // Run change detection to propagate the new doc to `DocViewer`. + safeFlushAsapScheduler(); // Flush `asapScheduler` to trigger `DocViewer#render()`. }; beforeEach(() => renderSpy = spyOn(docViewer, 'render').and.callFake(() => of(undefined))); it('should render the new document', () => { - setCurrentDoc('foo', 'bar'); + setCurrentDoc({contents: 'foo', id: 'bar'}); expect(renderSpy).toHaveBeenCalledTimes(1); expect(renderSpy.calls.mostRecent().args).toEqual([{id: 'bar', contents: 'foo'}]); - setCurrentDoc(null, 'baz'); + setCurrentDoc({contents: null, id: 'baz'}); expect(renderSpy).toHaveBeenCalledTimes(2); expect(renderSpy.calls.mostRecent().args).toEqual([{id: 'baz', contents: null}]); }); @@ -63,24 +66,20 @@ describe('DocViewerComponent', () => { const obs = new ObservableWithSubscriptionSpies(); renderSpy.and.returnValue(obs); - setCurrentDoc('foo', 'bar'); + setCurrentDoc({contents: 'foo', id: 'bar'}); expect(obs.subscribeSpy).toHaveBeenCalledTimes(1); expect(obs.unsubscribeSpies[0]).not.toHaveBeenCalled(); - setCurrentDoc('baz', 'qux'); + setCurrentDoc({contents: 'baz', id: 'qux'}); expect(obs.subscribeSpy).toHaveBeenCalledTimes(2); expect(obs.unsubscribeSpies[0]).toHaveBeenCalledTimes(1); }); it('should ignore falsy document values', () => { - parentComponent.currentDoc = null; - parentFixture.detectChanges(); - + setCurrentDoc(null); expect(renderSpy).not.toHaveBeenCalled(); - parentComponent.currentDoc = undefined; - parentFixture.detectChanges(); - + setCurrentDoc(undefined); expect(renderSpy).not.toHaveBeenCalled(); }); }); @@ -92,14 +91,17 @@ describe('DocViewerComponent', () => { expect(renderSpy).not.toHaveBeenCalled(); docViewer.doc = {contents: 'Some content', id: 'some-id'}; + safeFlushAsapScheduler(); expect(renderSpy).toHaveBeenCalledTimes(1); docViewer.ngOnDestroy(); docViewer.doc = {contents: 'Other content', id: 'other-id'}; + safeFlushAsapScheduler(); expect(renderSpy).toHaveBeenCalledTimes(1); docViewer.doc = {contents: 'More content', id: 'more-id'}; + safeFlushAsapScheduler(); expect(renderSpy).toHaveBeenCalledTimes(1); }); }); diff --git a/aio/src/app/layout/doc-viewer/doc-viewer.component.ts b/aio/src/app/layout/doc-viewer/doc-viewer.component.ts index e13cefffd6..1f7395b110 100644 --- a/aio/src/app/layout/doc-viewer/doc-viewer.component.ts +++ b/aio/src/app/layout/doc-viewer/doc-viewer.component.ts @@ -1,8 +1,8 @@ import { Component, ElementRef, EventEmitter, Input, OnDestroy, Output } from '@angular/core'; import { Title, Meta } from '@angular/platform-browser'; -import { Observable, of, timer } from 'rxjs'; -import { catchError, switchMap, takeUntil, tap } from 'rxjs/operators'; +import { asapScheduler, Observable, of, timer } from 'rxjs'; +import { catchError, observeOn, switchMap, takeUntil, tap } from 'rxjs/operators'; import { DocumentContents, FILE_NOT_FOUND_ID, FETCHING_ERROR_ID } from 'app/documents/document.service'; import { Logger } from 'app/shared/logger.service'; @@ -78,6 +78,7 @@ export class DocViewerComponent implements OnDestroy { this.docContents$ .pipe( + observeOn(asapScheduler), switchMap(newDoc => this.render(newDoc)), takeUntil(this.onDestroy$), ) diff --git a/packages/core/src/render3/instructions/styling.ts b/packages/core/src/render3/instructions/styling.ts index 5906f1597f..168de69f36 100644 --- a/packages/core/src/render3/instructions/styling.ts +++ b/packages/core/src/render3/instructions/styling.ts @@ -7,19 +7,20 @@ */ import {SafeValue} from '../../sanitization/bypass'; import {StyleSanitizeFn} from '../../sanitization/style_sanitizer'; +import {throwErrorIfNoChangesMode} from '../errors'; import {setInputsForProperty} from '../instructions/shared'; import {AttributeMarker, TAttributes, TNode, TNodeFlags, TNodeType} from '../interfaces/node'; import {RElement} from '../interfaces/renderer'; import {StylingMapArray, StylingMapArrayIndex, TStylingConfig, TStylingContext} from '../interfaces/styling'; import {isDirectiveHost} from '../interfaces/type_checks'; import {BINDING_INDEX, LView, RENDERER} from '../interfaces/view'; -import {getActiveDirectiveId, getCurrentStyleSanitizer, getLView, getSelectedIndex, resetCurrentStyleSanitizer, setCurrentStyleSanitizer, setElementExitFn} from '../state'; +import {getActiveDirectiveId, getCheckNoChangesMode, getCurrentStyleSanitizer, getLView, getSelectedIndex, resetCurrentStyleSanitizer, setCurrentStyleSanitizer, setElementExitFn} from '../state'; import {applyStylingMapDirectly, applyStylingValueDirectly, flushStyling, setClass, setStyle, updateClassViaContext, updateStyleViaContext} from '../styling/bindings'; 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, hasClassInput, hasStyleInput, hasValueChanged, isContextLocked, isHostStylingActive, isStylingContext, normalizeIntoStylingMap, patchConfig, selectClassBasedInputName, setValue, stylingMapToString} from '../util/styling_utils'; +import {addItemToStylingMap, allocStylingMapArray, allocTStylingContext, allowDirectStyling, concatString, forceClassesAsString, forceStylesAsString, getInitialStylingValue, getStylingMapArray, getValue, hasClassInput, hasStyleInput, hasValueChanged, isContextLocked, isHostStylingActive, isStylingContext, normalizeIntoStylingMap, patchConfig, selectClassBasedInputName, setValue, stylingMapToString} from '../util/styling_utils'; import {getNativeByTNode, getTNode} from '../util/view_utils'; @@ -171,6 +172,17 @@ function stylingProp( patchConfig(context, TStylingConfig.HasPropBindings); } + // [style.prop] and [class.name] bindings do not use `bind()` and will + // therefore manage accessing and updating the new value in the lView directly. + // For this reason, the checkNoChanges situation must also be handled here + // as well. + if (ngDevMode && getCheckNoChangesMode()) { + const oldValue = getValue(lView, bindingIndex); + if (hasValueChanged(oldValue, value)) { + throwErrorIfNoChangesMode(false, oldValue, value); + } + } + // Direct Apply Case: bypass context resolution and apply the // style/class value directly to the element if (allowDirectStyling(context, hostBindingsMode)) { @@ -246,7 +258,7 @@ export function ɵɵstyleMap(styles: {[styleName: string]: any} | NO_CHANGE | nu styles = NO_CHANGE; } - const updated = _stylingMap(index, context, bindingIndex, styles, false); + const updated = stylingMap(index, context, bindingIndex, styles, false); if (ngDevMode) { ngDevMode.styleMap++; if (updated) { @@ -303,7 +315,7 @@ export function classMapInternal( classes = NO_CHANGE; } - const updated = _stylingMap(elementIndex, context, bindingIndex, classes, true); + const updated = stylingMap(elementIndex, context, bindingIndex, classes, true); if (ngDevMode) { ngDevMode.classMap++; if (updated) { @@ -318,7 +330,7 @@ export function classMapInternal( * When this function is called it will activate support for `[style]` and * `[class]` bindings in Angular. */ -function _stylingMap( +function stylingMap( elementIndex: number, context: TStylingContext, bindingIndex: number, value: {[key: string]: any} | string | null, isClassBased: boolean): boolean { let updated = false; @@ -327,9 +339,18 @@ function _stylingMap( const directiveIndex = getActiveDirectiveId(); const tNode = getTNode(elementIndex, lView); const native = getNativeByTNode(tNode, lView) as RElement; - const oldValue = lView[bindingIndex] as StylingMapArray | null; + const oldValue = getValue(lView, bindingIndex); const hostBindingsMode = isHostStyling(); const sanitizer = getCurrentStyleSanitizer(); + const valueHasChanged = hasValueChanged(oldValue, value); + + // [style] and [class] bindings do not use `bind()` and will therefore + // manage accessing and updating the new value in the lView directly. + // For this reason, the checkNoChanges situation must also be handled here + // as well. + if (ngDevMode && valueHasChanged && getCheckNoChangesMode()) { + throwErrorIfNoChangesMode(false, oldValue, value); + } // we check for this in the instruction code so that the context can be notified // about prop or map bindings so that the direct apply check can decide earlier @@ -338,7 +359,6 @@ function _stylingMap( patchConfig(context, TStylingConfig.HasMapBindings); } - const valueHasChanged = hasValueChanged(oldValue, value); const stylingMapArr = value === NO_CHANGE ? NO_CHANGE : normalizeIntoStylingMap(oldValue, value, !isClassBased); @@ -479,12 +499,12 @@ export function registerInitialStylingOnTNode( if (typeof attr == 'number') { mode = attr; } else if (mode == AttributeMarker.Classes) { - classes = classes || allocStylingMapArray(); + classes = classes || allocStylingMapArray(null); addItemToStylingMap(classes, attr, true); hasAdditionalInitialStyling = true; } else if (mode == AttributeMarker.Styles) { const value = attrs[++i] as string | null; - styles = styles || allocStylingMapArray(); + styles = styles || allocStylingMapArray(null); addItemToStylingMap(styles, attr, value); hasAdditionalInitialStyling = true; } diff --git a/packages/core/src/render3/interfaces/styling.ts b/packages/core/src/render3/interfaces/styling.ts index 357195eb3c..53762fff9b 100644 --- a/packages/core/src/render3/interfaces/styling.ts +++ b/packages/core/src/render3/interfaces/styling.ts @@ -522,8 +522,11 @@ export type LStylingData = LView | (string | number | boolean | null)[]; * of the key/value array that was used to populate the property/ * value entries that take place in the remainder of the array. */ -export interface StylingMapArray extends Array<{}|string|number|null> { - [StylingMapArrayIndex.RawValuePosition]: {}|string|null; +export interface StylingMapArray extends Array<{}|string|number|null|undefined> { + /** + * The last raw value used to generate the entries in the map. + */ + [StylingMapArrayIndex.RawValuePosition]: {}|string|number|null|undefined; } /** diff --git a/packages/core/src/render3/util/styling_utils.ts b/packages/core/src/render3/util/styling_utils.ts index f96f53c155..7bac893b7c 100644 --- a/packages/core/src/render3/util/styling_utils.ts +++ b/packages/core/src/render3/util/styling_utils.ts @@ -43,7 +43,7 @@ export const DEFAULT_GUARD_MASK_VALUE = 0b1; */ export function allocTStylingContext( initialStyling: StylingMapArray | null, hasDirectives: boolean): TStylingContext { - initialStyling = initialStyling || allocStylingMapArray(); + initialStyling = initialStyling || allocStylingMapArray(null); let config = TStylingConfig.Initial; if (hasDirectives) { config |= TStylingConfig.HasDirectives; @@ -58,8 +58,8 @@ export function allocTStylingContext( ]; } -export function allocStylingMapArray(): StylingMapArray { - return ['']; +export function allocStylingMapArray(value: {} | string | null): StylingMapArray { + return [value]; } export function getConfig(context: TStylingContext) { @@ -169,7 +169,7 @@ export function setValue(data: LStylingData, bindingIndex: number, value: any) { } export function getValue(data: LStylingData, bindingIndex: number): T|null { - return bindingIndex > 0 ? data[bindingIndex] as T : null; + return bindingIndex !== 0 ? data[bindingIndex] as T : null; } export function lockContext(context: TStylingContext, hostBindingsMode: boolean): void { @@ -207,7 +207,7 @@ export function hasValueChanged( /** * Determines whether the provided styling value is truthy or falsy. */ -export function isStylingValueDefined(value: T): +export function isStylingValueDefined(value: T): value is NonNullable { // the reason why null is compared against is because // a CSS class value that is set to `false` must be @@ -396,8 +396,9 @@ export function normalizeIntoStylingMap( bindingValue: null | StylingMapArray, newValues: {[key: string]: any} | string | null | undefined, normalizeProps?: boolean): StylingMapArray { - const stylingMapArr: StylingMapArray = Array.isArray(bindingValue) ? bindingValue : [null]; - stylingMapArr[StylingMapArrayIndex.RawValuePosition] = newValues || null; + const stylingMapArr: StylingMapArray = + Array.isArray(bindingValue) ? bindingValue : allocStylingMapArray(null); + stylingMapArr[StylingMapArrayIndex.RawValuePosition] = newValues; // because the new values may not include all the properties // that the old ones had, all values are set to `null` before diff --git a/packages/core/test/acceptance/styling_spec.ts b/packages/core/test/acceptance/styling_spec.ts index 1d8c8ffb3e..ab0bcf2d99 100644 --- a/packages/core/test/acceptance/styling_spec.ts +++ b/packages/core/test/acceptance/styling_spec.ts @@ -2241,6 +2241,57 @@ describe('styling', () => { fixture.detectChanges(); expect(div.classList.contains('disabled')).toBe(false); }); + + it('should throw an error if a prop-based style/class binding value is changed during checkNoChanges', + () => { + @Component({ + template: ` +
+ ` + }) + class Cmp { + color = 'red'; + fooClass = true; + + ngAfterViewInit() { + this.color = 'blue'; + this.fooClass = false; + } + } + + TestBed.configureTestingModule({declarations: [Cmp]}); + const fixture = TestBed.createComponent(Cmp); + + expect(() => { + fixture.detectChanges(); + }).toThrowError(/ExpressionChangedAfterItHasBeenCheckedError/); + }); + + onlyInIvy('only ivy allows for map-based style AND class bindings') + .it('should throw an error if a map-based style/class binding value is changed during checkNoChanges', + () => { + @Component({ + template: ` +
+ ` + }) + class Cmp { + style: any = {width: '100px'}; + klass: any = {foo: true, bar: false}; + + ngAfterViewInit() { + this.style = {height: '200px'}; + this.klass = {foo: false}; + } + } + + TestBed.configureTestingModule({declarations: [Cmp]}); + const fixture = TestBed.createComponent(Cmp); + + expect(() => { + fixture.detectChanges(); + }).toThrowError(/ExpressionChangedAfterItHasBeenCheckedError/); + }); }); function assertStyleCounters(countForSet: number, countForRemove: number) { diff --git a/packages/core/test/render3/styling_next/map_based_bindings_spec.ts b/packages/core/test/render3/styling_next/map_based_bindings_spec.ts index 0845a35815..6e5a55e3a0 100644 --- a/packages/core/test/render3/styling_next/map_based_bindings_spec.ts +++ b/packages/core/test/render3/styling_next/map_based_bindings_spec.ts @@ -81,5 +81,5 @@ describe('map-based bindings', () => { function createAndAssertValues(newValue: any, entries: any[]) { const result = createMap(null, newValue); - expect(result).toEqual([newValue || null, ...entries]); + expect(result).toEqual([newValue, ...entries]); }