Refactor Attribute Processing (Step 2)

Summary: Move the ViewAttributes and StyleAttributes configuration into the
Components library since they're coupled and change with the
native component configuration.

This also decouples StyleAttributes from the reconciler by adding it
to the ReactViewAttributes.

To do that, I refactored the property diffing to allow for recursive
configurations. Now an attribute configuration can be a nested object,
a custom configuration (diff/process) or true.

The requireNativeComponent path incorrectly gets its attributes set up
on the root validAttributes instead of the nested style object.
So I also have to add the nested form. Effectively these currently allow
these attributes on props or nested.

@​public

Reviewed By: @vjeux

Differential Revision: D2456842

fb-gh-sync-id: cd5405bd8316c2fcb016d06c61244ce7719c26c0

Libraries/Components/View/ReactNativeViewAttributes.js

@@ -11,7 +11,7 @@
  */
 'use strict';
 
-var merge = require('merge');
+var ReactNativeStyleAttributes = require('ReactNativeStyleAttributes');
 
 var ReactNativeViewAttributes = {};
 
@@ -31,10 +31,11 @@ ReactNativeViewAttributes.UIView = {
   onMagicTap: true,
   collapsable: true,
   needsOffscreenAlphaCompositing: true,
+  style: ReactNativeStyleAttributes,
 };
 
