[Icon Builder] Add muiName to generated SvgIcons

[mbrookes]

• PR has tests / docs demo, and is linted.
• Commit and PR titles begin with [ComponentName], and are in imperative form: "[Component] Fix leaky abstraction".
• Description explains the issue / use-case resolved, and auto-closes the related issue(s) (http://tr.im/vFqem).

Adding muiName necessitated switching to class syntax to support shouldComponentUpdate

Edit: Removed pure() from the template, and added shouldComponentUpdate to SvgIcon.

After some discussion, just added muiName to the existing template, and updated the build script comments and package.json script.

[oliviertassinari]

Is this needed?

[nathanmarks]

We already have it in our utils folder don't we?

[nathanmarks]

@oliviertassinari We are currently using it from recompose: https://github.com/callemall/material-ui/blob/master/src/TextField/TextField.js#L4

But I think we should get rid of recompose.... all we are using it for is pure() which is an entire component instance just to wrap a component with:

class Pure extends Component {

  shouldComponentUpdate(nextProps) {
    return !shallowEqual(this.props, nextProps);
  }

  render() {}
}

Which should be done normally, it is very little code

[mbrookes]

@oliviertassinari It isn't needed - I had used that first, then removed it from the template, but not from package,json. Done now.

[mbrookes]

@nathanmarks - the only component left using pure() is Stepper! 😆

[oliviertassinari]

Could you give us some context on the use case. In which situation do we need the muiName to be present?

[mbrookes]

The same as any other component that we need to tell it's type, but specifically for Button. 🎉

[oliviertassinari]

Adding muiName necessitated switching to class syntax to support shouldComponentUpdate

I'm having some doubt about this statement. Why can't we add muiName to the prototype of the function, like for the defaultProps or the propTypes?

[mbrookes]

I'm having some doubt about this statement.

Yeah, you're right it was late, I misremembered the code refactor dependancy order. I switched to class to nix pure(), which @nathanmarks pointed out wraps the component in a component, just to add shouldComponentUpdate.

However if we add shouldComponentUpdate to SvgIcon instead, then we shouldn't need it in the svg-icons right? (I know it's better to have it as high up the render tree as possible, but since the svg-icons don't do anything it shouldn't matter if I understand correctly).

And if it's possible to add muiName to an FSC, then we don't need to switch to a class.

[oliviertassinari]

which @nathanmarks pointed out wraps the component in a component, just to add shouldComponentUpdate.

Isn't the purpose of composition? I don't see the win with this change of pure implementation.
The pure HOC do no create a new component as our icons are referentially transparents.
Switching to classes will increase the build size by 300 % (after transpilation, well I should check the number, it's what I remember from trying the two alternatives).
For the history of this change: #3326.

[mbrookes]

Switching to classes will increase the build size by 300 % (after transpilation).

But they aren't classes. :)

f71b5c9

pure() is replaced by shouldComponentUpdate() in SvgIcon.

[oliviertassinari]

I don't think that we should implement a shouldComponentUpdate for any component that accepts a children property. The check will almost always fail unless users are caching their children manually or with babel-plugin-transform-react-constant-elements.

[oliviertassinari]

Or we could tell users to use this babel plugin in production and to parse material-ui and we should be good 😁.

[nathanmarks]

@oliviertassinari thanks for highlighting the main feature I overlooked in this use case.

[0-vortex]

I thought I saw you change 'material-ui/lib/svg-icon' to 'material-ui/SvgIcon' in this PR. Generating icons with --mui-require absolute results in

var _SvgIcon = require('material-ui/lib/SvgIcon');

var _SvgIcon2 = _interopRequireDefault(_SvgIcon);

Is this a typo or did I miss the documentation changes to icon-builder ?

[oliviertassinari]

@vrtxf That's an old issue. The path should be material-ui/SvgIcon. Good catch.
Do you want to submit a PR?

[0-vortex]

sure