[Proposal] Leverage Inheritance to Address Cross-Cutting Concerns

[alitaheri]

Recently there has been much discussion of how to get rid of mixins and getting theme from context. all have many short comings:

• Stateless Functional Wrapper Component
  1. no ref
  2. methods cannot be forwarded at all!
• HOC
  1. Methods cannot be forwarded conveniently
  2. breaks instanceof
• HOC with proxy
  1. Very hard to maintain!
• Mixin
  1. The people at React make it very clear why they won't support mixins for es6-classes
  2. Very hard to reason about
• Utility Methods
  1. Cannot access the component's internal behavior

Solution:

Inheritance to rescue! This handled a lot of things! Almost all cross-cutting concerns can be handled this way:

1. Themes can be easily added to the state.
2. Changes to theme can be easily detected.
3. Many optimizations can be applied with ease.
4. Most importantly: elements can be checked with: a.type === Avatar instead of displayName see here! 🎉 🎉
5. Future concerns can be addressed here.
6. communication between the superclass and subclass can be very easily established as opposed to HOCs, the benefits:
   • Memoization of calculated styles can be easily implemented in the super class.
   • Similar components can have a common super class to implement some behaviors

@oliviertassinari @ntgn81 @newoga I want to seriously discuss this before we decide what to do.

[oliviertassinari]

Will we have a conflict with https://github.com/callemall/material-ui/blob/master/src/snackbar.jsx#L148?

[alitaheri]

a call to super.componentWillReceiveProps will fix this 😁

[ntgn81]

This definitely sounds much much better than HOC. The extra wrapping on top of every component makes debugging unwieldy quickly. And most likely some performance implications, which might be negligible.

I think we don't need to save muiTheme in state, rather just provide getMuiTheme() method on the base class that reads directly from this.context.muiTheme, falling back to default if needed. That way we don't need to implement componentWillReceiveProps, forcing inherited components to call super.

I have a codepen with this concept, it seems to work for me, I'm not sure if it has any side effects though.

http://codepen.io/ntgn81/pen/admovZ

Something like this:

const DEFAULT_THEME = { name: 'default' }

class AbstractMuiComponent extends React.Component {
  static contextTypes = {
    muiTheme: React.PropTypes.object,
  };
  getMuiTheme = () => this.context.muiTheme || DEFAULT_THEME;
}

class ActualComponent extends AbstractMuiComponent {
  render () {
    const muiTheme = this.getMuiTheme();
    return <div>Current theme: <h1>{muiTheme.name}</h1></div>
  }
}

[newoga]

Disclaimer: I'm a composition over inheritance guy. 😝 Though a year ago I would have been in complete agreement with this!

My gut feeling is against this approach but I'm going into this with an open mind. I think we should look carefully and see if anywhere else in the React community developers are extending React.Component or if this is supported or recommended at all by React (and how this could impact React related tooling). I do agree that this also does solve the "sharing theme" problem, but I'm afraid if could end us limiting how we have to design our components.

Out of curiosity, is the primary motivator behind this to support imperative methods on Component classes or is there anything else? I'd also argue that if the imperative methods were the only blocker, we should just put a hold on this, and move towards deprecating and removing them.

[oliviertassinari]

I agree with @newoga. The inheritance can have some valid use case, but the fonctionnal composability is better IMHO.
For instance, I don't really like depending on a magic this.getMuiTheme();.

The solution implemented in this PR #2585 was almost fine.

[ntgn81]

  Noob alert. Just started with React over a month, don't take what I say too seriously 😅

Composition is quite cool for when we need to compose things, like creating an application or a page. This is a component library though, I'm not sure if composition's best practices fully apply here.

I think performance is probably more important (stating the obvious? 😆), especially considering the components can be included many many times on a complex page.

Of course code clarity/maintainability are also important, which I don't think inheritance would hinder too much.

I just did a quick perf measerement using React.tools.Perf, comparing rendering of BaseComponent, HocComponent and InheritedComponent, 5000 times each.

