New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Proposal for porting React's Mixin APIs to a generic primitive #1380

Closed
sebmarkbage opened this Issue Apr 9, 2014 · 48 comments

Comments

Projects
None yet
@sebmarkbage
Member

sebmarkbage commented Apr 9, 2014

Currently React's mixins allow multiple mixins to implement the same method in multiple mixin, if it's a whitelist. We would like to decouple the mixin system from React and therefore we need a way to solve it without a whitelist.

The idea is to have every mixin call super() to allow predictable chaining.

The mixin(...mixins) function would essentially create a new prototype chain with each mixin's set of functions stacked on top of each other in order. An object is treated as a prototype. A function is treated as a class/constructor and gets all their static methods + their prototype merged.

var A = {

  componentDidMount() {
    super(); // This will end up calling an empty function, placed by mixin()
    console.log('A');
  }

};

class B {

  static getQueries() {
    super(); // This will end up calling an empty function, placed by mixin()
    console.log('B')
  }

  componentDidMount() {
    console.log('B');
    super();
  }

}

class C extends mixin(A, B) {

  static getQueries() {
    super();
    console.log('C');
  }

  componentDidMount() {
    super();
    console.log('C');
  }

}

C.getQueries(); // B, C
new C().componentDidMount(); // B, A, C

We can issue warnings when the mixin function is called and some of the overlapping methods are missing super calls.

Solvable but confusing/complex issues:

class C extends mixin(A, B) {

  // This state intializer overrides the state initializer in the base class.
  // The current React class system merges the two. This is also not valid ES6
  // since we don't have property initializers yet. This is based on the
  // TypeScript syntax.
  state = {
    b: true
  }

  componentDidMount() {
    // You forgot to put a super call here but there's no warning since
    // the mixin logic happens before this class is created.
  }

}

To be clear, mixins is an escape hatch to work around reusability limitations in the system. It's not idiomatic React.

Idiomatic React reusable code should primarily be implemented in terms of composition and not inheritance.

@basarat

This comment has been minimized.

Show comment
Hide comment
@basarat

basarat Apr 10, 2014

Contributor

Just pointing out that mixin(A, B) will not compile in TypeScript as it is.

