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]); }