From 21a9a41c5bba5f95166db830366e94b72a118ee8 Mon Sep 17 00:00:00 2001
From: Igor Minar <igor@angular.io>
Date: Sat, 11 Jan 2020 11:23:53 -0800
Subject: [PATCH] fix(common): cleanup the StylingDiffer and related code
 (#34307)

Since I was learning the codebase and had a hard time understanding what was going on I've done a
bunch of changes in one commit that under normal circumstances should have been split into several
commits. Because this code is likely going to be overwritten with Misko's changes I'm not going to
spend the time with trying to split this up.

Overall I've done the following:
- I processed review feedback from #34307
- I did a bunch of renaming to make the code easier to understand
- I refactored some internal functions that were either inefficient or hard to read
- I also updated lots of type signatures to correct them and to remove many casts in the code

PR Close #34307
---
 integration/_payload-limits.json              |   2 +-
 .../common/src/directives/ng_class_impl.ts    |  40 +--
 .../common/src/directives/ng_style_impl.ts    |   6 +-
 .../common/src/directives/styling_differ.ts   | 297 ++++++++++--------
 .../common/test/directives/ng_class_spec.ts   |  18 +-
 .../common/test/directives/ng_style_spec.ts   |  25 +-
 .../test/directives/styling_differ_spec.ts    | 154 ++++-----
 .../ngcc/test/integration/ngcc_spec.ts        |   3 +-
 8 files changed, 309 insertions(+), 236 deletions(-)

diff --git a/integration/_payload-limits.json b/integration/_payload-limits.json
index f7ecc299a3..34682d492d 100644
--- a/integration/_payload-limits.json
+++ b/integration/_payload-limits.json
@@ -39,7 +39,7 @@
     "master": {
       "uncompressed": {
         "runtime-es2015": 2289,
-        "main-es2015": 266648,
+        "main-es2015": 267182,
         "polyfills-es2015": 36808,
         "5-es2015": 751
       }
diff --git a/packages/common/src/directives/ng_class_impl.ts b/packages/common/src/directives/ng_class_impl.ts
index ef48b17292..6232cfdf6f 100644
--- a/packages/common/src/directives/ng_class_impl.ts
+++ b/packages/common/src/directives/ng_class_impl.ts
@@ -26,7 +26,7 @@ export abstract class NgClassImpl {
 }
 
 @Injectable()
-export class NgClassR2Impl implements NgClassImpl {
+export class NgClassR2Impl extends NgClassImpl {
   // TODO(issue/24571): remove '!'.
   private _iterableDiffer !: IterableDiffer<string>| null;
   // TODO(issue/24571): remove '!'.
@@ -37,7 +37,9 @@ export class NgClassR2Impl implements NgClassImpl {
 
   constructor(
       private _iterableDiffers: IterableDiffers, private _keyValueDiffers: KeyValueDiffers,
-      private _ngEl: ElementRef, private _renderer: Renderer2) {}
+      private _ngEl: ElementRef, private _renderer: Renderer2) {
+    super();
+  }
 
   getValue() { return null; }
 
@@ -48,7 +50,8 @@ export class NgClassR2Impl implements NgClassImpl {
     this._applyClasses(this._rawClass);
   }
 
-  setNgClass(value: string) {
+
+  setNgClass(value: string|string[]|Set<string>|{[klass: string]: any}) {
     this._removeClasses(this._rawClass);
     this._applyClasses(this._initialClasses);
 
@@ -96,7 +99,7 @@ export class NgClassR2Impl implements NgClassImpl {
         this._toggleClass(record.item, true);
       } else {
         throw new Error(
-            `NgClass can only toggle CSS classes expressed as strings, got ${stringify(record.item)}`);
+            `NgClass can only toggle CSS classes expressed as strings, got: ${stringify(record.item)}`);
       }
     });
 
@@ -150,13 +153,13 @@ export class NgClassR2Impl implements NgClassImpl {
 }
 
 @Injectable()
-export class NgClassR3Impl implements NgClassImpl {
+export class NgClassR3Impl extends NgClassImpl {
   private _value: {[key: string]: boolean}|null = null;
-  private _ngClassDiffer = new StylingDiffer<{[key: string]: boolean}|null>(
+  private _ngClassDiffer = new StylingDiffer<{[key: string]: true}>(
       'NgClass', StylingDifferOptions.TrimProperties|
                  StylingDifferOptions.AllowSubKeys|
                  StylingDifferOptions.AllowStringValue|StylingDifferOptions.ForceAsMap);
-  private _classStringDiffer: StylingDiffer<{[key: string]: boolean}>|null = null;
+  private _classStringDiffer: StylingDiffer<{[key: string]: true}>|null = null;
 
   getValue() { return this._value; }
 
@@ -168,26 +171,23 @@ export class NgClassR3Impl implements NgClassImpl {
     this._classStringDiffer = this._classStringDiffer ||
         new StylingDiffer('class',
                           StylingDifferOptions.AllowStringValue | StylingDifferOptions.ForceAsMap);
-    this._classStringDiffer.setValue(value);
+    this._classStringDiffer.setInput(value);
   }
 
   setNgClass(value: string|string[]|Set<string>|{[klass: string]: any}) {
-    this._ngClassDiffer.setValue(value);
+    this._ngClassDiffer.setInput(value);
   }
 
   applyChanges() {
-    const classChanged =
-        this._classStringDiffer ? this._classStringDiffer.hasValueChanged() : false;
-    const ngClassChanged = this._ngClassDiffer.hasValueChanged();
+    const classChanged = this._classStringDiffer ? this._classStringDiffer.updateValue() : false;
+    const ngClassChanged = this._ngClassDiffer.updateValue();
     if (classChanged || ngClassChanged) {
-      let value = this._ngClassDiffer.value;
-      if (this._classStringDiffer) {
-        let classValue = this._classStringDiffer.value;
-        if (classValue) {
-          value = value ? {...classValue, ...value} : classValue;
-        }
-      }
-      this._value = value;
+      let ngClassValue = this._ngClassDiffer.value;
+      let classValue = this._classStringDiffer ? this._classStringDiffer.value : null;
+
+      // merge classValue and ngClassValue and set value
+      this._value = (classValue && ngClassValue) ? {...classValue, ...ngClassValue} :
+                                                   classValue || ngClassValue;
     }
   }
 }
diff --git a/packages/common/src/directives/ng_style_impl.ts b/packages/common/src/directives/ng_style_impl.ts
index 5c0e87566d..e3c720ceaa 100644
--- a/packages/common/src/directives/ng_style_impl.ts
+++ b/packages/common/src/directives/ng_style_impl.ts
@@ -83,16 +83,16 @@ export class NgStyleR2Impl implements NgStyleImpl {
 @Injectable()
 export class NgStyleR3Impl implements NgStyleImpl {
   private _differ =
-      new StylingDiffer<{[key: string]: any}|null>('NgStyle', StylingDifferOptions.AllowUnits);
+      new StylingDiffer<{[key: string]: any}>('NgStyle', StylingDifferOptions.AllowUnits);
 
   private _value: {[key: string]: any}|null = null;
 
   getValue() { return this._value; }
 
-  setNgStyle(value: {[key: string]: any}|null) { this._differ.setValue(value); }
+  setNgStyle(value: {[key: string]: any}|null) { this._differ.setInput(value); }
 
   applyChanges() {
-    if (this._differ.hasValueChanged()) {
+    if (this._differ.updateValue()) {
       this._value = this._differ.value;
     }
   }
diff --git a/packages/common/src/directives/styling_differ.ts b/packages/common/src/directives/styling_differ.ts
index 792ab1a44a..bb4f13910c 100644
--- a/packages/common/src/directives/styling_differ.ts
+++ b/packages/common/src/directives/styling_differ.ts
@@ -13,7 +13,7 @@
  * of how [style] and [class] behave in Angular.
  *
  * The differences are:
- *  - ngStyle and ngClass both **watch** their binding values for changes each time CD runs
+ *  - ngStyle and ngClass both **deep-watch** their binding values for changes each time CD runs
  *    while [style] and [class] bindings do not (they check for identity changes)
  *  - ngStyle allows for unit-based keys (e.g. `{'max-width.px':value}`) and [style] does not
  *  - ngClass supports arrays of class values and [class] only accepts map and string values
@@ -24,8 +24,9 @@
  * and unnecessary. Instead, ngClass and ngStyle should have their input values be converted
  * into something that the core-level [style] and [class] bindings understand.
  *
- * This [StylingDiffer] class handles this conversion by creating a new input value each time
- * the inner representation of the binding value have changed.
+ * This [StylingDiffer] class handles this conversion by creating a new output value each time
+ * the input value of the binding value has changed (either via identity change or deep collection
+ * content change).
  *
  * ## Why do we care about ngStyle/ngClass?
  * The styling algorithm code (documented inside of `render3/interfaces/styling.ts`) needs to
@@ -40,8 +41,8 @@
  * - If ngStyle/ngClass is used in combination with [style]/[class] bindings then the
  *   styles and classes would fall out of sync and be applied and updated at
  *   inconsistent times
- * - Both ngClass/ngStyle do not respect [class.name] and [style.prop] bindings
- *   (they will write over them given the right combination of events)
+ * - Both ngClass/ngStyle should respect [class.name] and [style.prop] bindings (and not arbitrarily
+ *   overwrite their changes)
  *
  *   ```
  *   <!-- if `w1` is updated then it will always override `w2`
@@ -57,37 +58,38 @@
  * - ngClass/ngStyle were written as a directives and made use of maps, closures and other
  *   expensive data structures which were evaluated each time CD runs
  */
-export class StylingDiffer<T> {
+export class StylingDiffer<T extends({[key: string]: string | null} | {[key: string]: true})> {
+  /**
+   * Normalized string map representing the last value set via `setValue()` or null if no value has
+   * been set or the last set value was null
+   */
   public readonly value: T|null = null;
 
   /**
    * The last set value that was applied via `setValue()`
    */
-  private _lastSetValue: {[key: string]: any}|string|string[]|undefined|null = undefined;
+  private _inputValue: T|string|string[]|Set<string>|null = null;
 
   /**
    * The type of value that the `_lastSetValue` variable is
    */
-  private _lastSetValueType: StylingDifferValueTypes = StylingDifferValueTypes.Null;
+  private _inputValueType: StylingDifferValueTypes = StylingDifferValueTypes.Null;
 
   /**
    * Whether or not the last value change occurred because the variable itself changed reference
    * (identity)
    */
-  private _lastSetValueIdentityChange = false;
+  private _inputValueIdentityChangeSinceLastCheck = false;
 
   constructor(private _name: string, private _options: StylingDifferOptions) {}
 
   /**
-   * Sets (updates) the styling value within the differ.
+   * Sets the input value for the differ and updates the output value if necessary.
    *
-   * Only when `hasValueChanged` is called then this new value will be evaluted
-   * and checked against the previous value.
-   *
-   * @param value the new styling value provided from the ngClass/ngStyle binding
+   * @param value the new styling input value provided from the ngClass/ngStyle binding
    */
-  setValue(value: {[key: string]: any}|string[]|string|null) {
-    if (value !== this._lastSetValue) {
+  setInput(value: T|string[]|string|Set<string>|null): void {
+    if (value !== this._inputValue) {
       let type: StylingDifferValueTypes;
       if (!value) {  // matches empty strings, null, false and undefined
         type = StylingDifferValueTypes.Null;
@@ -105,15 +107,15 @@ export class StylingDiffer<T> {
         type = StylingDifferValueTypes.StringMap;
       }
 
-      this._lastSetValueType = type;
-      this._lastSetValueIdentityChange = true;
-      this._lastSetValue = value;
+      this._inputValue = value;
+      this._inputValueType = type;
+      this._inputValueIdentityChangeSinceLastCheck = true;
       this._processValueChange(true);
     }
   }
 
   /**
-   * Determines whether or not the value has changed.
+   * Checks the input value for identity or deep changes and updates output value if necessary.
    *
    * This function can be called right after `setValue()` is called, but it can also be
    * called incase the existing value (if it's a collection) changes internally. If the
@@ -122,9 +124,10 @@ export class StylingDiffer<T> {
    *
    * @returns whether or not the value has changed in some way.
    */
-  hasValueChanged(): boolean {
-    let valueHasChanged = this._lastSetValueIdentityChange;
-    if (!valueHasChanged && (this._lastSetValueType & StylingDifferValueTypes.Collection)) {
+  updateValue(): boolean {
+    let valueHasChanged = this._inputValueIdentityChangeSinceLastCheck;
+    if (!this._inputValueIdentityChangeSinceLastCheck &&
+        (this._inputValueType & StylingDifferValueTypes.Collection)) {
       valueHasChanged = this._processValueChange(false);
     } else {
       // this is set to false in the event that the value is a collection.
@@ -132,86 +135,89 @@ export class StylingDiffer<T> {
       // diff the collection value to see if the contents have mutated
       // (otherwise the value change was processed during the time when
       // the variable changed).
-      this._lastSetValueIdentityChange = false;
+      this._inputValueIdentityChangeSinceLastCheck = false;
     }
     return valueHasChanged;
   }
 
   /**
-   * Examines the last set value to see if there was a change in data.
+   * Examines the last set value to see if there was a change in content.
    *
-   * @param hasIdentityChange whether or not the last set value changed in identity or not
-   *
-   * @returns true when the value has changed (either by identity or by shape if its a collection).
+   * @param inputValueIdentityChanged whether or not the last set value changed in identity or not
+   * @returns `true` when the value has changed (either by identity or by shape if its a
+   * collection)
    */
-  private _processValueChange(hasIdentityChange: boolean) {
-    let valueHasChanged = hasIdentityChange;
+  private _processValueChange(inputValueIdentityChanged: boolean): boolean {
+    // if the inputValueIdentityChanged then we know that input has changed
+    let inputChanged = inputValueIdentityChanged;
 
-    let finalValue: {[key: string]: any}|string|null = null;
+    let newOutputValue: T|string|null = null;
     const trimValues = (this._options & StylingDifferOptions.TrimProperties) ? true : false;
     const parseOutUnits = (this._options & StylingDifferOptions.AllowUnits) ? true : false;
     const allowSubKeys = (this._options & StylingDifferOptions.AllowSubKeys) ? true : false;
 
-    switch (this._lastSetValueType) {
+    switch (this._inputValueType) {
       // case 1: [input]="string"
-      case StylingDifferValueTypes.String:
-        const tokens = (this._lastSetValue as string).split(/\s+/g);
-        if (this._options & StylingDifferOptions.ForceAsMap) {
-          finalValue = {} as{[key: string]: any};
-          for (let i = 0; i < tokens.length; i++) {
-            finalValue[tokens[i]] = true;
-          }
-        } else {
-          finalValue = '';
-          for (let i = 0; i < tokens.length; i++) {
-            finalValue += (i !== 0 ? ' ' : '') + tokens[i];
+      case StylingDifferValueTypes.String: {
+        if (inputValueIdentityChanged) {
+          // process string input only if the identity has changed since the strings are immutable
+          const keys = (this._inputValue as string).split(/\s+/g);
+          if (this._options & StylingDifferOptions.ForceAsMap) {
+            newOutputValue = {} as T;
+            for (let i = 0; i < keys.length; i++) {
+              (newOutputValue as any)[keys[i]] = true;
+            }
+          } else {
+            newOutputValue = keys.join(' ');
           }
         }
         break;
-
+      }
       // case 2: [input]="{key:value}"
-      case StylingDifferValueTypes.StringMap:
-        const map: {[key: string]: any} = this._lastSetValue as{[key: string]: any};
-        const keys = Object.keys(map);
-        if (!valueHasChanged) {
-          // we know that the classExp value exists and that it is
-          // a map (otherwise an identity change would have occurred)
-          valueHasChanged =
-              this.value ? mapHasChanged(keys, this.value as{[key: string]: any}, map) : true;
+      case StylingDifferValueTypes.StringMap: {
+        const inputMap = this._inputValue as T;
+        const inputKeys = Object.keys(inputMap);
+
+        if (!inputValueIdentityChanged) {
+          // if StringMap and the identity has not changed then output value must have already been
+          // initialized to a StringMap, so we can safely compare the input and output maps
+          inputChanged = mapsAreEqual(inputKeys, inputMap, this.value as T);
         }
 
-        if (valueHasChanged) {
-          finalValue =
-              bulidMapFromValues(this._name, trimValues, parseOutUnits, allowSubKeys, map, keys);
+        if (inputChanged) {
+          newOutputValue = bulidMapFromStringMap(
+              trimValues, parseOutUnits, allowSubKeys, inputMap, inputKeys) as T;
         }
         break;
-
+      }
       // case 3a: [input]="[str1, str2, ...]"
       // case 3b: [input]="Set"
       case StylingDifferValueTypes.Array:
-      case StylingDifferValueTypes.Set:
-        const values = Array.from(this._lastSetValue as string[] | Set<string>);
-        if (!valueHasChanged) {
-          const keys = Object.keys(this.value !);
-          valueHasChanged = !arrayEqualsArray(keys, values);
+      case StylingDifferValueTypes.Set: {
+        const inputKeys = Array.from(this._inputValue as string[] | Set<string>);
+        if (!inputValueIdentityChanged) {
+          const outputKeys = Object.keys(this.value !);
+          inputChanged = !keyArraysAreEqual(outputKeys, inputKeys);
         }
-        if (valueHasChanged) {
-          finalValue =
-              bulidMapFromValues(this._name, trimValues, parseOutUnits, allowSubKeys, values);
+        if (inputChanged) {
+          newOutputValue =
+              bulidMapFromStringArray(this._name, trimValues, allowSubKeys, inputKeys) as T;
         }
         break;
-
+      }
       // case 4: [input]="null|undefined"
       default:
-        finalValue = null;
+        inputChanged = inputValueIdentityChanged;
+        newOutputValue = null;
         break;
     }
 
-    if (valueHasChanged) {
-      (this as any).value = finalValue !;
+    if (inputChanged) {
+      // update the readonly `value` property by casting it to `any` first
+      (this as any).value = newOutputValue;
     }
 
-    return valueHasChanged;
+    return inputChanged;
   }
 }
 
@@ -241,34 +247,55 @@ const enum StylingDifferValueTypes {
 
 
 /**
- * builds and returns a map based on the values input value
- *
- * If the `keys` param is provided then the `values` param is treated as a
- * string map. Otherwise `values` is treated as a string array.
+ * @param trim whether the keys should be trimmed of leading or trailing whitespace
+ * @param parseOutUnits whether units like "px" should be parsed out of the key name and appended to
+ *   the value
+ * @param allowSubKeys whether key needs to be subsplit by whitespace into multiple keys
+ * @param values values of the map
+ * @param keys keys of the map
+ * @return a normalized string map based on the input string map
  */
-function bulidMapFromValues(
-    errorPrefix: string, trim: boolean, parseOutUnits: boolean, allowSubKeys: boolean,
-    values: {[key: string]: any} | string[], keys?: string[]) {
-  const map: {[key: string]: any} = {};
-  if (keys) {
-    // case 1: map
-    for (let i = 0; i < keys.length; i++) {
-      let key = keys[i];
-      const value = (values as{[key: string]: any})[key];
+function bulidMapFromStringMap(
+    trim: boolean, parseOutUnits: boolean, allowSubKeys: boolean,
+    values: {[key: string]: string | null | true},
+    keys: string[]): {[key: string]: string | null | true} {
+  const map: {[key: string]: string | null | true} = {};
 
-      if (value !== undefined) {
-        // Map uses untrimmed keys, so don't trim until passing to `setMapValues`
-        setMapValues(map, trim ? key.trim() : key, value, parseOutUnits, allowSubKeys);
+  for (let i = 0; i < keys.length; i++) {
+    let key = keys[i];
+    let value = values[key];
+
+    if (value !== undefined) {
+      if (typeof value !== 'boolean') {
+        value = '' + value;
       }
+      // Map uses untrimmed keys, so don't trim until passing to `setMapValues`
+      setMapValues(map, trim ? key.trim() : key, value, parseOutUnits, allowSubKeys);
     }
-  } else {
-    // case 2: array
-    for (let i = 0; i < values.length; i++) {
-      let value = (values as string[])[i];
-      assertValidValue(errorPrefix, value);
-      value = trim ? value.trim() : value;
-      setMapValues(map, value, true, false, allowSubKeys);
-    }
+  }
+
+  return map;
+}
+
+/**
+ * @param trim whether the keys should be trimmed of leading or trailing whitespace
+ * @param parseOutUnits whether units like "px" should be parsed out of the key name and appended to
+ *   the value
+ * @param allowSubKeys whether key needs to be subsplit by whitespace into multiple keys
+ * @param values values of the map
+ * @param keys keys of the map
+ * @return a normalized string map based on the input string array
+ */
+function bulidMapFromStringArray(
+    errorPrefix: string, trim: boolean, allowSubKeys: boolean,
+    keys: string[]): {[key: string]: true} {
+  const map: {[key: string]: true} = {};
+
+  for (let i = 0; i < keys.length; i++) {
+    let key = keys[i];
+    ngDevMode && assertValidValue(errorPrefix, key);
+    key = trim ? key.trim() : key;
+    setMapValues(map, key, true, false, allowSubKeys);
   }
 
   return map;
@@ -277,12 +304,12 @@ function bulidMapFromValues(
 function assertValidValue(errorPrefix: string, value: any) {
   if (typeof value !== 'string') {
     throw new Error(
-        `${errorPrefix} can only toggle CSS classes expressed as strings, got ${value}`);
+        `${errorPrefix} can only toggle CSS classes expressed as strings, got: ${value}`);
   }
 }
 
 function setMapValues(
-    map: {[key: string]: any}, key: string, value: any, parseOutUnits: boolean,
+    map: {[key: string]: unknown}, key: string, value: string | null | true, parseOutUnits: boolean,
     allowSubKeys: boolean) {
   if (allowSubKeys && key.indexOf(' ') > 0) {
     const innerKeys = key.split(/\s+/g);
@@ -295,39 +322,41 @@ function setMapValues(
 }
 
 function setIndividualMapValue(
-    map: {[key: string]: any}, key: string, value: any, parseOutUnits: boolean) {
-  if (parseOutUnits) {
-    const values = normalizeStyleKeyAndValue(key, value);
-    value = values.value;
-    key = values.key;
+    map: {[key: string]: unknown}, key: string, value: string | true | null,
+    parseOutUnits: boolean) {
+  if (parseOutUnits && typeof value === 'string') {
+    // parse out the unit (e.g. ".px") from the key and append it to the value
+    // e.g. for [width.px]="40" => ["width","40px"]
+    const unitIndex = key.indexOf('.');
+    if (unitIndex > 0) {
+      const unit = key.substr(unitIndex + 1);  // skip over the "." in "width.px"
+      key = key.substring(0, unitIndex);
+      value += unit;
+    }
   }
   map[key] = value;
 }
 
-function normalizeStyleKeyAndValue(key: string, value: string | null) {
-  const index = key.indexOf('.');
-  if (index > 0) {
-    const unit = key.substr(index + 1);  // ignore the . ([width.px]="'40'" => "40px")
-    key = key.substring(0, index);
-    if (value != null) {  // we should not convert null values to string
-      value += unit;
-    }
-  }
-  return {key, value};
-}
 
-function mapHasChanged(keys: string[], a: {[key: string]: any}, b: {[key: string]: any}) {
-  const oldKeys = Object.keys(a);
-  const newKeys = keys;
+/**
+ * Compares two maps and returns true if they are equal
+ *
+ * @param inputKeys value of `Object.keys(inputMap)` it's unclear if this actually performs better
+ * @param inputMap map to compare
+ * @param outputMap map to compare
+ */
+function mapsAreEqual(
+    inputKeys: string[], inputMap: {[key: string]: unknown},
+    outputMap: {[key: string]: unknown}, ): boolean {
+  const outputKeys = Object.keys(outputMap);
 
-  // the keys are different which means the map changed
-  if (!arrayEqualsArray(oldKeys, newKeys)) {
+  if (inputKeys.length !== outputKeys.length) {
     return true;
   }
 
-  for (let i = 0; i < newKeys.length; i++) {
-    const key = newKeys[i];
-    if (a[key] !== b[key]) {
+  for (let i = 0, n = inputKeys.length; i <= n; i++) {
+    let key = inputKeys[i];
+    if (key !== outputKeys[i] || inputMap[key] !== outputMap[key]) {
       return true;
     }
   }
@@ -335,13 +364,27 @@ function mapHasChanged(keys: string[], a: {[key: string]: any}, b: {[key: string
   return false;
 }
 
-function arrayEqualsArray(a: any[] | null, b: any[] | null) {
-  if (a && b) {
-    if (a.length !== b.length) return false;
-    for (let i = 0; i < a.length; i++) {
-      if (b.indexOf(a[i]) === -1) return false;
-    }
-    return true;
+
+/**
+ * Compares two Object.keys() arrays and returns true if they are equal.
+ *
+ * @param keyArray1 Object.keys() array to compare
+ * @param keyArray1 Object.keys() array to compare
+ */
+function keyArraysAreEqual(keyArray1: string[] | null, keyArray2: string[] | null): boolean {
+  if (!Array.isArray(keyArray1) || !Array.isArray(keyArray2)) {
+    return false;
   }
-  return false;
+
+  if (keyArray1.length !== keyArray2.length) {
+    return false;
+  }
+
+  for (let i = 0; i < keyArray1.length; i++) {
+    if (keyArray1[i] !== keyArray2[i]) {
+      return false;
+    }
+  }
+
+  return true;
 }
diff --git a/packages/common/test/directives/ng_class_spec.ts b/packages/common/test/directives/ng_class_spec.ts
index 4d83a6ee21..d661c87cad 100644
--- a/packages/common/test/directives/ng_class_spec.ts
+++ b/packages/common/test/directives/ng_class_spec.ts
@@ -196,7 +196,7 @@ import {ComponentFixture, TestBed, async} from '@angular/core/testing';
         fixture = createTestComponent(`<div [ngClass]="['foo', {}]"></div>`);
         expect(() => fixture !.detectChanges())
             .toThrowError(
-                /NgClass can only toggle CSS classes expressed as strings, got \[object Object\]/);
+                /NgClass can only toggle CSS classes expressed as strings, got: \[object Object\]/);
       });
     });
 
@@ -353,17 +353,23 @@ import {ComponentFixture, TestBed, async} from '@angular/core/testing';
          }));
     });
 
-    describe('non-regression', () => {
+    describe('prevent regressions', () => {
 
       // https://github.com/angular/angular/issues/34336
-      it('should not write to native node when a bound expression doesnt change', () => {
-        fixture = createTestComponent(`<div [ngClass]="{'color-red': true}"></div>`);
+      it('should not write to the native node unless the bound expression has changed', () => {
+        fixture = createTestComponent(`<div [ngClass]="{'color-red': condition}"></div>`);
         detectChangesAndExpectClassName('color-red');
 
-        // Overwrite CSS classes to make sure that ngClass is not doing any DOM manipulation (as
-        // there was no change to the expression bound to [ngClass]).
+        // Overwrite CSS classes so that we can check if ngClass performed DOM manipulation to
+        // update it
         fixture.debugElement.children[0].nativeElement.className = '';
+        // Assert that the DOM node still has the same value after change detection
         detectChangesAndExpectClassName('');
+
+        fixture.componentInstance.condition = false;
+        fixture.detectChanges();
+        fixture.componentInstance.condition = true;
+        detectChangesAndExpectClassName('color-red');
       });
 
     });
diff --git a/packages/common/test/directives/ng_style_spec.ts b/packages/common/test/directives/ng_style_spec.ts
index 2af393888e..aaba40cd80 100644
--- a/packages/common/test/directives/ng_style_spec.ts
+++ b/packages/common/test/directives/ng_style_spec.ts
@@ -12,7 +12,7 @@ import {ComponentFixture, TestBed, async} from '@angular/core/testing';
 
 {
   describe('NgStyle', () => {
-    let fixture: ComponentFixture<any>;
+    let fixture: ComponentFixture<TestComponent>;
 
     function getComponent(): TestComponent { return fixture.componentInstance; }
 
@@ -158,21 +158,36 @@ import {ComponentFixture, TestBed, async} from '@angular/core/testing';
          expectNativeEl(fixture).toHaveCssStyle({'font-size': '12px'});
        }));
 
-    it('should not write to native node when a bound expression doesnt change', () => {
+    it('should not write to the native node unless the bound expression has changed', () => {
 
-      const template = `<div [ngStyle]="{'color': 'red'}"></div>`;
+      const template = `<div [ngStyle]="{'color': expr}"></div>`;
 
       fixture = createTestComponent(template);
+      fixture.componentInstance.expr = 'red';
 
       fixture.detectChanges();
       expectNativeEl(fixture).toHaveCssStyle({'color': 'red'});
 
-      // Overwrite native styles to make sure that ngClass is not doing any DOM manipulation (as
-      // there was no change to the expression bound to [ngStyle]).
+      // Overwrite native styles so that we can check if ngStyle has performed DOM manupulation to
+      // update it.
       fixture.debugElement.children[0].nativeElement.style.color = 'blue';
       fixture.detectChanges();
+      // Assert that the style hasn't been updated
       expectNativeEl(fixture).toHaveCssStyle({'color': 'blue'});
 
+      fixture.componentInstance.expr = 'yellow';
+      fixture.detectChanges();
+      // Assert that the style has changed now that the model has changed
+      expectNativeEl(fixture).toHaveCssStyle({'color': 'yellow'});
+    });
+
+    it('should correctly update style with units (.px) when the model is set to number', () => {
+      const template = `<div [ngStyle]="{'width.px': expr}"></div>`;
+      fixture = createTestComponent(template);
+      fixture.componentInstance.expr = 400;
+
+      fixture.detectChanges();
+      expectNativeEl(fixture).toHaveCssStyle({'width': '400px'});
     });
 
   });
diff --git a/packages/common/test/directives/styling_differ_spec.ts b/packages/common/test/directives/styling_differ_spec.ts
index 0a9c9da53f..a4745b02f4 100644
--- a/packages/common/test/directives/styling_differ_spec.ts
+++ b/packages/common/test/directives/styling_differ_spec.ts
@@ -13,101 +13,111 @@ describe('StylingDiffer', () => {
         'ngClass', StylingDifferOptions.ForceAsMap | StylingDifferOptions.AllowStringValue);
     expect(d.value).toEqual(null);
 
-    d.setValue('one two');
+    d.setInput('one two');
     expect(d.value).toEqual({one: true, two: true});
 
-    d.setValue('three');
+    d.setInput('three');
     expect(d.value).toEqual({three: true});
   });
 
-  it('should not emit that a value has changed if a new non-collection value was not set', () => {
-    const d = new StylingDiffer(
-        'ngClass', StylingDifferOptions.ForceAsMap | StylingDifferOptions.AllowStringValue);
-    expect(d.value).toEqual(null);
 
-    d.setValue('one two');
-    expect(d.hasValueChanged()).toBeTruthy();
-    expect(d.value).toEqual({one: true, two: true});
-    expect(d.hasValueChanged()).toBeFalsy();
-    expect(d.value).toEqual({one: true, two: true});
+  describe('setInput', () => {
 
-    d.setValue('three');
-    expect(d.hasValueChanged()).toBeTruthy();
-    expect(d.value).toEqual({three: true});
-    expect(d.hasValueChanged()).toBeFalsy();
-    expect(d.value).toEqual({three: true});
+    it('should not emit that a value has changed if a new non-collection value was not set', () => {
+      const d = new StylingDiffer(
+          'ngClass', StylingDifferOptions.ForceAsMap | StylingDifferOptions.AllowStringValue);
+      expect(d.value).toEqual(null);
 
-    d.setValue(null);
-    expect(d.hasValueChanged()).toBeTruthy();
-    expect(d.value).toEqual(null);
-    expect(d.hasValueChanged()).toBeFalsy();
-    expect(d.value).toEqual(null);
+      d.setInput('one two');
+      expect(d.updateValue()).toBeTruthy();
+      expect(d.value).toEqual({one: true, two: true});
+      expect(d.updateValue()).toBeFalsy();
+      expect(d.value).toEqual({one: true, two: true});
+
+      d.setInput('three');
+      expect(d.updateValue()).toBeTruthy();
+      expect(d.value).toEqual({three: true});
+      expect(d.updateValue()).toBeFalsy();
+      expect(d.value).toEqual({three: true});
+
+      d.setInput(null);
+      expect(d.updateValue()).toBeTruthy();
+      expect(d.value).toEqual(null);
+      expect(d.updateValue()).toBeFalsy();
+      expect(d.value).toEqual(null);
+    });
   });
 
-  it('should watch the contents of a StringMap value and emit new values if they change', () => {
-    const d = new StylingDiffer('ngClass', StylingDifferOptions.ForceAsMap);
 
-    const myMap: {[key: string]: any} = {};
-    myMap['abc'] = true;
+  describe('updateValue', () => {
 
-    d.setValue(myMap);
-    expect(d.hasValueChanged()).toBeTruthy();
-    expect(d.value).toEqual({abc: true});
-    expect(d.hasValueChanged()).toBeFalsy();
+    it('should update the differ value if the contents of a input StringMap change', () => {
+      const d = new StylingDiffer('ngClass', StylingDifferOptions.ForceAsMap);
 
-    myMap['def'] = true;
-    expect(d.hasValueChanged()).toBeTruthy();
-    expect(d.value).toEqual({abc: true, def: true});
-    expect(d.hasValueChanged()).toBeFalsy();
+      const myMap: {[key: string]: true} = {};
+      myMap['abc'] = true;
 
-    delete myMap['abc'];
-    delete myMap['def'];
-    expect(d.hasValueChanged()).toBeTruthy();
-    expect(d.value).toEqual({});
-    expect(d.hasValueChanged()).toBeFalsy();
-  });
+      d.setInput(myMap);
+      expect(d.updateValue()).toBeTruthy();
+      expect(d.value).toEqual({abc: true});
+      expect(d.updateValue()).toBeFalsy();
 
-  it('should watch the contents of an Array value and emit new values if they change', () => {
-    const d = new StylingDiffer('ngClass', StylingDifferOptions.ForceAsMap);
+      myMap['def'] = true;
+      expect(d.updateValue()).toBeTruthy();
+      expect(d.value).toEqual({abc: true, def: true});
+      expect(d.updateValue()).toBeFalsy();
 
-    const myArray: string[] = [];
-    myArray.push('abc');
+      delete myMap['abc'];
+      delete myMap['def'];
+      expect(d.updateValue()).toBeTruthy();
+      expect(d.value).toEqual({});
+      expect(d.updateValue()).toBeFalsy();
+    });
 
-    d.setValue(myArray);
-    expect(d.hasValueChanged()).toBeTruthy();
-    expect(d.value).toEqual({abc: true});
-    expect(d.hasValueChanged()).toBeFalsy();
 
-    myArray.push('def');
-    expect(d.hasValueChanged()).toBeTruthy();
-    expect(d.value).toEqual({abc: true, def: true});
-    expect(d.hasValueChanged()).toBeFalsy();
+    it('should update the differ value if the contents of an input Array change', () => {
+      const d = new StylingDiffer('ngClass', StylingDifferOptions.ForceAsMap);
 
-    myArray.length = 0;
-    expect(d.hasValueChanged()).toBeTruthy();
-    expect(d.value).toEqual({});
-    expect(d.hasValueChanged()).toBeFalsy();
-  });
+      const myArray: string[] = [];
+      myArray.push('abc');
 
-  it('should watch the contents of a Set value and emit new values if they change', () => {
-    const d = new StylingDiffer('ngClass', StylingDifferOptions.ForceAsMap);
+      d.setInput(myArray);
+      expect(d.updateValue()).toBeTruthy();
+      expect(d.value).toEqual({abc: true});
+      expect(d.updateValue()).toBeFalsy();
 
-    const mySet = new Set<string>();
-    mySet.add('abc');
+      myArray.push('def');
+      expect(d.updateValue()).toBeTruthy();
+      expect(d.value).toEqual({abc: true, def: true});
+      expect(d.updateValue()).toBeFalsy();
 
-    d.setValue(mySet);
-    expect(d.hasValueChanged()).toBeTruthy();
-    expect(d.value).toEqual({abc: true});
-    expect(d.hasValueChanged()).toBeFalsy();
+      myArray.length = 0;
+      expect(d.updateValue()).toBeTruthy();
+      expect(d.value).toEqual({});
+      expect(d.updateValue()).toBeFalsy();
+    });
 
-    mySet.add('def');
-    expect(d.hasValueChanged()).toBeTruthy();
-    expect(d.value).toEqual({abc: true, def: true});
-    expect(d.hasValueChanged()).toBeFalsy();
 
-    mySet.clear();
-    expect(d.hasValueChanged()).toBeTruthy();
-    expect(d.value).toEqual({});
-    expect(d.hasValueChanged()).toBeFalsy();
+    it('should update the differ value if the contents of an input Set change', () => {
+      const d = new StylingDiffer('ngClass', StylingDifferOptions.ForceAsMap);
+
+      const mySet = new Set<string>();
+      mySet.add('abc');
+
+      d.setInput(mySet);
+      expect(d.updateValue()).toBeTruthy();
+      expect(d.value).toEqual({abc: true});
+      expect(d.updateValue()).toBeFalsy();
+
+      mySet.add('def');
+      expect(d.updateValue()).toBeTruthy();
+      expect(d.value).toEqual({abc: true, def: true});
+      expect(d.updateValue()).toBeFalsy();
+
+      mySet.clear();
+      expect(d.updateValue()).toBeTruthy();
+      expect(d.value).toEqual({});
+      expect(d.updateValue()).toBeFalsy();
+    });
   });
 });
diff --git a/packages/compiler-cli/ngcc/test/integration/ngcc_spec.ts b/packages/compiler-cli/ngcc/test/integration/ngcc_spec.ts
index d571f16952..ec676e814f 100644
--- a/packages/compiler-cli/ngcc/test/integration/ngcc_spec.ts
+++ b/packages/compiler-cli/ngcc/test/integration/ngcc_spec.ts
@@ -257,8 +257,7 @@ runInEachFileSystem(() => {
 
          // We need to make sure that the flat typings file exports this directly
          const dtsContents = fs.readFile(_('/node_modules/@angular/common/common.d.ts'));
-         expect(dtsContents)
-             .toContain(`export declare class ${exportedName} implements ɵNgClassImpl`);
+         expect(dtsContents).toContain(`export declare class ${exportedName} extends ɵNgClassImpl`);
          // And that ngcc's modifications to that class use the correct (exported) name
          expect(dtsContents).toContain(`static ɵprov: ɵngcc0.ɵɵInjectableDef<${exportedName}>`);
        });