class C extends mixin(A, B) { // TS: Error

This will be useful for ES6 classes. But will not make it easy for people using TypeScript.

Contributor

basarat commented Apr 10, 2014

Just pointing out that mixin(A, B) will not compile in TypeScript as it is.

class C extends mixin(A, B) { // TS: Error

This will be useful for ES6 classes. But will not make it easy for people using TypeScript.

@sebmarkbage

This comment has been minimized.

Show comment
Hide comment
@sebmarkbage

sebmarkbage Apr 10, 2014

Member

That's a good point. I don't think there is a way to create composable interfaces in this way in TypeScript. Do you see any alternatives?

Member

sebmarkbage commented Apr 10, 2014

That's a good point. I don't think there is a way to create composable interfaces in this way in TypeScript. Do you see any alternatives?

@fdecampredon

This comment has been minimized.

Show comment
Hide comment
@fdecampredon

fdecampredon Apr 10, 2014

In typescript the only way I see to implements something similar to what you propose would be something like that :

declare function applyMixins(target: any, mixins: any []) : void;
declare function superMixin<T>(t:T): T;

class A {
   componentDidMount() {  }
   myMethodA() { return 'A' }
}

class B {
   componentDidMount() {  }
   myMethodB() { return 'B' }
}



class C implements A,B {
    componentDidMount(): void {
       superMixin(this).componentDidMount()
       console.log('C');
    }
    myMethodA: () => string;
    myMethodB: () => string;
}

applyMixins(C, [A, B]);

I already tried to wrap react logic with es6 class for typescript usage and obtained the following result : https://github.com/fdecampredon/react-typescript with similar way of doing that.

By the way mixin are in the roadmap of typescript for 1.x, but I have no clue when they plan to implement that. (I guess not soon)

fdecampredon commented Apr 10, 2014

In typescript the only way I see to implements something similar to what you propose would be something like that :

declare function applyMixins(target: any, mixins: any []) : void;
declare function superMixin<T>(t:T): T;

class A {
   componentDidMount() {  }
   myMethodA() { return 'A' }
}

class B {
   componentDidMount() {  }
   myMethodB() { return 'B' }
}



class C implements A,B {
    componentDidMount(): void {
       superMixin(this).componentDidMount()
       console.log('C');
    }
    myMethodA: () => string;
    myMethodB: () => string;
}

applyMixins(C, [A, B]);

I already tried to wrap react logic with es6 class for typescript usage and obtained the following result : https://github.com/fdecampredon/react-typescript with similar way of doing that.

By the way mixin are in the roadmap of typescript for 1.x, but I have no clue when they plan to implement that. (I guess not soon)

@basarat

This comment has been minimized.

Show comment
Hide comment
@basarat

basarat Apr 10, 2014

Contributor

I don't see a better way than what @fdecampredon is recommending. Notice his superMixin because super is a contextual keyword and would have given a TS error as well if used in a class.

Contributor

basarat commented Apr 10, 2014

I don't see a better way than what @fdecampredon is recommending. Notice his superMixin because super is a contextual keyword and would have given a TS error as well if used in a class.

@sebmarkbage

This comment has been minimized.

Show comment
Hide comment
@sebmarkbage

sebmarkbage Apr 10, 2014

Member

Luckily mixins are completely optional and you can use whatever system you want. Presumably TypeScript would have to build in similar features and you can just use that.

They're even frowned upon when they're overused so they're completely optional.

handwave, punt

Member

sebmarkbage commented Apr 10, 2014

Luckily mixins are completely optional and you can use whatever system you want. Presumably TypeScript would have to build in similar features and you can just use that.

They're even frowned upon when they're overused so they're completely optional.

handwave, punt

@pspeter3

This comment has been minimized.

Show comment
Hide comment
@pspeter3

pspeter3 Apr 25, 2014

So is this on the road map for React soon then?

pspeter3 commented Apr 25, 2014

So is this on the road map for React soon then?

@ctaggart

This comment has been minimized.

Show comment
Hide comment
@ctaggart

ctaggart Jun 4, 2014

@pspeter3, my understanding is that it is targeted for version 0.12 as part of Components as ES6 Classes. To guestimate the timeframe, take a look at the tags. Really looking forward to this! We want to create React components in code using TypeScript.

ctaggart commented Jun 4, 2014

@pspeter3, my understanding is that it is targeted for version 0.12 as part of Components as ES6 Classes. To guestimate the timeframe, take a look at the tags. Really looking forward to this! We want to create React components in code using TypeScript.

@pspeter3

This comment has been minimized.

Show comment
Hide comment
@pspeter3

pspeter3 Jul 11, 2014

@ctaggart I'm in the same place. Is there anyway to help get this out faster? (Eg, which issues would need to be resolved so I can create Pull Requests for them)

pspeter3 commented Jul 11, 2014

@ctaggart I'm in the same place. Is there anyway to help get this out faster? (Eg, which issues would need to be resolved so I can create Pull Requests for them)

@jquense

This comment has been minimized.

Show comment
Hide comment
@jquense

jquense Jul 20, 2014

Collaborator

Is there a possibility for something less inheritence based in this? Seems like doing something a la Traits would be a more composition-ally focused approach.

My worry is that this makes for a Ruby-esque problem of clever module ordering in order to get the right combination of super calls. It also means that you need to know that you are overriding a method in your mixin which makes mixins less reusable. An alternative would be to just require the class definer to resolve conflicts for non lifestyle methods/props and provide some utility functions for making that really easy? Admittedly I don't know the limitations of the es6 class syntax but in the current one

var Comp = React.createClass({

    mixins: [ A, B ],

    statics: {
       getQueries: util.chain(function(){ //or merge or after, before, around, etc 
           console.log('C')
       })
    },
    componentDidMount: function() {
        console.log('C');
    }
})
Collaborator

jquense commented Jul 20, 2014

Is there a possibility for something less inheritence based in this? Seems like doing something a la Traits would be a more composition-ally focused approach.

My worry is that this makes for a Ruby-esque problem of clever module ordering in order to get the right combination of super calls. It also means that you need to know that you are overriding a method in your mixin which makes mixins less reusable. An alternative would be to just require the class definer to resolve conflicts for non lifestyle methods/props and provide some utility functions for making that really easy? Admittedly I don't know the limitations of the es6 class syntax but in the current one

var Comp = React.createClass({

    mixins: [ A, B ],

    statics: {
       getQueries: util.chain(function(){ //or merge or after, before, around, etc 
           console.log('C')
       })
    },
    componentDidMount: function() {
        console.log('C');
    }
})
@syranide

This comment has been minimized.

Show comment
Hide comment
@syranide

syranide Jul 20, 2014

Contributor

@theporchrat The code in OP is based on ECMAScript proposals (although changing from day to day it seems).

Contributor

syranide commented Jul 20, 2014

@theporchrat The code in OP is based on ECMAScript proposals (although changing from day to day it seems).

@jquense

This comment has been minimized.

Show comment
Hide comment
@jquense

jquense Jul 20, 2014

Collaborator

@syranide the class syntax is from es6 proposals, but a mixin() method is all React. Until the language has some solid approach to mixins/modules/traits/whatever React will be providing some mechanism for flattening a bunch of object literals/prototypes and adding them to the a derived component prototype, no?

Sure one can mixin however they want, since the component is just a constructor and prototype, which is great, except that it destroys the interoperability of mixins.

It is definitely possible to have a mixin system that is not es6, but works with es6 (which is what I took goal here to be). All that is needed for the class syntax is a mixin function that returns a func or object

It just strikes me that the above mixin method doesn't seem any more in keeping with the language as it is progressing then the current methods, while also being very un-reactlike. Mixins aren't an inheritence based concept, it seems odd to try and push them that way.

Collaborator

jquense commented Jul 20, 2014

@syranide the class syntax is from es6 proposals, but a mixin() method is all React. Until the language has some solid approach to mixins/modules/traits/whatever React will be providing some mechanism for flattening a bunch of object literals/prototypes and adding them to the a derived component prototype, no?

Sure one can mixin however they want, since the component is just a constructor and prototype, which is great, except that it destroys the interoperability of mixins.

It is definitely possible to have a mixin system that is not es6, but works with es6 (which is what I took goal here to be). All that is needed for the class syntax is a mixin function that returns a func or object

It just strikes me that the above mixin method doesn't seem any more in keeping with the language as it is progressing then the current methods, while also being very un-reactlike. Mixins aren't an inheritence based concept, it seems odd to try and push them that way.

@syranide

This comment has been minimized.

Show comment
Hide comment
@syranide

syranide Jul 20, 2014

Contributor

@theporchrat The mixin() showed by sebmarkbage would probably not be exposed by React and if it was, it would be very simple and not in any way tied to React core. But it's likely that React won't go the way of classes, https://github.com/reactjs/react-future/blob/master/06%20-%20Returning%20State/02%20-%20Module%20Pattern.js

Contributor

syranide commented Jul 20, 2014

@theporchrat The mixin() showed by sebmarkbage would probably not be exposed by React and if it was, it would be very simple and not in any way tied to React core. But it's likely that React won't go the way of classes, https://github.com/reactjs/react-future/blob/master/06%20-%20Returning%20State/02%20-%20Module%20Pattern.js

@jquense

This comment has been minimized.

Show comment
Hide comment
@jquense

jquense Jul 20, 2014

Collaborator

ooo that is interesting thanks for the link

Collaborator

jquense commented Jul 20, 2014

ooo that is interesting thanks for the link

@sebmarkbage

This comment has been minimized.

Show comment
Hide comment
@sebmarkbage

sebmarkbage Jul 21, 2014

Member

React Core itself is not opinionated about how you build your classes. Generally we prefer composition. This is already possible using components.

This proposal is about providing feature parity with current React classes. It's not an encouraged pattern.

Native trait features in the ECMAScript language will likely have similar feature sets.

On Jul 20, 2014, at 2:11 PM, theporchrat notifications@github.com wrote:

ooo that is interesting thanks for the link


Reply to this email directly or view it on GitHub.

Member

sebmarkbage commented Jul 21, 2014

React Core itself is not opinionated about how you build your classes. Generally we prefer composition. This is already possible using components.

This proposal is about providing feature parity with current React classes. It's not an encouraged pattern.

Native trait features in the ECMAScript language will likely have similar feature sets.

On Jul 20, 2014, at 2:11 PM, theporchrat notifications@github.com wrote:

ooo that is interesting thanks for the link


Reply to this email directly or view it on GitHub.

@pspeter3

This comment has been minimized.

Show comment
Hide comment
@pspeter3

pspeter3 Oct 22, 2014

So for third party languages, http://facebook.github.io/react/blog/2014/10/14/introducing-react-elements.html#third-party-languages, will the class in type be the ES6 class or the result of React.createClass(EsClassHere)

pspeter3 commented Oct 22, 2014

So for third party languages, http://facebook.github.io/react/blog/2014/10/14/introducing-react-elements.html#third-party-languages, will the class in type be the ES6 class or the result of React.createClass(EsClassHere)

@sophiebits

This comment has been minimized.

Show comment
Hide comment
@sophiebits

sophiebits Oct 22, 2014

Member

@pspeter3 0.12 doesn't support classes in this way yet, but once we support ES6 classes then type will be the class, not the factory.

Member

sophiebits commented Oct 22, 2014

@pspeter3 0.12 doesn't support classes in this way yet, but once we support ES6 classes then type will be the class, not the factory.

@pspeter3

This comment has been minimized.

Show comment
Hide comment
@pspeter3

pspeter3 Oct 22, 2014

@spicyj I understand that, I meant for 0.13. Will I need to pass the ES6 class into React.createClass?

pspeter3 commented Oct 22, 2014

@spicyj I understand that, I meant for 0.13. Will I need to pass the ES6 class into React.createClass?

@sebmarkbage

This comment has been minimized.

Show comment
Hide comment
@sebmarkbage

sebmarkbage Oct 22, 2014

Member

@pspeter3, no you will not need to do that. type will be the ES6 class.

Member

sebmarkbage commented Oct 22, 2014

@pspeter3, no you will not need to do that. type will be the ES6 class.

@pspeter3

This comment has been minimized.

Show comment
Hide comment
@pspeter3

pspeter3 Oct 22, 2014

@sebmarkbage Oh that's awesome!

pspeter3 commented Oct 22, 2014

@sebmarkbage Oh that's awesome!

@jbrantly

This comment has been minimized.

Show comment
Hide comment
@jbrantly

jbrantly Jan 5, 2015

Contributor

It seems that ES6 classes are being actively worked on and I presume targeted for v0.13 (can anyone confirm that?). If so, what is the current plan for ES6 classes and mixins in that release? I'm assuming React Core will not ship with any magic to somehow handle mixins with createElement like it does today with createClass, so any mixin functionality will need to be handled outside of React. Will something along the lines of the OP be shipped?

Contributor

jbrantly commented Jan 5, 2015

It seems that ES6 classes are being actively worked on and I presume targeted for v0.13 (can anyone confirm that?). If so, what is the current plan for ES6 classes and mixins in that release? I'm assuming React Core will not ship with any magic to somehow handle mixins with createElement like it does today with createClass, so any mixin functionality will need to be handled outside of React. Will something along the lines of the OP be shipped?

@sebmarkbage

This comment has been minimized.

Show comment
Hide comment
@sebmarkbage

sebmarkbage Jan 5, 2015

Member

Yes, they are scheduled for 0.13. In the 0.13 release, we will not ship this mixin helper. It requires toMethod() support which is unusual in current ES6 transpilers. You will need to roll your own or use createClass.

Member

sebmarkbage commented Jan 5, 2015

Yes, they are scheduled for 0.13. In the 0.13 release, we will not ship this mixin helper. It requires toMethod() support which is unusual in current ES6 transpilers. You will need to roll your own or use createClass.

@rtorr

This comment has been minimized.

Show comment
Hide comment
@rtorr

rtorr Jan 28, 2015

This proposal is about providing feature parity with current React classes. It's not an encouraged pattern.

@sebmarkbage is this still the case? React.createClass is still the preferred way of doing things in 0.13? I don't think http://facebook.github.io/react/blog/2015/01/27/react-v0.13.0-beta-1.html really illustrates that opinion, and maybe that is why there is a lot of confusion.

rtorr commented Jan 28, 2015

This proposal is about providing feature parity with current React classes. It's not an encouraged pattern.

@sebmarkbage is this still the case? React.createClass is still the preferred way of doing things in 0.13? I don't think http://facebook.github.io/react/blog/2015/01/27/react-v0.13.0-beta-1.html really illustrates that opinion, and maybe that is why there is a lot of confusion.

@jimfb

This comment has been minimized.

Show comment
Hide comment
@jimfb

jimfb Jan 29, 2015

Contributor

@rtorr We are moving towards native ES6 classes as the "modern" way of doing things. That said, many people (including a large portion of Facebook) will continue to use React.createClass for some time, so both options will be supported in the near term.

Contributor

jimfb commented Jan 29, 2015

@rtorr We are moving towards native ES6 classes as the "modern" way of doing things. That said, many people (including a large portion of Facebook) will continue to use React.createClass for some time, so both options will be supported in the near term.

@alexeygolev

This comment has been minimized.

Show comment
Hide comment
@alexeygolev

alexeygolev Feb 8, 2015

Was wondering if subclassing can solve it for now. A 'mixin' can extend React.Component and all the components we need to use this 'mixin' could just extend it. Seems to be working. Am I missing something? (besides the fact that the mixins already created are not compatible with this pattern)

alexeygolev commented Feb 8, 2015

Was wondering if subclassing can solve it for now. A 'mixin' can extend React.Component and all the components we need to use this 'mixin' could just extend it. Seems to be working. Am I missing something? (besides the fact that the mixins already created are not compatible with this pattern)

@brigand

This comment has been minimized.

Show comment
Hide comment
@brigand

brigand Feb 9, 2015

Contributor

toMethod is now officially not in ES6 (planned for ES7).

Contributor

brigand commented Feb 9, 2015

toMethod is now officially not in ES6 (planned for ES7).

@sebmarkbage

This comment has been minimized.

Show comment
Hide comment
@sebmarkbage

sebmarkbage Feb 11, 2015

Member

Even without toMethod it could be built like this: https://gist.github.com/sebmarkbage/fac0830dbb13ccbff596

However, I think that making composition easier is a higher priority than making arbitrary mixins work.

Member

sebmarkbage commented Feb 11, 2015

Even without toMethod it could be built like this: https://gist.github.com/sebmarkbage/fac0830dbb13ccbff596

However, I think that making composition easier is a higher priority than making arbitrary mixins work.

@gsklee

This comment has been minimized.

Show comment
Hide comment
@gsklee

gsklee Feb 24, 2015

Contributor

I've played with @sebmarkbage's example above a bit but don't really like the fact that super calls become mandatory in each augmented method. Also played around with @brigand's react-mixin which looks to be one of the more reasonable solutions for now, but I don't like the syntax.

What's the status quo of mixins/traits in ES7? Would it be better to fill the syntax using something like sweet.js? For example Scala has something like this: class A extends B with X with Y with Z.

Contributor

gsklee commented Feb 24, 2015

I've played with @sebmarkbage's example above a bit but don't really like the fact that super calls become mandatory in each augmented method. Also played around with @brigand's react-mixin which looks to be one of the more reasonable solutions for now, but I don't like the syntax.

What's the status quo of mixins/traits in ES7? Would it be better to fill the syntax using something like sweet.js? For example Scala has something like this: class A extends B with X with Y with Z.

@donaldpipowitch

This comment has been minimized.

Show comment
Hide comment
@donaldpipowitch

donaldpipowitch Feb 24, 2015

Scala syntax looks good o_O

donaldpipowitch commented Feb 24, 2015

Scala syntax looks good o_O

@gsklee

This comment has been minimized.

Show comment
Hide comment
@gsklee

gsklee Mar 11, 2015

Contributor

@sebmarkbage I think I've come up with a feasible solution:

traits.js

import traits from 'es6-traits';
import React from 'react/addons';

export const {on, using} = traits();

export const autobind = {
  [Symbol.toStringTag]: 'autobind',

  constructor() {
    Object.getOwnPropertyNames(this.constructor.prototype)
          .filter(x => x.startsWith('on'))
          .map(x => this[x] = this[x].bind(this));
  }
};

export const purerender = Object.assign(React.addons.PureRenderMixin, {
  [Symbol.toStringTag]: 'purerender'
});

some-component.js

import React from 'react';
import {on, using, autobind, purerender} from './traits';

export default class SomeComponent extends (on (React.Component), using (autobind, purerender)) {
  ...
}

The syntax is a bit longer than the proposed mixins(React.Component, X, Y, Z) but is more concise IMHO. I actually went through numerous API ideas before finally settling on this one.

The package is available at es6-traits and is based on @brigand's excellent smart-mixin. Since smart-mixin is offering a composition conflict resolution mechanism I'm calling this traits instead of mixins.

Contributor

gsklee commented Mar 11, 2015

@sebmarkbage I think I've come up with a feasible solution:

traits.js

import traits from 'es6-traits';
import React from 'react/addons';

export const {on, using} = traits();

export const autobind = {
  [Symbol.toStringTag]: 'autobind',

  constructor() {
    Object.getOwnPropertyNames(this.constructor.prototype)
          .filter(x => x.startsWith('on'))
          .map(x => this[x] = this[x].bind(this));
  }
};

export const purerender = Object.assign(React.addons.PureRenderMixin, {
  [Symbol.toStringTag]: 'purerender'
});

some-component.js

import React from 'react';
import {on, using, autobind, purerender} from './traits';

export default class SomeComponent extends (on (React.Component), using (autobind, purerender)) {
  ...
}

The syntax is a bit longer than the proposed mixins(React.Component, X, Y, Z) but is more concise IMHO. I actually went through numerous API ideas before finally settling on this one.

The package is available at es6-traits and is based on @brigand's excellent smart-mixin. Since smart-mixin is offering a composition conflict resolution mechanism I'm calling this traits instead of mixins.

@brigand

This comment has been minimized.

Show comment
Hide comment
@brigand

brigand Mar 11, 2015

Contributor

Those syntax hacks are impressive :-)

I like it, but looking over the source I think the second example needs to be this because traits() can't be shared by multiple classes.

import React from 'react';
import traits from 'es6-traits';
import {autobind, purerender} from './traits';
const {on, using} = traits();

export default class SomeComponent extends (on (React.Component), using (autobind, purerender)) {
  ...
}

I also like the use of constructor over componentWillMount in the traits. It feels much more pure.

Contributor

brigand commented Mar 11, 2015

Those syntax hacks are impressive :-)

