Merge pull request #1601 from gaearon/apply-component-and-mixin-spec-deterministically

Apply component and mixins specs deterministically

Author: leebyron

docs/docs/05-reusable-components.md

@@ -190,5 +190,5 @@ React.renderComponent(
 );
 ```
 
-A nice feature of mixins is that if a component is using multiple mixins and several mixins define the same lifecycle method (i.e. several mixins want to do some cleanup when the component is destroyed), all of the lifecycle methods are guaranteed to be called.
+A nice feature of mixins is that if a component is using multiple mixins and several mixins define the same lifecycle method (i.e. several mixins want to do some cleanup when the component is destroyed), all of the lifecycle methods are guaranteed to be called. Methods defined on mixins run in the order mixins were listed, followed by a method call on the component.
 

src/core/ReactCompositeComponent.js

@@ -36,13 +36,16 @@ var ReactUpdates = require('ReactUpdates');
 var instantiateReactComponent = require('instantiateReactComponent');
 var invariant = require('invariant');
 var keyMirror = require('keyMirror');
+var keyOf = require('keyOf');
 var merge = require('merge');
 var mixInto = require('mixInto');
 var monitorCodeUse = require('monitorCodeUse');
 var mapObject = require('mapObject');
 var shouldUpdateReactComponent = require('shouldUpdateReactComponent');
 var warning = require('warning');
 
+var MIXINS_KEY = keyOf({mixins: null});
+
 /**
  * Policies that describe methods in `ReactCompositeComponentInterface`.
  */
@@ -475,12 +478,25 @@ function mixSpecIntoComponent(Constructor, spec) {
   );
 
   var proto = Constructor.prototype;
+
+  // By handling mixins before any other properties, we ensure the same
+  // chaining order is applied to methods with DEFINE_MANY policy, whether
+  // mixins are listed before or after these methods in the spec.
+  if (spec.hasOwnProperty(MIXINS_KEY)) {
+    RESERVED_SPEC_KEYS.mixins(ConvenienceConstructor, spec.mixins);
+  }
+
   for (var name in spec) {
-    var property = spec[name];
     if (!spec.hasOwnProperty(name)) {
       continue;
     }
 
+    if (name === MIXINS_KEY) {
+      // We have already handled mixins in a special case above
+      continue;
+    }
+
+    var property = spec[name];
     validateMethodOverride(proto, name);
 
     if (RESERVED_SPEC_KEYS.hasOwnProperty(name)) {

src/core/__tests__/ReactCompositeComponentMixin-test.js

@@ -27,6 +27,7 @@ var reactComponentExpect;
 
 var TestComponent;
 var TestComponentWithPropTypes;
+var TestComponentWithReverseSpec;
 var mixinPropValidator;
 var componentPropValidator;
 
@@ -58,6 +59,13 @@ describe('ReactCompositeComponent-mixin', function() {
       }
     };
 
+    var MixinBWithReverseSpec = {
+      componentDidMount: function() {
+        this.props.listener('MixinBWithReverseSpec didMount');
+      },
+      mixins: [MixinA]
+    };
+
     var MixinC = {
       statics: {
         staticC: function() {}
@@ -89,6 +97,16 @@ describe('ReactCompositeComponent-mixin', function() {
       }
     });
 
+    TestComponentWithReverseSpec = React.createClass({
+      render: function() {
+        return <div />;
+      },
+      componentDidMount: function() {
+        this.props.listener('Component didMount');
+      },
+      mixins: [MixinBWithReverseSpec, MixinC, MixinD]
+    });
+
     TestComponentWithPropTypes = React.createClass({
       mixins: [MixinD],
       propTypes: {
@@ -128,6 +146,19 @@ describe('ReactCompositeComponent-mixin', function() {
     ]);
   });
 
+  it('should chain functions regardless of spec property order', function() {
+    var listener = mocks.getMockFunction();
+    var instance = <TestComponentWithReverseSpec listener={listener} />;
+    instance = ReactTestUtils.renderIntoDocument(instance);
+
+    expect(listener.mock.calls).toEqual([
+      ['MixinA didMount'],
+      ['MixinBWithReverseSpec didMount'],
+      ['MixinC didMount'],
+      ['Component didMount']
+    ]);
+  });
+
   it('should validate prop types via mixins', function() {
     expect(TestComponent.type.propTypes).toBeDefined();
     expect(TestComponent.type.propTypes.value)