From 6244fd003de4eb6ce7fc7c40632948bf27cca201 Mon Sep 17 00:00:00 2001 From: Sebastian Markbage Date: Tue, 6 Oct 2015 15:19:59 -0700 Subject: [PATCH] Refactor Attribute Processing (Step 3) Reviewed By: @spicyj Differential Revision: D2514193 fb-gh-sync-id: 8ba55014e8d052c69d8e1def327284ec974d6837 --- .../View/ReactNativeStyleAttributes.js | 2 + Libraries/ReactIOS/NativeMethodsMixin.js | 3 - .../ReactNativeAttributePayload.js | 47 +++++++--- .../ReactNativeAttributePayload-test.js | 94 ++++++++++++++++++- Libraries/StyleSheet/TransformPropTypes.js | 18 +++- ...precomputeStyle.js => processTransform.js} | 75 +-------------- 6 files changed, 148 insertions(+), 91 deletions(-) rename Libraries/StyleSheet/{precomputeStyle.js => processTransform.js} (74%) diff --git a/Libraries/Components/View/ReactNativeStyleAttributes.js b/Libraries/Components/View/ReactNativeStyleAttributes.js index e2099c9d2a..ffeff5420d 100644 --- 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 index 6d158338f9..4da11abc7a 100644 --- 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 index a0d328f28e..8ee8378c10 100644 --- 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 (!validAttributes[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 index 9951a9eb79..2f142c5739 100644 --- 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 index 2c797ae324..ecc44d4aa7 100644 --- 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/precomputeStyle.js b/Libraries/StyleSheet/processTransform.js similarity index 74% rename from Libraries/StyleSheet/precomputeStyle.js rename to Libraries/StyleSheet/processTransform.js index 97bf5a0c1e..4666a30552 100644 --- a/Libraries/StyleSheet/precomputeStyle.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;