[Core] Remove default theme handling from components

[mbrookes]

After some investigation prompted by a question from @nathanmarks about themes, it seems there is a huge amount of boilerplate associated with ensuring a theme is available when none has been defined already on context. (#1662).

Having every component check for the existence of muiTheme on context, and adding it if not found, results in significant added complexity to the code. This reduces code clarity and maintainability, and in some cases introduced state solely for the purpose of moving muiTheme through the pipeline.

By removing this code we significantly simplify the components. In addition several components are now stateless as a result.

A developer using MUI will need to pass a theme on context once in their app, (for example with MuIThemeProvider), so the tradeoff is 2 lines of code for the user, vs nearly 2000 in MUI.

TL;DR: This PR removes the code from components that adds a theme if not already on context.

[nathanmarks]

I think this is a good idea... esp considering the idea of context to begin with. There is a crazy amount of boilerplate involved to support an edge case. Not only that, but we're currently forcing internal state (and soon a constructor method) for the sole purpose of putting the theme from context into state as well.

Having users use MuiThemeProvider somewhere nearer the root of their tree is perfectly acceptable IMO, esp since default themes are configured on a global basis. This is what you have to do anyways if you want to customize anything so a good portion of users are already familiar with it too.

This change would cut some unnecessary LoC, increase clarity in the codebase and make the components less daunting to new contributors! (The theme stuff is kinda scary... admit it!)

[newoga]

We really need to fix this at some point. Passing theme as third parameter doesn't do anything anymore.

[mbrookes]

I can do that as part of this PR if you like since it's semi-related.

[newoga]

Separate note: It seems like this PR removes cases where there are two successive empty lines? Should we just change this rule if that's what we want to enforce? http://eslint.org/docs/rules/no-multiple-empty-lines

[newoga]

Personally, I think we should just maintain the method signature for consistency across the codebase (props, context, state). If I was reading a render method and saw context was being passed as first argument to the method, my gut would think it's a bug.

[mbrookes]

I agree. That was one of the first ones I changed, and must have missed it in the tidy-up when I settled on a consistent signature.

[newoga]

I haven't reviewed the code that thoroughly or tested anything, just reviewed the lines changes. I never liked or fully understood the use of state for muiTheme and was looking forward to when we could remove it. Based on what I know of React and material-ui, this change shouldn't cause any problems, but I haven't really investigated.

@oliviertassinari You have any time to comment on the implications of this change or the historical rationales for why it was done like this?

[mbrookes]

It seems like this PR removes cases where there are two successive empty lines?

Ah, one of my changes accidentally introduced a blank line, so when I realised, I blindly replaced all doubles assuming they were mine. Not sure how to put any existing ones back if there's a preference to keep them!

Should we just change this rule if that's what we want to enforce? http://eslint.org/docs/rules/no-multiple-empty-lines

Might as well! 😄

[mbrookes]

@newoga - thanks for the review. I've applied your changes (bar one or two - see comments), and added a few more!

[newoga]

@newoga - thanks for the review. I've applied your changes (bar one or two - see comments), and added a few more!

Thanks @mbrookes! I know a lot of these issues weren't your fault, but I think addressing them now while they are loosely related may help avoid some of the "why is it written like this" questions later!

[newoga]

Sorry, nitpick: Could we change this (and other similar scenarios previously fixed) to:

if (nextProps.hasOwnProperty('valueSelected')) {
  this.setState({
    selected: nextProps.valueSelected
  });
}

That way the object allocation only occurs in the event the condition passes. 😄

[mbrookes]

Much better.

[anyong]

@mbrookes #4021 is locked (why?) so I'll post this related question here: is it really your intention now that any component we want to test must include the following boilerplate:

import getMuiTheme from 'material-ui/styles/getMuiTheme';

// in component...
childContextTypes:{
  muiTheme: React.PropTypes.object.isRequired,
},
getChildContext() {
  return {muiTheme: getMuiTheme()};
},

whenever there is a mui component somewhere down in that component's render hierarchy? Essentially, that means that every component with more than trivial functionality will need to add these lines just to be tested, so 100 components means somewhere between 300 and 700 extra lines of boilerplate.

You mentioned that you were able to cut 2,000 extra lines from this package, but isn't this simply shifting the extra code from material-ui to me (albeit with simpler boilerplate than the more complex code on your end)? For hundreds or thousands of people with a similar issue, that's potentially millions of lines of boilerplate added to cut 2,000 from material-ui. Maybe I'm misunderstanding the intent or your approach to testing in the new version, so please correct me if I'm wrong, but I feel like there must be a way to make this library test-friendly with necessitating thousands of lines of boilerplate (neither in the library nor in the end-user's code).

Thanks!

[nathanmarks]

Millions of lines of boilerplate?

[nathanmarks]

All you need to do is provide the context somewhere in your rendering tree for fully rendered component trees. And you don't need it at all for shallow unit testing your own components.

[nathanmarks]

Here's how we pass context to our shallow rendered components (needed because ours use the theme as they are the components being shallow rendered).

  const shallowWithContext = (node, context = {}) => {
    return shallow(node, {
      context: {muiTheme, ...context},
    });
  };

We use enzyme for our test utility, but you can do the same thing with the vanilla test utilities by creating a simple wrapper.

import stubContext from '../react-stub-context';
import getMuiTheme from 'styles/getMuiTheme';

function injectTheme(Component, theme) {
  const injectedTheme = theme || getMuiTheme();
  return stubContext(Component, {muiTheme: injectedTheme});
}

module.exports = injectTheme;

[anyong]

provide the context somewhere in your rendering tree

Right, which means that during testing, where we are testing components individually, we need to provide that context for every component that has tests. For an app that makes heavy use of material design, that means providing the context explicitly in essentially every component. Or at least, every component which directly includes any mui component in its render method.

7 extra lines could easily add up to a million given the number of users of this project, and the number of components that include any mui component in a typical material design project.

[anyong]

Ok, the wrapper idea might work, I'll give it a shot. There should probably be docs on this at some point :)

Thanks again for the fast reply.

[nathanmarks]

@anyong np, let me know if you have any continued problems with this issue.

[jason-henriksen]

I'm trying to make material-ui work at all in the react-slingshot environment. I've already spent 2 days trying to get the context thing to work. It works in the webpack example under your code, but in the react-slingshot example I cannot ever get that context to not be null.

This deeply sucks. I have no idea what sets what. I try to set a break point in the constructor but that context variable throws for being undefined before I even seem to get control. None of this makes any sense and god knows what transpilingingifyifyifybableifywtfify is going on. I'm supposed to add things to my component but just dropping the stuff into the component doesn't even compile.

I'm beyond frustrated. I'm sure I'm doing some simple noob thing wrong but all I want is a hello world with a couple of material-ui components showing up in the react-slingshot boiler plate.

I'd love to have working unit tests. Right now I can't even get a button to render in a browser because I've spent literally an entire day trying to understand a Theme system that I really wish I didn't have to care about.

[nathanmarks]

@jason-henriksen You don't need to understand it, just use MuiThemeProvider.

[jason-henriksen]

I'm certain that if I knew how to use it, I'd be quite happy to do so.

[jason-henriksen]

Clearly, I'm very new to the whole eco system. I was quite happy in C# land. I'm doing all of this:

import React, { PropTypes } from 'react';
import { Link, IndexLink } from 'react-router';

import AppBar from 'material-ui/AppBar';
import IconButton from 'material-ui/IconButton';
import IconMenu from 'material-ui/IconMenu';
import MenuItem from 'material-ui/MenuItem';
import MoreVertIcon from 'material-ui/svg-icons/navigation/more-vert';
import ActionHome from 'material-ui/svg-icons/action/home';

import getMuiTheme from 'material-ui/styles/getMuiTheme';
import MuiThemeProvider from 'material-ui/styles/MuiThemeProvider';

import injectTapEventPlugin from 'react-tap-event-plugin';



// Needed for onTouchTap
// Check this repo:
// https://github.com/zilverline/react-tap-event-plugin
injectTapEventPlugin();

const muiTheme = getMuiTheme();

class App extends React.Component{


  render() {
    return (

      <div>

        <MuiThemeProvider muiTheme={muiTheme}>
          <AppBar
            title="Laru TPP-RAM"
            iconElementLeft={<IconButton><ActionHome /></IconButton>}
            iconElementRight={
            <IconMenu
              iconButtonElement={<IconButton><MoreVertIcon /></IconButton>}
              targetOrigin={{horizontal: 'right', vertical: 'top'}}
              anchorOrigin={{horizontal: 'right', vertical: 'top'}}
            >
              <Link to="/fuel-savings"><MenuItem primaryText="Fuel Savings Demo"></MenuItem></Link>
              <Link to="/about"><MenuItem primaryText="Help"></MenuItem></Link>
              <IndexLink to="/"><MenuItem primaryText="Sign out" ></MenuItem></IndexLink>
            </IconMenu>
          }
          />
        </MuiThemeProvider>
        <br/><br/>
        {this.props.children}
      </div>
    );
  }

}

App.childContextTypes = {
  muiTheme: React.PropTypes.object
};

App.propTypes = {
  children: PropTypes.element
};

export default App;

And receiving nothing but this:

TextField.js?ba5b:330
Uncaught TypeError: Cannot read property 'prepareStyles' of undefined

Any ideas?

Edited by @newoga for syntax highlighting

[jason-henriksen]

Ah! And that code links to another object called HomePage that looks like this:

---

import React from 'react';
//import {Link} from 'react-router';

import TextField from 'material-ui/TextField';
import getMuiTheme from 'material-ui/styles/getMuiTheme';
import MuiThemeProvider from 'material-ui/styles/MuiThemeProvider';

const muiTheme = getMuiTheme();

class HomePage extends React.Component {

constructor(props, context) {
super(props, context); // never gets here
// this.handleRequestClose = this.handleRequestClose.bind(this);
// this.handleTouchTap = this.handleTouchTap.bind(this);

}

render() {
return (

  <div>
    <MuiThemeProvider muiTheme={muiTheme}>
    <hr/>
    Test
    <hr/>

    Sample Login Page -- these fail for lack of a context theme.  I honestly could not care less about themes!
    <TextField
      hintText="E-Mail"
      floatingLabelText="Login E-Mail"
      floatingLabelFixed={true}
    />
    <br />
    <TextField
      hintText="Password Field"
      floatingLabelText="Password"
      type="password"
    />
    <br />
    </MuiThemeProvider>
  </div>
);

};
};

export default HomePage;

---

Homepage must be the problem since it's the only thing that contains a TextField. I can guess that react-link is trying to bring it in somehow, but it seems like I'm doing the same thing in this component as the other. So I'm still stuck with no idea what I'm supposed to do.

[newoga]

@jason-henriksen

For future reference, it's a good idea to look and see if open source projects have a contribution guide for where to put these types of questions. We have one here: https://github.com/callemall/material-ui/blob/master/CONTRIBUTING.md

A closed PR is hardly the place to be asking for support. (Friendly reminder)

Your issue is in your App component, the <MuiThemeProvider /> needs to wrap everything that uses material-ui (including {this.props.children}.

[jason-henriksen]

Excellent. Thank you. I had thought from all of the other comments that I needed a MuiThemeProvider tag for each component I built. From that I (wrongly) thought the the props children should be outside of that.

For reference, I had to move the closing tag of down around the {this.props.childeren} tag. Then it complained that MuiThemeProvide cannot take a list, so withing I wrapped everything into another div. At that point it worked!

I am definitely exposing my uncouth lack of integration into polite society here. To make up for it, I'd be happy to extend the example if you're interested to show an example with more sub-components. Alternately, I could just remove these questions, move them to stack overflow and then answer them myself if you prefer.

Thank you again for the help!

[newoga]

@jason-henriksen No worries, thanks for offering and caring to help! 👍 The best way to start helping is learning material-ui, joining the community (we're on Github for project management, and StackOverflow and Gitter for general questions), and pitching in and helping point others that are having issues in the right direction. If we all lead by example then the burden doesn't have to fall on anyone.

For the record, our core team is made up of volunteers who use their free time to make this framework great. That's the main reason why we try to enforce that questions are asked and answered in the right channels and encourage community involvement (so no one has to get burnt out 😄)

[krnaveen14]

This PR was supposed to handle #4021 I guess.

But in my jest snapshot testing of a Component, I get the same issue in <Avatar/> Component.

  TypeError: Cannot read property 'prepareStyles' of undefined
  
       at Avatar.render (node_modules/material-ui/Avatar/Avatar.js:99:48)
       at node_modules/react-test-renderer/lib/ReactCompositeComponent.js:796:21
       at measureLifeCyclePerf (node_modules/react-test-renderer/lib/ReactCompositeComponent.js:75:12)

Am I still required to inject muiTheme in some possible way during standalone component testing?

"material-ui": "^0.18.1"

[oliviertassinari]

Am I still required to inject muiTheme in some possible way during standalone component testing?

Yes you are. That's making the implementation much simpler.
We could revert that on the next branch if we end up using a Higher-order component to inject the style. Then it would be simple to implement a default theme.