I like it, but looking over the source I think the second example needs to be this because traits() can't be shared by multiple classes.

import React from 'react';
import traits from 'es6-traits';
import {autobind, purerender} from './traits';
const {on, using} = traits();

export default class SomeComponent extends (on (React.Component), using (autobind, purerender)) {
  ...
}

I also like the use of constructor over componentWillMount in the traits. It feels much more pure.

@gsklee

This comment has been minimized.

Show comment
Hide comment
@gsklee

gsklee Mar 11, 2015

Contributor

@brigand It can be shared, you can also specify different resolution rulesets for different classes like this:

export const {on, using} = traits({
  ruleset: {
    Cat: {
      sleep: 'ONCE',
      play: 'MANY'
    },
    Dog: {
      sleep: 'ONCE',
      play: 'ONCE'
    }
  }
});

I need to find some time to come up with a document and add browser support so it can be played with inside JSBin etc. but while we are at it, any suggestion to the API and how the lib should work are welcome.

Contributor

gsklee commented Mar 11, 2015

@brigand It can be shared, you can also specify different resolution rulesets for different classes like this:

export const {on, using} = traits({
  ruleset: {
    Cat: {
      sleep: 'ONCE',
      play: 'MANY'
    },
    Dog: {
      sleep: 'ONCE',
      play: 'ONCE'
    }
  }
});

