Refactor Attribute Processing (Step 3)

sebmarkbage · facebook-github-bot-4 · commit ac5b7548d2ea · 2015-10-06T10:28:41.000-07:00
Summary: Decouple processStyle from the main reconciliation. It is now a process extension to the style attribute `transform`. This effectively decouples a large portion of special cases and helper dependencies from the reconciler. The transform attribute becomes translated into the transformMatrix attribute on the native side so this becomes a little weird in that I have to special case it. I don't think it is worth while having a general solution for this so I intend to rename the native attribute to `transform` and just have it accept the resolved transform. Then I can remove the special cases. The next step is generalizing the flattenStyle function and optimizing it. @​public Reviewed By: @vjeux Differential Revision: D2460465 fb-gh-sync-id: 243e7fd77d282b401bc2c028aec8d57f24522a8e
diff --git a/Libraries/Components/View/ReactNativeStyleAttributes.js b/Libraries/Components/View/ReactNativeStyleAttributes.js
@@ -19,6 +19,7 @@ var ViewStylePropTypes = require('ViewStylePropTypes');
 var keyMirror = require('keyMirror');
 var matricesDiffer = require('matricesDiffer');
 var processColor = require('processColor');
+var processTransform = require('processTransform');
 var sizesDiffer = require('sizesDiffer');
 
 var ReactNativeStyleAttributes = {
@@ -27,6 +28,7 @@ var ReactNativeStyleAttributes = {
   ...keyMirror(ImageStylePropTypes),
 };
 
+ReactNativeStyleAttributes.transform = { process: processTransform };
 ReactNativeStyleAttributes.transformMatrix = { diff: matricesDiffer };
 ReactNativeStyleAttributes.shadowOffset = { diff: sizesDiffer };
 
diff --git a/Libraries/ReactIOS/NativeMethodsMixin.js b/Libraries/ReactIOS/NativeMethodsMixin.js
@@ -17,10 +17,7 @@ var ReactNativeAttributePayload = require('ReactNativeAttributePayload');
 var TextInputState = require('TextInputState');
 
 var findNodeHandle = require('findNodeHandle');
-var flattenStyle = require('flattenStyle');
 var invariant = require('invariant');
-var mergeFast = require('mergeFast');
-var precomputeStyle = require('precomputeStyle');
 
 type MeasureOnSuccessCallback = (
   x: number,
diff --git a/Libraries/ReactNative/ReactNativeAttributePayload.js b/Libraries/ReactNative/ReactNativeAttributePayload.js
@@ -11,10 +11,11 @@
  */
 'use strict';
 
+var Platform = require('Platform');
+
 var deepDiffer = require('deepDiffer');
 var styleDiffer = require('styleDiffer');
 var flattenStyle = require('flattenStyle');
-var precomputeStyle = require('precomputeStyle');
 
 type AttributeDiffer = (prevProp : mixed, nextProp : mixed) => boolean;
 type AttributePreprocessor = (nextProp: mixed) => mixed;
@@ -29,6 +30,21 @@ type AttributeConfiguration =
     CustomAttributeConfiguration | AttributeConfiguration /*| boolean*/
   ) };
 