Not quite sure if I'm measuring it correctly. But here's the output. The HOC time is consistently twice as much as Base/Inherited. Not sure why but Inherited most of the time is faster than BaseComp by a bit.

Code @ CodePen

const limit = 5000;
const arr = Array.apply(null, Array(limit));
class App extends React.Component {
  render = () => (
    <div>
      { arr.map((v, i) => <BaseComp key={i} index={i} />) }
      { arr.map((v, i) => <InheritedComp key={i} index={i} />) }
      { arr.map((v, i) => <HocComp key={i} index={i} />) }
    </div>
  )
}

class BaseComp extends React.Component {
  render = () => <div>Component: {this.props.index}</div>
}
const HocComp = wrap(BaseComp);
class InheritedComp extends BaseComp {}

function wrap(Wrapped) {
  class HOCWrapper extends React.Component {
    render = () => <Wrapped {...this.props} />
  }
  return HOCWrapper;
}

React.addons.Perf.start();
React.render(<App />, document.getElementById('app'));
console.log('Perf.printInclusive()');
React.addons.Perf.printInclusive();

[alitaheri]

Guys guys, please take into account that this is on the es6-classes. I can simply merge this, migrate some components, and see what effects it will have overall. if we hate it all, we always just git branch -d it 😁

Out of curiosity, is the primary motivator behind this to support imperative methods on Component classes or is there anything else? I'd also argue that if the imperative methods were the only blocker, we should just put a hold on this, and move towards deprecating and removing them.

@newoga No it's not. They should be removed, granted 😁 But the concerns are bigger here. first of all. We have damn too many mixins that serve some really important functionality, some of them cannot be made into utility classes. like windowListenable. but implementing them in the super class is extremely easy and very coherent.

For instance, I don't really like depending on a magic this.getMuiTheme();.

@oliviertassinari how about this.theme? we can use getters with babel out of box 😁 You'll always be sure there is a theme with all the variables you need. And it's not magic. Mixins are magic. inheritance is very effective.

rather just provide getMuiTheme() method on the base class that reads directly from this.context.muiTheme, falling back to default if needed.