I need to find some time to come up with a document and add browser support so it can be played with inside JSBin etc. but while we are at it, any suggestion to the API and how the lib should work are welcome.

@brigand

This comment has been minimized.

Show comment
Hide comment
@brigand

brigand Mar 11, 2015

Contributor

I followed up in gsklee/estraits#2.

Contributor

brigand commented Mar 11, 2015

I followed up in gsklee/estraits#2.

@mlrawlings

This comment has been minimized.

Show comment
Hide comment
@mlrawlings

mlrawlings Mar 11, 2015

It would seem to me that unless React is going to move to an event or hook based system for the lifecycle methods, those methods need to be handled specifically. You can't just mix them into the prototype, which means whatever the solution it's going to be React specific. I like something like the following where it is clear that I am extending a React.Component with PureRenderMixin mixed in.

// user's code
class MyComponent extends React.Component(PureRenderMixin) {
   //...
}

// ReactComponent.js
function ReactComponent(props, context) {
  if(!(this instanceof ReactComponent)) {
    if(arguments.length) return createMixedComponent(arguments)
    return ReactComponent
  }
  this.props = props;
  this.context = context;
}

mlrawlings commented Mar 11, 2015

It would seem to me that unless React is going to move to an event or hook based system for the lifecycle methods, those methods need to be handled specifically. You can't just mix them into the prototype, which means whatever the solution it's going to be React specific. I like something like the following where it is clear that I am extending a React.Component with PureRenderMixin mixed in.