+function translateKey(propKey : string) : string {
+  if (propKey === 'transform') {
+    // We currently special case the key for `transform`. iOS uses the
+    // transformMatrix name and Android uses the decomposedMatrix name.
+    // TODO: We could unify these names and just use the name `transform`
+    // all the time. Just need to update the native side.
+    if (Platform.OS === 'android') {
+      return 'decomposedMatrix';
+    } else {
+      return 'transformMatrix';
+    }
+  }
+  return propKey;
+}
+
 function defaultDiffer(prevProp: mixed, nextProp: mixed) : boolean {
   if (typeof nextProp !== 'object' || nextProp === null) {
     // Scalars have already been checked for equality
@@ -55,17 +71,9 @@ function diffNestedProperty(
   }
 
   // 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
-  );
+  var previousFlattenedStyle = flattenStyle(prevProp);
+  var nextFlattenedStyle = flattenStyle(nextProp);
 
   if (!previousFlattenedStyle || !nextFlattenedStyle) {
     if (nextFlattenedStyle) {
@@ -144,7 +152,14 @@ function diffProperties(
     if (!attributeConfig) {
       continue; // not a valid native prop
     }
-    if (updatePayload && updatePayload[propKey] !== undefined) {
+
+    var altKey = translateKey(propKey);
+    if (!attributeConfig[altKey]) {
+      // If there is no config for the alternative, bail out. Helps ART.
+      altKey = propKey;
+    }
+
+    if (updatePayload && updatePayload[altKey] !== 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;
@@ -172,7 +187,7 @@ function diffProperties(
       // case: !Object is the default case
       if (defaultDiffer(prevProp, nextProp)) {
         // a normal leaf has changed
-        (updatePayload || (updatePayload = {}))[propKey] = nextProp;
+        (updatePayload || (updatePayload = {}))[altKey] = nextProp;
       }
     } else if (typeof attributeConfig.diff === 'function' ||
                typeof attributeConfig.process === 'function') {
@@ -186,7 +201,7 @@ function diffProperties(
         var nextValue = typeof attributeConfig.process === 'function' ?
                         attributeConfig.process(nextProp) :
                         nextProp;
-        (updatePayload || (updatePayload = {}))[propKey] = nextValue;
+        (updatePayload || (updatePayload = {}))[altKey] = nextValue;
       }
     } else {
       // default: fallthrough case when nested properties are defined
@@ -210,6 +225,7 @@ function diffProperties(
     if (!attributeConfig) {
       continue; // not a valid native prop
     }
+
     prevProp = prevProps[propKey];
     if (prevProp === undefined) {
       continue; // was already empty anyway
@@ -218,9 +234,10 @@ function diffProperties(
     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;
+      (updatePayload || (updatePayload = {}))[translateKey(propKey)] = null;
     } else {
       // default:
       // This is a nested attribute configuration where all the properties
diff --git a/Libraries/ReactNative/__tests__/ReactNativeAttributePayload-test.js b/Libraries/ReactNative/__tests__/ReactNativeAttributePayload-test.js
@@ -4,11 +4,13 @@
 'use strict';
 
 jest.dontMock('ReactNativeAttributePayload');
+jest.dontMock('StyleSheetRegistry');
 jest.dontMock('deepDiffer');
-jest.dontMock('styleDiffer');
-jest.dontMock('precomputeStyle');
 jest.dontMock('flattenStyle');
+jest.dontMock('styleDiffer');
+
 var ReactNativeAttributePayload = require('ReactNativeAttributePayload');
+var StyleSheetRegistry = require('StyleSheetRegistry');
 
 var diff = ReactNativeAttributePayload.diff;
 
@@ -122,6 +124,94 @@ describe('ReactNativeAttributePayload', function() {
     )).toEqual(null);
   });
 
+  it('should flatten nested styles and predefined styles', () => {
+    var validStyleAttribute = { someStyle: { foo: true, bar: true } };
+
+    expect(diff(
+      {},
+      { someStyle: [{ foo: 1 }, { bar: 2 }]},
+      validStyleAttribute
+    )).toEqual({ foo: 1, bar: 2 });
+
+    expect(diff(
+      { someStyle: [{ foo: 1 }, { bar: 2 }]},
+      {},
+      validStyleAttribute
+    )).toEqual({ foo: null, bar: null });
+
+    var barStyle = StyleSheetRegistry.registerStyle({
+      bar: 3,
+    });
+
+    expect(diff(
+      {},
+      { someStyle: [[{ foo: 1 }, { foo: 2 }], barStyle]},
+      validStyleAttribute
+    )).toEqual({ foo: 2, bar: 3 });
+  });
+
+  it('should reset a value to a previous if it is removed', () => {
+    var validStyleAttribute = { someStyle: { foo: true, bar: true } };
+
+    expect(diff(
+      { someStyle: [{ foo: 1 }, { foo: 3 }]},
+      { someStyle: [{ foo: 1 }, { bar: 2 }]},
+      validStyleAttribute
+    )).toEqual({ foo: 1, bar: 2 });
+  });
+
+  it('should not clear removed props if they are still in another slot', () => {
+    var validStyleAttribute = { someStyle: { foo: true, bar: true } };
+
+    expect(diff(
+      { someStyle: [{}, { foo: 3, bar: 2 }]},
+      { someStyle: [{ foo: 3 }, { bar: 2 }]},
+      validStyleAttribute
+    )).toEqual(null);
+
+    expect(diff(
+      { someStyle: [{}, { foo: 3, bar: 2 }]},
+      { someStyle: [{ foo: 1, bar: 1 }, { bar: 2 }]},
+      validStyleAttribute
+    )).toEqual({ foo: 1 });
+  });
+
+  it('should clear a prop if a later style is explicit null/undefined', () => {
+    var validStyleAttribute = { someStyle: { foo: true, bar: true } };
+    expect(diff(
+      { someStyle: [{}, { foo: 3, bar: 2 }]},
+      { someStyle: [{ foo: 1 }, { bar: 2, foo: null }]},
+      validStyleAttribute
+    )).toEqual({ foo: null });
+
+    expect(diff(
+      { someStyle: [{ foo: 3 }, { foo: null, bar: 2 }]},
+      { someStyle: [{ foo: null }, { bar: 2 }]},
+      validStyleAttribute
+    )).toEqual(null);
+
+    expect(diff(
+      { someStyle: [{ foo: 1 }, { foo: null }]},
+      { someStyle: [{ foo: 2 }, { foo: null }]},
+      validStyleAttribute
+    )).toEqual(null);
+
+    // Test the same case with object equality because an early bailout doesn't
+    // work in this case.
+    var fooObj = { foo: 3 };
+    expect(diff(
+      { someStyle: [{ foo: 1 }, fooObj]},
+      { someStyle: [{ foo: 2 }, fooObj]},
+      validStyleAttribute
+    )).toEqual(null);
+
+    expect(diff(
+      { someStyle: [{ foo: 1 }, { foo: 3 }]},
+      { someStyle: [{ foo: 2 }, { foo: undefined }]},
+      validStyleAttribute
+    )).toEqual({ foo: null });
+  });
+
   // Function properties are just markers to native that events should be sent.
   it('should convert functions to booleans', () => {
     // Note that if the property changes from one function to another, we don't
diff --git a/Libraries/StyleSheet/TransformPropTypes.js b/Libraries/StyleSheet/TransformPropTypes.js
@@ -13,6 +13,22 @@
 
 var ReactPropTypes = require('ReactPropTypes');
 
+var ArrayOfNumberPropType = ReactPropTypes.arrayOf(ReactPropTypes.number);
+
+var TransformMatrixPropType = function(
+  props : Object,
+  propName : string,
+  componentName : string
+) : ?Error {
+  if (props.transform && props.transformMatrix) {
+    return new Error(
+      'transformMatrix and transform styles cannot be used on the same ' +
+      'component'
+    );
+  }
+  return ArrayOfNumberPropType(props, propName, componentName);
+};
+
 var TransformPropTypes = {
   transform: ReactPropTypes.arrayOf(
     ReactPropTypes.oneOfType([
@@ -30,7 +46,7 @@ var TransformPropTypes = {
       ReactPropTypes.shape({skewY: ReactPropTypes.string})
     ])
   ),
-  transformMatrix: ReactPropTypes.arrayOf(ReactPropTypes.number),
+  transformMatrix: TransformMatrixPropType,
 };
 
 module.exports = TransformPropTypes;
diff --git a/Libraries/StyleSheet/processTransform.js b/Libraries/StyleSheet/processTransform.js
@@ -6,74 +6,17 @@
  * LICENSE file in the root directory of this source tree. An additional grant
  * of patent rights can be found in the PATENTS file in the same directory.
  *
- * @providesModule precomputeStyle
+ * @providesModule processTransform
  * @flow
  */
 'use strict';
 
 var MatrixMath = require('MatrixMath');
-var ReactNativeStyleAttributes = require('ReactNativeStyleAttributes');
 var Platform = require('Platform');
 
-var deepFreezeAndThrowOnMutationInDev = require('deepFreezeAndThrowOnMutationInDev');
 var invariant = require('invariant');
 var stringifySafe = require('stringifySafe');
 
-/**
- * This method provides a hook where flattened styles may be precomputed or
- * otherwise prepared to become better input data for native code.
- */
-function precomputeStyle(style: ?Object, validAttributes: Object): ?Object {
-  if (!style) {
-    return style;
-  }
-
-  var hasPreprocessKeys = false;
-  for (var i = 0, keys = Object.keys(style); i < keys.length; i++) {
-    var key = keys[i];
-    if (_processor(key, validAttributes)) {
-      hasPreprocessKeys = true;
-      break;
-    }
-  }
-
-  if (!hasPreprocessKeys && !style.transform) {
-    return style;
-  }
-
-  var newStyle = {...style};
-  for (var i = 0, keys = Object.keys(style); i < keys.length; i++) {
-    var key = keys[i];
-    var process = _processor(key, validAttributes);
-    if (process) {
-      newStyle[key] = process(newStyle[key]);
-    }
-  }
-
-  if (style.transform) {
-    invariant(
-      !style.transformMatrix,
-      'transformMatrix and transform styles cannot be used on the same component'
-    );
-
-    newStyle = _precomputeTransforms(newStyle);
-  }
-
-  deepFreezeAndThrowOnMutationInDev(newStyle);
-  return newStyle;
-}
-
-function _processor(key: string, validAttributes: Object) {
-  var process = validAttributes[key] && validAttributes[key].process;
-  if (!process) {
-    process =
-      ReactNativeStyleAttributes[key] &&
-      ReactNativeStyleAttributes[key].process;
-  }
-
-  return process;
-}
-
 /**
  * Generate a transform matrix based on the provided transforms, and use that
  * within the style object instead.
@@ -82,8 +25,7 @@ function _processor(key: string, validAttributes: Object) {
  * be applied in an arbitrary order, and yet have a universal, singular
  * interface to native code.
  */
-function _precomputeTransforms(style: Object): Object {
-  var {transform} = style;
+function processTransform(transform: Object): Object {
   var result = MatrixMath.createIdentityMatrix();
 
   transform.forEach(transformation => {
@@ -144,16 +86,9 @@ function _precomputeTransforms(style: Object): Object {
   // get applied in the specific order of (1) translate (2) scale (3) rotate.
   // Once we can directly apply a matrix, we can remove this decomposition.
   if (Platform.OS === 'android') {
-    return {
-      ...style,
-      transformMatrix: result,
-      decomposedMatrix: MatrixMath.decomposeMatrix(result),
-    };
+    return MatrixMath.decomposeMatrix(result);
   }
-  return {
-    ...style,
-    transformMatrix: result,
-  };
+  return result;
 }
 
 /**
@@ -240,4 +175,4 @@ function _validateTransform(key, value, transformation) {
   }
 }
 
-module.exports = precomputeStyle;
+module.exports = processTransform;