-ReactNativeViewAttributes.RCTView = merge(
-  ReactNativeViewAttributes.UIView, {
+ReactNativeViewAttributes.RCTView = {
+  ...ReactNativeViewAttributes.UIView,
 
   // This is a special performance property exposed by RCTView and useful for
   // scrolling content when there are many subviews, most of which are offscreen.
@@ -42,6 +43,6 @@ ReactNativeViewAttributes.RCTView = merge(
   // many subviews that extend outside its bound. The subviews must also have
   // overflow: hidden, as should the containing view (or one of its superviews).
   removeClippedSubviews: true,
-});
+};
 
 module.exports = ReactNativeViewAttributes;

Libraries/ReactIOS/requireNativeComponent.js

@@ -12,6 +12,7 @@
 'use strict';
 
 var RCTUIManager = require('NativeModules').UIManager;
+var ReactNativeStyleAttributes = require('ReactNativeStyleAttributes');
 var UnimplementedView = require('UnimplementedView');
 
 var createReactNativeComponentClass = require('createReactNativeComponentClass');
@@ -75,6 +76,14 @@ function requireNativeComponent(
 
     viewConfig.validAttributes[key] = useAttribute ? attribute : true;
   }
+
+  // Unfortunately, the current set up puts the style properties on the top
+  // level props object. We also need to add the nested form for API
+  // compatibility. This allows these props on both the top level and the
+  // nested style level. TODO: Move these to nested declarations on the
+  // native side.
+  viewConfig.validAttributes.style = ReactNativeStyleAttributes;
+
   if (__DEV__) {
     componentInterface && verifyPropTypes(
       componentInterface,

Libraries/ReactNative/ReactNativeAttributePayload.js

@@ -11,180 +11,277 @@
  */
 'use strict';
 
-var ReactNativeStyleAttributes = require('ReactNativeStyleAttributes');
-
 var deepDiffer = require('deepDiffer');
 var styleDiffer = require('styleDiffer');
-var deepFreezeAndThrowOnMutationInDev = require('deepFreezeAndThrowOnMutationInDev');
 var flattenStyle = require('flattenStyle');
 var precomputeStyle = require('precomputeStyle');
 
+type AttributeDiffer = (prevProp : mixed, nextProp : mixed) => boolean;
+type AttributePreprocessor = (nextProp: mixed) => mixed;
+
+type CustomAttributeConfiguration =
+  { diff : AttributeDiffer, process : AttributePreprocessor } |
+  { diff : AttributeDiffer } |
+  { process : AttributePreprocessor };
+
+type AttributeConfiguration =
+  { [key : string]: (
+    CustomAttributeConfiguration | AttributeConfiguration /*| boolean*/
+  ) };
+
+function defaultDiffer(prevProp: mixed, nextProp: mixed) : boolean {
+  if (typeof nextProp !== 'object' || nextProp === null) {
+    // Scalars have already been checked for equality
+    return true;
+  } else {
+    // For objects and arrays, the default diffing algorithm is a deep compare
+    return deepDiffer(prevProp, nextProp);
+  }
+}
+
+function diffNestedProperty(
+  updatePayload :? Object,
+  prevProp, // inferred
+  nextProp, // inferred
+  validAttributes : AttributeConfiguration
+) : ?Object {
+  // The style property is a deeply nested element which includes numbers
+  // to represent static objects. Most of the time, it doesn't change across
+  // renders, so it's faster to spend the time checking if it is different
+  // before actually doing the expensive flattening operation in order to
+  // compute the diff.
+  if (!styleDiffer(prevProp, nextProp)) {
+    return updatePayload;
+  }
+
+  // TODO: Walk both props in parallel instead of flattening.
+  // TODO: Move precomputeStyle to .process for each attribute.
+
+  var previousFlattenedStyle = precomputeStyle(
+    flattenStyle(prevProp),
+    validAttributes
+  );
+
+  var nextFlattenedStyle = precomputeStyle(
+    flattenStyle(nextProp),
+    validAttributes
+  );
+
+  if (!previousFlattenedStyle || !nextFlattenedStyle) {
+    if (nextFlattenedStyle) {
+      return addProperties(
+        updatePayload,
+        nextFlattenedStyle,
+        validAttributes
+      );
+    }
+    if (previousFlattenedStyle) {
+      return clearProperties(
+        updatePayload,
+        previousFlattenedStyle,
+        validAttributes
+      );
+    }
+    return updatePayload;
+  }
+
+  // recurse
+  return diffProperties(
+    updatePayload,
+    previousFlattenedStyle,
+    nextFlattenedStyle,
+    validAttributes
+  );
+}
+
 /**
- * diffRawProperties takes two sets of props and a set of valid attributes
- * and write to updatePayload the values that changed or were deleted
- *
- * @param {?object} updatePayload Overriden with the props that changed.
- * @param {!object} prevProps Previous properties to diff against current
- * properties. These properties are as supplied to component construction.
- * @param {!object} prevProps Next "current" properties to diff against
- * previous. These properties are as supplied to component construction.
- * @return {?object}
+ * addNestedProperty takes a single set of props and valid attribute
+ * attribute configurations. It processes each prop and adds it to the
+ * updatePayload.
  */
-function diffRawProperties(
-  updatePayload: ?Object,
-  prevProps: ?Object,
-  nextProps: ?Object,
-  validAttributes: Object
+/*
+function addNestedProperty(
+  updatePayload :? Object,
+  nextProp : Object,
+  validAttributes : AttributeConfiguration
+) {
+  // TODO: Fast path
+  return diffNestedProperty(updatePayload, {}, nextProp, validAttributes);
+}
+*/
+
+/**
+ * clearNestedProperty takes a single set of props and valid attributes. It
+ * adds a null sentinel to the updatePayload, for each prop key.
+ */
+function clearNestedProperty(
+  updatePayload :? Object,
+  prevProp : Object,
+  validAttributes : AttributeConfiguration
+) : ?Object {
+  // TODO: Fast path
+  return diffNestedProperty(updatePayload, prevProp, {}, validAttributes);
+}
+
+/**
+ * diffProperties takes two sets of props and a set of valid attributes
+ * and write to updatePayload the values that changed or were deleted.
+ * If no updatePayload is provided, a new one is created and returned if
+ * anything changed.
+ */
+function diffProperties(
+  updatePayload : ?Object,
+  prevProps : Object,
+  nextProps : Object,
+  validAttributes : AttributeConfiguration
 ): ?Object {
-  var validAttributeConfig;
+  var attributeConfig : ?AttributeConfiguration;
   var nextProp;
   var prevProp;
-  var isScalar;
-  var shouldUpdate;
-  var differ;
 
-  if (nextProps) {
-    for (var propKey in nextProps) {
-      validAttributeConfig = validAttributes[propKey];
-      if (!validAttributeConfig) {
-        continue; // not a valid native prop
-      }
-      prevProp = prevProps && prevProps[propKey];
-      nextProp = nextProps[propKey];
+  for (var propKey in nextProps) {
+    attributeConfig = validAttributes[propKey];
+    if (!attributeConfig) {
+      continue; // not a valid native prop
+    }
+    if (updatePayload && updatePayload[propKey] !== undefined) {
+      // If we're in a nested attribute set, we may have set this property
+      // already. If so, bail out. The previous update is what counts.
+      continue;
+    }
+    prevProp = prevProps[propKey];
+    nextProp = nextProps[propKey];
 
-      // functions are converted to booleans as markers that the associated
-      // events should be sent from native.
+    // functions are converted to booleans as markers that the associated
+    // events should be sent from native.
+    if (typeof nextProp === 'function') {
+      nextProp = true;
+      // If nextProp is not a function, then don't bother changing prevProp
+      // since nextProp will win and go into the updatePayload regardless.
       if (typeof prevProp === 'function') {
         prevProp = true;
       }
-      if (typeof nextProp === 'function') {
-        nextProp = true;
-      }
+    }
 
-      if (prevProp !== nextProp) {
-        // Scalars and new props are always updated.  Objects use deepDiffer by
-        // default, but can be optimized with custom differs.
-        differ = validAttributeConfig.diff || deepDiffer;
-        isScalar = typeof nextProp !== 'object' || nextProp === null;
-        shouldUpdate = isScalar || !prevProp || differ(prevProp, nextProp);
-        if (shouldUpdate) {
-          updatePayload = updatePayload || {};
-          updatePayload[propKey] = nextProp;
-        }
+    if (prevProp === nextProp) {
+      continue; // nothing changed
+    }
+
+    // Pattern match on: attributeConfig
+    if (typeof attributeConfig !== 'object') {
+      // case: !Object is the default case
+      if (defaultDiffer(prevProp, nextProp)) {
+        // a normal leaf has changed
+        (updatePayload || (updatePayload = {}))[propKey] = nextProp;
       }
+    } else if (typeof attributeConfig.diff === 'function' ||
+               typeof attributeConfig.process === 'function') {
+      // case: CustomAttributeConfiguration
+      var shouldUpdate = prevProp === undefined || (
+        typeof attributeConfig.diff === 'function' ?
+        attributeConfig.diff(prevProp, nextProp) :
+        defaultDiffer(prevProp, nextProp)
+      );
+      if (shouldUpdate) {
+        var nextValue = typeof attributeConfig.process === 'function' ?
+                        attributeConfig.process(nextProp) :
+                        nextProp;
+        (updatePayload || (updatePayload = {}))[propKey] = nextValue;
+      }
+    } else {
+      // default: fallthrough case when nested properties are defined
+      updatePayload = diffNestedProperty(
+        updatePayload,
+        prevProp,
+        nextProp,
+        attributeConfig
+      );
     }
   }
 
   // Also iterate through all the previous props to catch any that have been
   // removed and make sure native gets the signal so it can reset them to the
   // default.
-  if (prevProps) {
-    for (var propKey in prevProps) {
-      validAttributeConfig = validAttributes[propKey];
-      if (!validAttributeConfig) {
-        continue; // not a valid native prop
-      }
-      if (updatePayload && updatePayload[propKey] !== undefined) {
-        continue; // Prop already specified
-      }
-      prevProp = prevProps[propKey];
-      nextProp = nextProps && nextProps[propKey];
-
-      // functions are converted to booleans as markers that the associated
-      // events should be sent from native.
-      if (typeof prevProp === 'function') {
-        prevProp = true;
-      }
-      if (typeof nextProp === 'function') {
-        nextProp = true;
-      }
-
-      if (prevProp !== nextProp) {
-        if (nextProp === undefined) {
-          nextProp = null; // null is a sentinel we explicitly send to native
-        }
-        // Scalars and new props are always updated.  Objects use deepDiffer by
-        // default, but can be optimized with custom differs.
-        differ = validAttributeConfig.diff || deepDiffer;
-        isScalar = typeof nextProp !== 'object' || nextProp === null;
-        shouldUpdate =
-          isScalar &&
-          prevProp !== nextProp ||
-          differ(prevProp, nextProp);
-        if (shouldUpdate) {
-          updatePayload = updatePayload || {};
-          updatePayload[propKey] = nextProp;
-        }
-      }
+  for (var propKey in prevProps) {
+    if (nextProps[propKey] !== undefined) {
+      continue; // we've already covered this key in the previous pass
+    }
+    attributeConfig = validAttributes[propKey];
+    if (!attributeConfig) {
+      continue; // not a valid native prop
+    }
+    prevProp = prevProps[propKey];
+    if (prevProp === undefined) {
+      continue; // was already empty anyway
+    }
+    // Pattern match on: attributeConfig
+    if (typeof attributeConfig !== 'object' ||
+        typeof attributeConfig.diff === 'function' ||
+        typeof attributeConfig.process === 'function') {
+      // case: CustomAttributeConfiguration | !Object
+      // Flag the leaf property for removal by sending a sentinel.
+      (updatePayload || (updatePayload = {}))[propKey] = null;
+    } else {
+      // default:
+      // This is a nested attribute configuration where all the properties
+      // were removed so we need to go through and clear out all of them.
+      updatePayload = clearNestedProperty(
+        updatePayload,
+        prevProp,
+        attributeConfig
+      );
     }
   }
   return updatePayload;
 }
 
+/**
+ * addProperties adds all the valid props to the payload after being processed.
+ */
+function addProperties(
+  updatePayload : ?Object,
+  props : Object,
+  validAttributes : AttributeConfiguration
+) : ?Object {
+  return diffProperties(updatePayload, {}, props, validAttributes);
+}
+
+/**
+ * clearProperties clears all the previous props by adding a null sentinel
+ * to the payload for each valid key.
+ */
+function clearProperties(
+  updatePayload : ?Object,
+  prevProps : Object,
+  validAttributes : AttributeConfiguration
+) :? Object {
+  return diffProperties(updatePayload, prevProps, {}, validAttributes);
+}
+
 var ReactNativeAttributePayload = {
 
   create: function(
     props : Object,
-    validAttributes : Object
+    validAttributes : AttributeConfiguration
   ) : ?Object {
-    return ReactNativeAttributePayload.diff({}, props, validAttributes);
+    return addProperties(
+      null, // updatePayload
+      props,
+      validAttributes
+    );
   },
 
   diff: function(
     prevProps : Object,
     nextProps : Object,
-    validAttributes : Object
+    validAttributes : AttributeConfiguration
   ) : ?Object {
-
-    if (__DEV__) {
-      for (var key in nextProps) {
-        if (nextProps.hasOwnProperty(key) &&
-            nextProps[key] &&
-            validAttributes[key]) {
-          deepFreezeAndThrowOnMutationInDev(nextProps[key]);
-        }
-      }
-    }
-
-    var updatePayload = diffRawProperties(
+    return diffProperties(
       null, // updatePayload
       prevProps,
       nextProps,
       validAttributes
     );
-
-    for (var key in updatePayload) {
-      var process = validAttributes[key] && validAttributes[key].process;
-      if (process) {
-        updatePayload[key] = process(updatePayload[key]);
-      }
-    }
-
-    // The style property is a deeply nested element which includes numbers
-    // to represent static objects. Most of the time, it doesn't change across
-    // renders, so it's faster to spend the time checking if it is different
-    // before actually doing the expensive flattening operation in order to
-    // compute the diff.
-    if (styleDiffer(nextProps.style, prevProps.style)) {
-      // TODO: Use a cached copy of previousFlattenedStyle, or walk both
-      // props in parallel.
-      var previousFlattenedStyle = precomputeStyle(
-        flattenStyle(prevProps.style),
-        validAttributes
-      );
-      var nextFlattenedStyle = precomputeStyle(
-        flattenStyle(nextProps.style),
-        validAttributes
-      );
-      updatePayload = diffRawProperties(
-        updatePayload,
-        previousFlattenedStyle,
-        nextFlattenedStyle,
-        ReactNativeStyleAttributes
-      );
-    }
-
-    return updatePayload;
   }
 
 };

Libraries/ReactNative/ReactNativeBaseComponent.js

@@ -19,6 +19,7 @@ var ReactNativeTagHandles = require('ReactNativeTagHandles');
 var ReactMultiChild = require('ReactMultiChild');
 var RCTUIManager = require('NativeModules').UIManager;
 
+var deepFreezeAndThrowOnMutationInDev = require('deepFreezeAndThrowOnMutationInDev');
 var warning = require('warning');
 
 var registrationNames = ReactNativeEventEmitter.registrationNames;
@@ -139,6 +140,10 @@ ReactNativeBaseComponent.Mixin = {
     var prevElement = this._currentElement;
     this._currentElement = nextElement;
 
+    if (__DEV__) {
+      deepFreezeAndThrowOnMutationInDev(this._currentElement.props);
+    }
+
     var updatePayload = ReactNativeAttributePayload.diff(
       prevElement.props,
       nextElement.props,
@@ -201,6 +206,10 @@ ReactNativeBaseComponent.Mixin = {
 
     var tag = ReactNativeTagHandles.allocateTag();
 
+    if (__DEV__) {
+      deepFreezeAndThrowOnMutationInDev(this._currentElement.props);
+    }
+
     var updatePayload = ReactNativeAttributePayload.create(
       this._currentElement.props,
       this.viewConfig.validAttributes

Libraries/ReactNative/__tests__/ReactNativeAttributePayload-test.js

@@ -38,6 +38,14 @@ describe('ReactNativeAttributePayload', function() {
     )).toEqual({a: null});
   });
 
+  it('should remove fields that are set to undefined', () => {
+    expect(diff(
+      {a: 1},
+      {a: undefined},
+      {a: true}
+    )).toEqual({a: null});
+  });
+
   it('should ignore invalid fields', () => {
     expect(diff(
       {a: 1},
@@ -80,19 +88,19 @@ describe('ReactNativeAttributePayload', function() {
 
   it('should work with undefined styles', () => {
     expect(diff(
-      { style: { a: '#ffffff', opacity: 1 } },
+      { style: { a: '#ffffff', b: 1 } },
       { style: undefined },
-      { }
-    )).toEqual({ opacity: null });
+      { style: { b: true } }
+    )).toEqual({ b: null });
     expect(diff(
       { style: undefined },
-      { style: { a: '#ffffff', opacity: 1 } },
-      { }
-    )).toEqual({ opacity: 1 });
+      { style: { a: '#ffffff', b: 1 } },
+      { style: { b: true } }
+    )).toEqual({ b: 1 });
     expect(diff(
       { style: undefined },
       { style: undefined },
-      { }
+      { style: { b: true } }
     )).toEqual(null);
   });