// user's code
class MyComponent extends React.Component(PureRenderMixin) {
   //...
}

// ReactComponent.js
function ReactComponent(props, context) {
  if(!(this instanceof ReactComponent)) {
    if(arguments.length) return createMixedComponent(arguments)
    return ReactComponent
  }
  this.props = props;
  this.context = context;
}
@BerkeleyTrue

This comment has been minimized.

Show comment
Hide comment
@BerkeleyTrue

BerkeleyTrue commented Mar 11, 2015

@mlrawlings That looks great!

@jjoos

This comment has been minimized.

Show comment
Hide comment

jjoos commented Mar 23, 2015

@brigand

This comment has been minimized.

Show comment
Hide comment
@brigand

brigand Mar 23, 2015

Contributor

Here's an implementation of @mlrawlings's syntax http://jsbin.com/tofidoxucu/2/edit?js,console,output

It does look much cleaner. If I switch to es6 classes, I'll probably write it like that, and have a small transform to move it out of there. Early errors aren't worth giving up imo, but luckily we don't need to compromise :-)

Contributor

brigand commented Mar 23, 2015

Here's an implementation of @mlrawlings's syntax http://jsbin.com/tofidoxucu/2/edit?js,console,output

It does look much cleaner. If I switch to es6 classes, I'll probably write it like that, and have a small transform to move it out of there. Early errors aren't worth giving up imo, but luckily we don't need to compromise :-)