@ntgn81 I would agree with this (it's a great idea that I didn't think of). But if we are going to add extra functionality and address other concerns we will have to override those methods here anyway (with a breaking change if we don't enforce override). I think we should follow this pattern for every lifecycle method:

class MuiComponent extends React.Component {
  // ...
  componentWillReceiveProps(nextProps, nextContext) {
   // Optimazation, handling of some props, and other stuff. 
   if(this.componentWillReceivePropsOverride) { // Extra implementation hook.
      this.componentWillReceivePropsOverride(nextProps, nextContext);
    }
  }
  // ...
}

class SomeComponent extends MuiComponent {
  // ... 
  componentWillReceivePropsOverride(nextProps, nextContext) {
    // stuff... (no need to call super)
  }
}

This pattern is used extensively within Microsoft's WPF project. And it works well. Ugly, granted, Hard to understand for new comers, granted. Just a shot in the dark. 😬 😁

My own opinion:

Composition is great! But the costs are high for internals of libraries. They can make code easier to reason about than inheritance. But we are forgetting something very important. Productivity is far more important than conciseness, My proposal provides these values:

1. Performance (as benchmarked by @ntgn81, thanks a lot 🙏 🙏 )
2. Solves multiple cross-cutting-concerns (window-listenable, handling of theme, pure-render implementation,
3. VERY easily implementation for style memoization (devastating performance optimization)
4. Guaranteed safe type checking ( a.type === Avatar ) instead of ( a.displayName === 'Avatar' ) this can fail on so many grounds.
5. Guaranteed working ref and methods (guys we need methods, what are we gonna do about focus() then? There is no denying that focus is important and props can't conveniently handle focus forwarding it is hard to maintain and prone to regression.
6. We can document the behavior of that class so others can also benefit from it (for those who wish to add extensions (localized, enhanced,...) for their work that can leverage these solutions)

I think that's enough. Tell me how we can achieve all these with HOC, Mixin, Utility Function (library). I'm more of a productivity and added value guy, so inheritance, composition, utility or anything else are only tools in my mind. I leverage the one that provides the most value.

You can only convince me that this is not a good approach if these items can be conveniently and safely address with your proposed method.

Thank you all for your feedbacks, I love this community 🤘 🤘 🤘

[newoga]

I made some related comments on this here.

I'll reiterate this portion first:

I'm fine exploring the inheritance approach in the es6-classes branch like you suggested.

Below are some additional comments.

Note: In my comments below, I am playing devil's advocate and being hard on purpose so that we can challenge each other and better understand the pros, cons, and arguments for each side. I am not necessarily fully convinced about either approach, but hopefully through discussion we can learn and move forward.

Performance (as benchmarked by @ntgn81, thanks a lot 🙏 🙏 )

I'm hesitant to use @ntgn81's examples as our only indicator of performance. I'm far from an expert on React performance, and would consider myself barely knowledgeable, but at first glance, the numbers aren't surprising. If you have twice as many instances you'd expect it to take twice as long. But with more realistic examples (more complex components), we may find that the times aren't directly related to the number of instances and in fact the majority of time is spent in other areas that are easily optimizable in all approaches.

tl;dr The differences in performance illustrated in those examples may not be truly representative of typical performance problems found in real world React applications.

Solves multiple cross-cutting-concerns (window-listenable, handling of theme, pure-render implementation,

I'm really scared about this one. I really question whether all these concerns should be implemented in a single base component. There is where inheritance can get tricky and composability wins.

VERY easily implementation for style memoization (devastating performance optimization)

I think this is great. Though I don't fully understand in what ways it will be easier to implement this properly for all components compared to the composition approach.

Guaranteed safe type checking ( a.type === Avatar ) instead of ( a.displayName === 'Avatar' ) this can fail on so many grounds.

Do we really need this? Whenever we do check Component types, should we really be checking the type, or should really be implementing it differently. In my opinion, our components shouldn't have to modify it's behavior based on the type of components it's interacting with. We should investigate approaches where it doesn't matter.

Guaranteed working ref and methods (guys we need methods, what are we gonna do about focus() then? There is no denying that focus is important and props can't conveniently handle focus forwarding it is hard to maintain and prone to regression.

Good question, focus is hard. But I don't want to give up yet looking for solutions outside defining publicly accessible methods on component classes.

We can document the behavior of that class so others can also benefit from it (for those who wish to add extensions (localized, enhanced,...) for their work that can leverage these solutions)

I think we should encourage people to build React components and not MuiComponents. If we make a publicly available MuiComponent that doesn't behave exactly the way a React component works, we can risk fragmenting the community. For example, we might be invite a world of trouble for people that get unexpected or unintended behavior when using MuiComponent components with other frameworks and libraries that are designed to work with plain old functions or classes that extend directly from React.Component.

Another note on this: Context is especially tricky. I'm nervous to make our base components use context out of the box.

Thank you all for your feedbacks, I love this community 🤘 🤘 🤘

Thank you as well! And me too! 💃

[alitaheri]

In my comments below, I am playing devil's advocate and being hard on purpose so that

I appreciate you doing this. The better we understand this, the better the library will turn out to be 👍 👍 👍

The differences in performance illustrated in those examples may not be truly representative of typical performance problems found in real work React applications.

I understand! Performance is more By Design than By Implementation. I totally agree. I shouldn't make assumptions, my bad 😅 I think we should have some production project that uses react switch to inheritance and see the outcome in real world. 👍

I'm really scared about this one. I really question whether all these concerns should be implemented in a single base component. There is where inheritance can get tricky and composability wins.

Well not all 😁 Only the ones that are very tightly coupled like windowListenable, handling of context, pure render and memoization.

Do we really need this? Whenever we do check Component types, should we really be checking the type, or should really be implementing it differently.

Some times we have no choice. Like in Menu, it needs to get a lot of info from it's nested items. The other way would be communication through context, but that will make it way too complex, and needs spec! in the long run using context is far better. but still debatable 😄

Good question, focus is hard. But I don't want to give up yet looking for solutions outside defining method on components.

I HAVE come up with solutions for this, but they all involve teaching people what the hell is going on with that super-duper special freak prop O.O

I think we should encourage people to build React components and not MuiComponents.

Yup after writing this, I came to disagreement with myself 😅 😅

[alitaheri]

I forgot about memoization.

With only one instance object we can memoize everything that need it. with an api like this:

The reason I don't like this to be utility function is easier memory management.
Although that is very debatable and I haven't given it much thought.

class MyMemoizableHeavilyStyledComponent extends MuiComponent {

 // This will avoid reallocation when only prop change!
 _menuStyles = this.memoize(color1, color2, otherDependentStuff) => ({
   // calculate styles for menu.
  });

  // This will avoid hundreds of object allocations per render!!!
  getStyles = this.memoize(theme, someprop1, someprop2) => ({
    menu: _menuStyles(theme.color1, /*...*/);
  });

}

[ntgn81]

Thanks for this great discussion guys 🙇 🙇 @subjectix @newoga

I am getting so much insights and learning so much from this.

I don't want to be a pain, but I am kinda hung up on performance. Especially with how popular this library is, I definitely don't want to see it being one of the cause for slowness.

So I went back and tried to make the sample performance code a bit more complex. Each component would loop through items, and render them.

The inclusive time definitely jumped up (~900ms to render 100 components vs ~350 to render 5000).

I thought if the actual component is more complex, while the wrapper/abstractClass logic remains simple, the difference between HOC and Inheritance should be smaller, but it's still twice as much.

Double the time, with many complex elements on the page can add up quite quickly. A table with 100 rows with buttons/select/textfield on each row, not very uncommon request I think 😄

[alitaheri]

@ntgn81 Thanks a whole bunch for providing another benchmark for this 👍 👍

@newoga @oliviertassinari Performance got debatable again 😎

[newoga]

@ntgn81 It's fair for you to bring this up and you're def not being a pain!

My only point in my earlier comment was that these performance tests indicate that the total inclusive time is directly related to the number of instances. So the rendering performance per instance is roughly the same for both approaches.

What we really need to investigate is: is the number of instances in a given state tree the typical cause for performance problems in a React application, or is it detecting changes and knowing when and when not to re-render components in a subtree (or is it something else entirely).

I'm assuming these tests are only testing performance on initial mount and render.

Once again, thanks for bringing this up @ntgn81. I don't think we should neglect performance, but I also believe we shouldn't prematurely optimize without understanding what the real problem is.

@ntgn81 Do you have time to make a simple project on git with some of your examples so we can play around with it and investigate more? It might not hurt to engage the larger React community as well and a project would be a good place to start.

[ntgn81]

Thanks for the encouragement :D Yep, that test was just for the initial rendering. I can definitely try to put together something on git.

[newoga]

@ntgn81 Thanks a lot a lot a lot! 👍 It's super helpful and I'm learning a lot.

[oliviertassinari]

@ntgn81 Regarding the performance implication of using an HOC, I wouldn't worry to much. Theoricaly, that seems cheap in memory and in CPU.

To put this in perspective, look at what Radium is doing in his HOC like approach: https://github.com/FormidableLabs/radium/blob/master/src/resolve-styles.js. They are travelling the all virtual tree.
We were thinking about using Radium or react-look to improve our style approach. I'm not longer sure it's a good idea now.

[alitaheri]

I am going to close this. Although I'm using inheritance in my application and its really good. But, however I think about it, it just complicates the code. for community libraries like this over engineering is bad news. So, we'll go with composition, Recompose will probably help. but we have work to do before we can move on.