@gaearon

This comment has been minimized.

Show comment
Hide comment
@gaearon

gaearon Mar 23, 2015

Member

In my opinion higher order components and the new observe hook in 0.14 solve almost all use cases I had for mixins.

Member

gaearon commented Mar 23, 2015

In my opinion higher order components and the new observe hook in 0.14 solve almost all use cases I had for mixins.

@gsklee

This comment has been minimized.

Show comment
Hide comment
@gsklee

gsklee Mar 24, 2015

Contributor

@gaearon Looks promising, but a little bit too React-specific. @mlrawlings's example is the most ideal but it'd require manually wrapping all classes with a function; I wonder if there is any possible way to abstract that part out into a generic lib?

Contributor

gsklee commented Mar 24, 2015

@gaearon Looks promising, but a little bit too React-specific. @mlrawlings's example is the most ideal but it'd require manually wrapping all classes with a function; I wonder if there is any possible way to abstract that part out into a generic lib?

@chicoxyzzy

This comment has been minimized.

Show comment
Hide comment
@chicoxyzzy

chicoxyzzy Mar 24, 2015

Contributor

there will be TC39 proposal for annotations Microsoft/TypeScript#1557 (comment) can we use something like that to abstract mixins?

Contributor

chicoxyzzy commented Mar 24, 2015

there will be TC39 proposal for annotations Microsoft/TypeScript#1557 (comment) can we use something like that to abstract mixins?

@brigand

This comment has been minimized.

Show comment
Hide comment
@brigand

brigand Mar 24, 2015

Contributor

@chicoxyzzy it doesn't really add any value, other than being able to declare the mixins before the class in code. Annotations don't do anything, so you need something else to act on them.

With classes, either directly on the class via Class.prototype or on instances via this.constructor.annotate, Object.getPrototypeOf(this).constructor.annotate, etc.

// used for an instanceof check, because other annotations may be used
class ReactMixin {
    constructor(mixin){ this.mixin = mixin }
}

@ReactMixin(PureRenderMixin)
class Foo extends React.Component {
   ...
}
applyMixins(Foo);

function applyMixins(Class){
    if (Class.annotate) {
       Class.annotate.forEach(maybeMixin => {
           // apply the mixin behavior for each mixin annotation
           if (maybeMixin instanceof ReactMixin) 
               reactMixin(Class.prototype, maybeMixin.mixin);
       })
    }
}

As @gaearon pointed out, you can expect alternatives to mixins in the coming months... maybe before we get annotation support in babel.

Contributor

brigand commented Mar 24, 2015

@chicoxyzzy it doesn't really add any value, other than being able to declare the mixins before the class in code. Annotations don't do anything, so you need something else to act on them.

With classes, either directly on the class via Class.prototype or on instances via this.constructor.annotate, Object.getPrototypeOf(this).constructor.annotate, etc.

// used for an instanceof check, because other annotations may be used
class ReactMixin {
    constructor(mixin){ this.mixin = mixin }
}

@ReactMixin(PureRenderMixin)
class Foo extends React.Component {
   ...
}
applyMixins(Foo);

function applyMixins(Class){
    if (Class.annotate) {
       Class.annotate.forEach(maybeMixin => {
           // apply the mixin behavior for each mixin annotation
           if (maybeMixin instanceof ReactMixin) 
               reactMixin(Class.prototype, maybeMixin.mixin);
       })
    }
}

As @gaearon pointed out, you can expect alternatives to mixins in the coming months... maybe before we get annotation support in babel.

@donaldpipowitch

This comment has been minimized.

Show comment
Hide comment
@donaldpipowitch

donaldpipowitch Mar 24, 2015

I think Babel already has annotation support behind a flag.

donaldpipowitch commented Mar 24, 2015

I think Babel already has annotation support behind a flag.

@andreypopp

This comment has been minimized.

Show comment
Hide comment
@andreypopp

andreypopp Mar 24, 2015

Contributor

@brigand I think the idea is to add decorators, not annotations to ES7. Class decorators are code so it's possible to have withMixins decorator which works like:

@withMixins(PureRenderMixin)
class Foo extends React.Component {
   ...
}

I also think that decorator syntax will be useful for higher order components, so instead of:

class Foo extends React.Component {
  ...
}
Foo = Enhance(Foo)

we can write:

@Enhance
class Foo extends React.Component {
  ...
}
Contributor

andreypopp commented Mar 24, 2015

@brigand I think the idea is to add decorators, not annotations to ES7. Class decorators are code so it's possible to have withMixins decorator which works like:

@withMixins(PureRenderMixin)
class Foo extends React.Component {
   ...
}

I also think that decorator syntax will be useful for higher order components, so instead of:

class Foo extends React.Component {
  ...
}
Foo = Enhance(Foo)

we can write:

@Enhance
class Foo extends React.Component {
  ...
}
@rstuven

This comment has been minimized.

Show comment
Hide comment
@rstuven

rstuven Apr 8, 2015

@brigand @andreypopp Here's a description of decorators that includes API and desugaring: https://github.com/wycats/javascript-decorators

This is already implemented in Babel 5+ and TypeScript 1.5

rstuven commented Apr 8, 2015

@brigand @andreypopp Here's a description of decorators that includes API and desugaring: https://github.com/wycats/javascript-decorators

This is already implemented in Babel 5+ and TypeScript 1.5

wincent added a commit to wincent/corpus that referenced this issue May 29, 2015

Make a decorator equivalent to PureRenderMixin
This is not very sophisticated; it assumes the class does not define its
own `shouldComponentUpdate`.

For more discussion of the future for this kind of pattern, see:

  facebook/react#1380
@jedwards1211

This comment has been minimized.

Show comment
Hide comment
@jedwards1211

jedwards1211 Aug 11, 2015

Contributor

Will we ever have automatic autobinding again?

Contributor

jedwards1211 commented Aug 11, 2015

Will we ever have automatic autobinding again?

@sophiebits

This comment has been minimized.

Show comment
Hide comment
@sophiebits

sophiebits Aug 11, 2015

Member

@jedwards1211 Unlikely. We'll probably recommend using a ES-future syntax feature when it lands: https://facebook.github.io/react/blog/2015/01/27/react-v0.13.0-beta-1.html#autobinding.

Member

sophiebits commented Aug 11, 2015

@jedwards1211 Unlikely. We'll probably recommend using a ES-future syntax feature when it lands: https://facebook.github.io/react/blog/2015/01/27/react-v0.13.0-beta-1.html#autobinding.

@jedwards1211

This comment has been minimized.

Show comment
Hide comment
@jedwards1211

jedwards1211 Aug 11, 2015

Contributor

Hopefully the come up with something nice for that. More often than not, whenever I've passed functions around, I've wished they were autobound, even outside of React code.

I understand why React devs don't "think you're in the business of designing a class system," but when you're going to start using an entire view framework, it's not that much cognitive burden to learn its class system. Certainly less cognitive burden for me than keeping track of all this churn.

Contributor

jedwards1211 commented Aug 11, 2015

Hopefully the come up with something nice for that. More often than not, whenever I've passed functions around, I've wished they were autobound, even outside of React code.

I understand why React devs don't "think you're in the business of designing a class system," but when you're going to start using an entire view framework, it's not that much cognitive burden to learn its class system. Certainly less cognitive burden for me than keeping track of all this churn.

@gaearon

This comment has been minimized.

Show comment
Hide comment
@gaearon

gaearon Apr 21, 2017

Member

I guess this ship has sailed.

Member

gaearon commented Apr 21, 2017

I guess this ship has sailed.

@gaearon gaearon closed this Apr 21, 2017

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment